Copilot commented on code in PR #3552:
URL: https://github.com/apache/fluss/pull/3552#discussion_r3518094463
##########
fluss-metrics/fluss-metrics-prometheus/src/main/java/org/apache/fluss/metrics/prometheus/PrometheusPushGatewayReporter.java:
##########
@@ -46,12 +52,18 @@ public PrometheusPushGatewayReporter(
String jobName,
Map<String, String> groupingKey,
final boolean deleteOnShutdown,
- Duration pushInterval) {
+ Duration pushInterval,
+ @Nullable String username,
+ @Nullable String password) {
this.pushGateway = new PushGateway(hostUrl);
this.jobName = jobName;
this.groupingKey = groupingKey;
this.deleteOnShutdown = deleteOnShutdown;
this.pushInterval = pushInterval;
+ if (username != null && !username.isEmpty()) {
+ this.pushGateway.setConnectionFactory(
+ basicAuthConnectionFactory(username, password == null ? ""
: password));
+ }
Review Comment:
`username` is treated as configured when it is non-empty, but
whitespace-only values (e.g. " ") will still enable Basic Auth and send an
invalid `Authorization` header. This is inconsistent with the plugin, which
treats whitespace-only usernames as disabled. Consider using the same
`StringUtils.isNullOrWhitespaceOnly` check here as well.
##########
fluss-common/src/main/java/org/apache/fluss/config/ConfigOptions.java:
##########
@@ -2171,6 +2171,22 @@ public class ConfigOptions {
.withDescription(
"The interval of pushing metrics to
Prometheus PushGateway.");
+ public static final ConfigOption<String>
METRICS_REPORTER_PROMETHEUS_PUSHGATEWAY_USERNAME =
+ key("metrics.reporter.prometheus-push.username")
+ .stringType()
+ .noDefaultValue()
+ .withDescription(
+ "The username for Basic Auth of the Prometheus
PushGateway. "
+ + "Leave it unset to disable
authentication.");
+
+ public static final ConfigOption<String>
METRICS_REPORTER_PROMETHEUS_PUSHGATEWAY_PASSWORD =
+ key("metrics.reporter.prometheus-push.password")
+ .stringType()
+ .noDefaultValue()
+ .withDescription(
+ "The password for Basic Auth of the Prometheus
PushGateway. "
+ + "Only takes effect when username is
configured.");
Review Comment:
`metrics.reporter.prometheus-push.password` is defined as a plain `String`
option, which means it won’t be treated as sensitive by the configuration
subsystem (e.g., it may be included in config dumps or error messages).
Consider defining it as a `Password` option via `.passwordType()` so it is
automatically redacted when configurations are logged or displayed.
##########
fluss-metrics/fluss-metrics-prometheus/src/main/java/org/apache/fluss/metrics/prometheus/PrometheusPushGatewayReporter.java:
##########
@@ -80,4 +92,17 @@ public void report() {
LOG.warn("Could not push metrics to PushGateway.", e);
}
}
+
+ private static HttpConnectionFactory basicAuthConnectionFactory(String
user, String password) {
+ final String header =
+ "Basic "
+ + Base64.getEncoder()
+ .encodeToString(
+ (user + ":" +
password).getBytes(StandardCharsets.UTF_8));
Review Comment:
The PR description says `BasicAuthHttpConnectionFactory` is injected into
`PushGateway`, but the implementation here uses a custom
`HttpConnectionFactory`. Please either update the PR description to match the
implementation, or switch to the Prometheus client’s built-in factory if it is
available in the dependency version used by this module.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
