Add druid_service and host_name labels to Prometheus exporter #12769
Conversation
|
CI failure is unrelated to this change. @FrankChen021 |
There was a problem hiding this comment.
Thanks for the changes, @liujianhuanzz !
I have left some comments.
| private static final String HOST_LABEL_NAME = "hostName"; | ||
| private static final String SERVICE_LABEL_NAME = "druidService"; |
There was a problem hiding this comment.
For consistency with the other emitters, say StatsDEmitter,
| private static final String HOST_LABEL_NAME = "hostName"; | |
| private static final String SERVICE_LABEL_NAME = "druidService"; | |
| private static final String TAG_HOSTNAME = "host_name"; | |
| private static final String TAG_SERVICE = "druid_service"; |
| { | ||
| 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) { |
There was a problem hiding this 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.
There was a problem hiding this comment.
These flags are used to maintain compatibility.
| @@ -142,6 +146,25 @@ private void emitMetric(ServiceMetricEvent metricEvent) | |||
| } | |||
| } | |||
|
|
|||
| private String fillLabelValue(String lableName, Object userDim, boolean isIncludeHost, String hostName, boolean isServiceAsTag, String serviceName) | |||
There was a problem hiding this comment.
Nit:
isIncludeHost and isServiceTag are not needed as arguments. They can always be obtained from the config.
| private String fillLabelValue(String lableName, Object userDim, boolean isIncludeHost, String hostName, boolean isServiceAsTag, String serviceName) | |
| private String getLabelValue(String labelName, Object userDim, String hostName, String serviceName) |
There was a problem hiding this 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
hostNameandserviceNameto this method is a little weird
| @@ -99,6 +109,16 @@ public Strategy getStrategy() | |||
| return strategy; | |||
| } | |||
|
|
|||
| public boolean isHostAsLabel() | |||
There was a problem hiding this 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.
| @@ -43,17 +43,17 @@ | |||
| <dependency> | |||
| <groupId>io.prometheus</groupId> | |||
| <artifactId>simpleclient</artifactId> | |||
| <version>0.7.0</version> | |||
| <version>0.16.0</version> | |||
There was a problem hiding this comment.
Thanks!
I assume this version is fully backward compatible and doesn't cause any upgrade issues.
There was a problem hiding this comment.
Thanks for the changes, @liujianhuanzz .
Minor comment on docs.
Overall, LGTM 🚀
| @@ -40,6 +40,8 @@ All the configuration parameters for the Prometheus emitter are under `druid.emi | |||
| |`druid.emitter.prometheus.port`|The port on which to expose the prometheus HTTPServer. Required if using exporter strategy.|no|none| | |||
| |`druid.emitter.prometheus.namespace`|Optional metric namespace. Must match the regex `[a-zA-Z_:][a-zA-Z0-9_:]*`|no|"druid"| | |||
| |`druid.emitter.prometheus.dimensionMapPath`|JSON file defining the Prometheus metric type, desired dimensions, help text, and conversionFactor for every Druid metric.|no|Default mapping provided. See below.| | |||
| |`druid.emitter.prometheus.addHostAsLabel`|Flag to include the hostname as a prometheus label.|no|false| | |||
| |`druid.emitter.prometheus.addServiceAsLabel`|If `druid.emitter.prometheus.addServiceAsLabel` is true, druid service (e.g. `druid/broker`, `druid/coordinator`, etc) is reported as a tag (e.g. `druid_service:druid/broker`)|no|false| | |||
There was a problem hiding this comment.
Nit: similar to addHostAsLabel.
| |`druid.emitter.prometheus.addServiceAsLabel`|If `druid.emitter.prometheus.addServiceAsLabel` is true, druid service (e.g. `druid/broker`, `druid/coordinator`, etc) is reported as a tag (e.g. `druid_service:druid/broker`)|no|false| | |
| |`druid.emitter.prometheus.addServiceAsLabel`|Flag to include the druid service name (e.g. `druid/broker`, `druid/coordinator`, etc.) as a prometheus label.|no|false| |
Thanks |
|
CI failure is unrelated to this change. |
Fixes #12731
Description
Some labels are missing from the Prometheus exporter. They were in the Statsd exporter, such as druid_service and host_name, Which makes the result of Prometheus exporter incompatible with the stated exporter.
In this current implementation, these two labels can be optionally added.
Key changed/added classes in this PR
docs/development/extensions-contrib/prometheus.mdextensions-contrib/prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/Metrics.javaextensions-contrib/prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/PrometheusEmitter.javaextensions-contrib/prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/PrometheusEmitterConfig.javaextensions-contrib/prometheus-emitter/src/test/java/org/apache/druid/emitter/prometheus/MetricsTest.javaextensions-contrib/prometheus-emitter/src/test/java/org/apache/druid/emitter/prometheus/PrometheusEmitterTest.javaThis PR has: