kfaraz commented on code in PR #12769:
URL: https://github.com/apache/druid/pull/12769#discussion_r918984863
##########
extensions-contrib/prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/PrometheusEmitterConfig.java:
##########
@@ -99,6 +109,16 @@ public Strategy getStrategy()
return strategy;
}
+ public boolean isHostAsLabel()
Review Comment:
Suggestion: If we do decide to include these flags, better names would be
`includeHost`, `includeHostAsLabel` or `addHostAsLabel`. Although, I see that
even `StatsDEmitter` is inconsistent in its naming of `includeHost` and
`dogstatsdServiceAsTag`.
##########
extensions-contrib/prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/Metrics.java:
##########
@@ -49,6 +49,9 @@
private final ObjectMapper mapper = new ObjectMapper();
public static final Pattern PATTERN =
Pattern.compile("[^a-zA-Z_:][^a-zA-Z0-9_:]*");
+ private static final String HOST_LABEL_NAME = "hostName";
+ private static final String SERVICE_LABEL_NAME = "druidService";
Review Comment:
For consistency with the other emitters, say `StatsDEmitter`,
```suggestion
private static final String TAG_HOSTNAME = "host_name";
private static final String TAG_SERVICE = "druid_service";
```
##########
extensions-contrib/prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/Metrics.java:
##########
@@ -58,13 +61,22 @@ public DimensionsAndCollector getByName(String name, String
service)
}
}
- public Metrics(String namespace, String path)
+ public Metrics(String namespace, String path, boolean isIncludeHost, boolean
isServiceAsTag)
{
Map<String, Metric> metrics = readConfig(path);
for (Map.Entry<String, Metric> entry : metrics.entrySet()) {
String name = entry.getKey();
Metric metric = entry.getValue();
Metric.Type type = metric.type;
+
+ if (isIncludeHost) {
Review Comment:
I am not sure if we need these flags. Can't we always include these
dimensions?
I guess it would create a separate time series though according to
Prometheus docs:
> Changing any label value, including adding or removing a label, will
create a new time series.
##########
extensions-contrib/prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/PrometheusEmitter.java:
##########
@@ -142,6 +146,25 @@ private void emitMetric(ServiceMetricEvent metricEvent)
}
}
+ private String fillLabelValue(String lableName, Object userDim, boolean
isIncludeHost, String hostName, boolean isServiceAsTag, String serviceName)
Review Comment:
Nit:
`isIncludeHost` and `isServiceTag` are not needed as arguments. They can
always be obtained from the `config`.
```suggestion
private String getLabelValue(String labelName, Object userDim, String
hostName, String serviceName)
```
##########
extensions-contrib/prometheus-emitter/pom.xml:
##########
@@ -43,17 +43,17 @@
<dependency>
<groupId>io.prometheus</groupId>
<artifactId>simpleclient</artifactId>
- <version>0.7.0</version>
+ <version>0.16.0</version>
Review Comment:
Thanks!
I assume this version is fully backward compatible and doesn't cause any
upgrade issues.
##########
extensions-contrib/prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/PrometheusEmitter.java:
##########
@@ -142,6 +146,25 @@ private void emitMetric(ServiceMetricEvent metricEvent)
}
}
+ private String fillLabelValue(String lableName, Object userDim, boolean
isIncludeHost, String hostName, boolean isServiceAsTag, String serviceName)
Review Comment:
You could even move this if-else to the calling method itself.
- this method is called only from that one place
- the contents are fairly small and would be easier to follow there
- having to pass `hostName` and `serviceName` to this method is a little
weird
--
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]
