[GitHub] [storm] Ethanlm commented on a change in pull request #3329: STORM-3694 allow reporting V2 metrics with dimensions and short names

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

Ethanlm commented on a change in pull request #3329:
URL: https://github.com/apache/storm/pull/3329#discussion_r489883300



##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/StormMetricRegistry.java
##########
@@ -171,6 +188,59 @@ public Histogram histogram(String name, TopologyContext context) {
         metrics.put(names.getV2TickName(), metric);
     }
 
+    private <T> Gauge<T> registerGauge(String name, MetricNames metricNames, Gauge<T> gauge, int taskId,
+                                       String componentId, String streamId) {
+        TaskMetricDimensions taskMetricDimensions = new TaskMetricDimensions(taskId, componentId, streamId, this);
+        synchronized (this) {

Review comment:
       Why do we need `synchronized`? 
   
   If there are two threads trying to register the same metric,
    line198 `gauge = registry.register(metricNames.getLongName(), gauge);` will throw `IllegalArgumentException`. (what's interesting is if registry.gauge() or registry.meter() etc. method is invoked, it will check and return the existing object if possible)
   
   It implies this `registerGauge` can only be invoked once for each long name.
   

##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/StormMetricRegistry.java
##########
@@ -171,6 +188,59 @@ public Histogram histogram(String name, TopologyContext context) {
         metrics.put(names.getV2TickName(), metric);
     }
 
+    private <T> Gauge<T> registerGauge(String name, MetricNames metricNames, Gauge<T> gauge, int taskId,
+                                       String componentId, String streamId) {
+        TaskMetricDimensions taskMetricDimensions = new TaskMetricDimensions(taskId, componentId, streamId, this);
+        synchronized (this) {
+            TaskMetricRepo repo = taskMetrics.computeIfAbsent(taskMetricDimensions, (k) -> new TaskMetricRepo());
+            repo.addGauge(name, gauge);

Review comment:
       Can we use `metricNames.getV2TickName` instead of `name`? It seems code duplication to pass this `name` parameter?
   
   I would also change `metricNames.getV2TickName` to `metricNames.getShortName` since it is not only about v2 tick anymore.
   
   And we need to update comments at https://github.com/apache/storm/blob/master/storm-client/src/jvm/org/apache/storm/metrics2/StormMetricRegistry.java#L316-L317

##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/StormMetricRegistry.java
##########
@@ -198,14 +268,17 @@ public Histogram histogram(String name, TopologyContext context) {
         return getMetricNameMap(taskId, taskIdTimers);
     }
 
-    public void start(Map<String, Object> topoConf) {
+    public void start(Map<String, Object> topoConf, int port) {
         try {
             hostName = dotToUnderScore(Utils.localHostname());
         } catch (UnknownHostException e) {
             LOG.warn("Unable to determine hostname while starting the metrics system. Hostname will be reported"
                      + " as 'localhost'.");
         }
 
+        this.topologyName = (String) topoConf.get(Config.TOPOLOGY_NAME);

Review comment:
       should we use  `topologyId` since we use topologyId in the long name

##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/reporters/StormReporter.java
##########
@@ -15,12 +15,15 @@
 import com.codahale.metrics.MetricRegistry;
 import com.codahale.metrics.Reporter;
 import java.util.Map;
+import org.apache.storm.metrics2.StormMetricRegistry;
 
 public interface StormReporter extends Reporter {
     String REPORT_PERIOD = "report.period";
     String REPORT_PERIOD_UNITS = "report.period.units";
+    String REPORT_DIMENSIONS_ENABLED = "report.dimensions.enabled";
 
-    void prepare(MetricRegistry metricsRegistry, Map<String, Object> topoConf, Map<String, Object> reporterConf);
+    void prepare(MetricRegistry metricsRegistry, Map<String, Object> topoConf, Map<String, Object> reporterConf,

Review comment:
       The interface change is concerning. It breaks backwards compatibility. 

##########
File path: storm-client/src/jvm/org/apache/storm/metrics2/reporters/ScheduledStormReporter.java
##########
@@ -37,6 +37,10 @@ public static long getReportPeriod(Map<String, Object> reporterConf) {
         return ObjectReader.getInt(reporterConf.get(REPORT_PERIOD), 10).longValue();
     }
 
+    public static boolean getReportDimensions(Map<String, Object> reporterConf) {

Review comment:
       I would suggest to change this to `getReportDimensionsEnabled` or `isReportDimensionsEnabled`  




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