[GitHub] [storm] agresch commented on a change in pull request #3251: STORM-3623 executors should only report their metrics for V2 metric tick

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

agresch commented on a change in pull request #3251:
URL: https://github.com/apache/storm/pull/3251#discussion_r412261965



##########
File path: storm-client/src/jvm/org/apache/storm/executor/Executor.java
##########
@@ -339,47 +342,106 @@ private void addV2Metrics(List<IMetricsConsumer.DataPoint> dataPoints) {
             return;
         }
         StormMetricRegistry stormMetricRegistry = workerData.getMetricRegistry();
-        for (Map.Entry<String, Gauge> entry : stormMetricRegistry.registry().getGauges().entrySet()) {
-            String name = entry.getKey();
-            Object v = entry.getValue().getValue();
-            if (v instanceof Number) {
-                IMetricsConsumer.DataPoint dataPoint = new IMetricsConsumer.DataPoint(name, v);
-                dataPoints.add(dataPoint);
-            } else {
-                LOG.warn("Cannot report {}, its value is not a Number {}", name, v);
+        Map<String, Metric> metrics = stormMetricRegistry.getExecutorMetrics(this);
+        processGauges(metrics, dataPoints);
+        processCounters(metrics, dataPoints);
+        processHistograms(metrics, dataPoints);
+        processMeters(metrics, dataPoints);
+        processTimers(metrics, dataPoints);
+
+        // at this point, we should have no metrics left to process
+        if (!metrics.isEmpty()) {
+            for (Map.Entry<String, Metric> entry : metrics.entrySet()) {
+                LOG.warn("Unable to process metric {} {}", entry.getKey(), entry.getValue());
             }
         }
-        for (Map.Entry<String, Counter> entry : stormMetricRegistry.registry().getCounters().entrySet()) {
-            Object value = entry.getValue().getCount();
-            IMetricsConsumer.DataPoint dataPoint = new IMetricsConsumer.DataPoint(entry.getKey(), value);
-            dataPoints.add(dataPoint);
+    }
+
+    private void processGauges(Map<String, Metric> metrics, List<IMetricsConsumer.DataPoint> dataPoints) {
+        Set<String> processedMetrics = new HashSet<>();

Review comment:
       We would need something like getExecutorGauges().  I think it's overkill, but I can add it if desired.  
   
   I don't think extending com.codahale.metrics.MetricRegistry is a good idea.  We should enforce API that tie registering metrics with the taskids so we can track.  What benefit would we get?




----------------------------------------------------------------
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.

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