[GitHub] [druid] kfaraz commented on a diff in pull request #12769: Add druid_service and host_name labels to Prometheus exporter

[GitBox <gi...@apache.org>]

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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org