abhishekrb19 commented on code in PR #14728:
URL: https://github.com/apache/druid/pull/14728#discussion_r1306721363
##########
extensions-contrib/prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/PrometheusEmitterConfig.java:
##########
@@ -137,6 +149,12 @@ public boolean isAddServiceAsLabel()
return addServiceAsLabel;
}
+ @Nullable
Review Comment:
Same comment as above - this can be empty but not null:
```suggestion
```
##########
extensions-contrib/prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/PrometheusEmitterConfig.java:
##########
@@ -94,6 +101,11 @@ public PrometheusEmitterConfig(
this.flushPeriod = flushPeriod;
this.addHostAsLabel = addHostAsLabel;
this.addServiceAsLabel = addServiceAsLabel;
+ this.extraLabels = extraLabels != null ? extraLabels :
Collections.emptyMap();
+ // Validate label names early to prevent Prometheus exceptions later.
+ for (String key : this.extraLabels.keySet()) {
+ Preconditions.checkArgument(PATTERN.matcher(key).matches(), "Invalid
metric label name: " + key);
Review Comment:
Perhaps we can use the `DruidException` class to surface this error to the
operator:
```suggestion
throw DruidException.forPersona(DruidException.Persona.OPERATOR)
.ofCategory(DruidException.Category.INVALID_INPUT)
.build(
StringUtils.format(
"Invalid metric label name [%s]. Label
names must conform to the pattern [%s].",
key,
PATTERN.pattern()
)
);
```
##########
extensions-contrib/prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/PrometheusEmitter.java:
##########
@@ -136,6 +137,12 @@ private void emitMetric(ServiceMetricEvent metricEvent)
if (metric != null) {
String[] labelValues = new String[metric.getDimensions().length];
String[] labelNames = metric.getDimensions();
+
+ Map<String, String> extraLabels = config.getExtraLabels();
+ if (extraLabels == null) {
+ extraLabels = Collections.emptyMap(); // Avoid null checks later
+ }
Review Comment:
I think `config.getExtraLabels()` is guaranteed to be non-null at this
point, given it's initialized to an `emptyMap` in the constructor if it's null:
```suggestion
```
##########
extensions-contrib/prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/PrometheusEmitterConfig.java:
##########
@@ -63,6 +65,10 @@ public class PrometheusEmitterConfig
@JsonProperty
private final boolean addServiceAsLabel;
+ @JsonProperty
+ @Nullable
Review Comment:
Since it's initialized with `Collections.emptyMap()` we can drop this
annotation:
```suggestion
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
