dsmiley commented on code in PR #3734:
URL: https://github.com/apache/solr/pull/3734#discussion_r2421626262


##########
solr/core/src/java/org/apache/solr/metrics/otel/instruments/AttributedInstrumentFactory.java:
##########
@@ -68,117 +75,86 @@ public AttributedInstrumentFactory(
   }
 
   public AttributedLongCounter attributedLongCounter(
-      MetricNameProvider metricNameProvider, String description, Attributes 
additionalAttributes) {
+      String metricName, String description, Attributes additionalAttributes) {
     Attributes finalPrimaryAttrs = appendAttributes(primaryAttributes, 
additionalAttributes);
 
     if (aggregateToNodeRegistry && nodeMetricsContext != null) {
       Attributes finalNodeAttrs = appendAttributes(nodeAttributes, 
additionalAttributes);
 
-      LongCounter primaryCounter =
-          
primaryMetricsContext.longCounter(metricNameProvider.getPrimaryMetricName(), 
description);
+      LongCounter primaryCounter = 
primaryMetricsContext.longCounter(metricName, description);
       LongCounter nodeCounter =
-          
nodeMetricsContext.longCounter(metricNameProvider.getNodeMetricName(), 
description);
+          nodeMetricsContext.longCounter(toNodeMetricName(metricName), 
description);
       return new DualRegistryAttributedLongCounter(
           primaryCounter, finalPrimaryAttrs, nodeCounter, finalNodeAttrs);
     } else {
-      String metricName =
-          primaryIsNodeRegistry
-              ? metricNameProvider.getNodeMetricName()
-              : metricNameProvider.getPrimaryMetricName();
-
-      LongCounter counter = primaryMetricsContext.longCounter(metricName, 
description);
+      String finalMetricName = primaryIsNodeRegistry ? 
toNodeMetricName(metricName) : metricName;
+      LongCounter counter = primaryMetricsContext.longCounter(finalMetricName, 
description);
       return new AttributedLongCounter(counter, finalPrimaryAttrs);
     }
   }
 
   public AttributedLongUpDownCounter attributedLongUpDownCounter(
-      MetricNameProvider metricNameProvider, String description, Attributes 
additionalAttributes) {
+      String metricName, String description, Attributes additionalAttributes) {
     Attributes finalPrimaryAttrs = appendAttributes(primaryAttributes, 
additionalAttributes);
 
     if (aggregateToNodeRegistry && nodeMetricsContext != null) {
       Attributes finalNodeAttrs = appendAttributes(nodeAttributes, 
additionalAttributes);
 
       LongUpDownCounter primaryCounter =
-          primaryMetricsContext.longUpDownCounter(
-              metricNameProvider.getPrimaryMetricName(), description);
+          primaryMetricsContext.longUpDownCounter(metricName, description);
       LongUpDownCounter nodeCounter =
-          
nodeMetricsContext.longUpDownCounter(metricNameProvider.getNodeMetricName(), 
description);
-
+          nodeMetricsContext.longUpDownCounter(toNodeMetricName(metricName), 
description);
       return new DualRegistryAttributedLongUpDownCounter(
           primaryCounter, finalPrimaryAttrs, nodeCounter, finalNodeAttrs);
     } else {
-      String metricName =
-          primaryIsNodeRegistry
-              ? metricNameProvider.getNodeMetricName()
-              : metricNameProvider.getPrimaryMetricName();
-
-      LongUpDownCounter counter = 
primaryMetricsContext.longUpDownCounter(metricName, description);
+      String finalMetricName = primaryIsNodeRegistry ? 
toNodeMetricName(metricName) : metricName;
+      LongUpDownCounter counter =
+          primaryMetricsContext.longUpDownCounter(finalMetricName, 
description);
       return new AttributedLongUpDownCounter(counter, finalPrimaryAttrs);
     }
   }
 
   public AttributedLongTimer attributedLongTimer(
-      MetricNameProvider metricNameProvider,
-      String description,
-      OtelUnit unit,
-      Attributes additionalAttributes) {
+      String metricName, String description, OtelUnit unit, Attributes 
additionalAttributes) {
     Attributes finalPrimaryAttrs = appendAttributes(primaryAttributes, 
additionalAttributes);
 
-    if (aggregateToNodeRegistry) {
+    if (aggregateToNodeRegistry && nodeMetricsContext != null) {
       Attributes finalNodeAttrs = appendAttributes(nodeAttributes, 
additionalAttributes);
-      LongHistogram coreHistogram =
-          primaryMetricsContext.longHistogram(
-              metricNameProvider.getPrimaryMetricName(), description, unit);
+      LongHistogram primaryHistogram =
+          primaryMetricsContext.longHistogram(metricName, description, unit);
       LongHistogram nodeHistogram =
-          nodeMetricsContext.longHistogram(
-              metricNameProvider.getNodeMetricName(), description, unit);
+          nodeMetricsContext.longHistogram(toNodeMetricName(metricName), 
description, unit);
       return new DualRegistryAttributedLongTimer(
-          coreHistogram, finalPrimaryAttrs, nodeHistogram, finalNodeAttrs);
+          primaryHistogram, finalPrimaryAttrs, nodeHistogram, finalNodeAttrs);
     } else {
-      String metricName =
-          primaryIsNodeRegistry
-              ? metricNameProvider.getNodeMetricName()
-              : metricNameProvider.getPrimaryMetricName();
-
-      LongHistogram histogram = 
primaryMetricsContext.longHistogram(metricName, description, unit);
+      String finalMetricName = primaryIsNodeRegistry ? 
toNodeMetricName(metricName) : metricName;
+      LongHistogram histogram =
+          primaryMetricsContext.longHistogram(finalMetricName, description, 
unit);
       return new AttributedLongTimer(histogram, finalPrimaryAttrs);
     }
   }
 
-  // Filter out core attributes and keep only category and handler if they 
exist
+  // Replace core metric name prefix to node prefix
+  private String toNodeMetricName(String coreMetricName) {
+    return coreMetricName.replace("solr_core", "solr_node");
+  }
+
+  // Filter out core attributes and keep all others for node-level metrics

Review Comment:
   Same here; javadocs.



##########
solr/core/src/java/org/apache/solr/metrics/otel/instruments/AttributedInstrumentFactory.java:
##########
@@ -68,117 +75,86 @@ public AttributedInstrumentFactory(
   }
 
   public AttributedLongCounter attributedLongCounter(
-      MetricNameProvider metricNameProvider, String description, Attributes 
additionalAttributes) {
+      String metricName, String description, Attributes additionalAttributes) {
     Attributes finalPrimaryAttrs = appendAttributes(primaryAttributes, 
additionalAttributes);
 
     if (aggregateToNodeRegistry && nodeMetricsContext != null) {
       Attributes finalNodeAttrs = appendAttributes(nodeAttributes, 
additionalAttributes);
 
-      LongCounter primaryCounter =
-          
primaryMetricsContext.longCounter(metricNameProvider.getPrimaryMetricName(), 
description);
+      LongCounter primaryCounter = 
primaryMetricsContext.longCounter(metricName, description);
       LongCounter nodeCounter =
-          
nodeMetricsContext.longCounter(metricNameProvider.getNodeMetricName(), 
description);
+          nodeMetricsContext.longCounter(toNodeMetricName(metricName), 
description);
       return new DualRegistryAttributedLongCounter(
           primaryCounter, finalPrimaryAttrs, nodeCounter, finalNodeAttrs);
     } else {
-      String metricName =
-          primaryIsNodeRegistry
-              ? metricNameProvider.getNodeMetricName()
-              : metricNameProvider.getPrimaryMetricName();
-
-      LongCounter counter = primaryMetricsContext.longCounter(metricName, 
description);
+      String finalMetricName = primaryIsNodeRegistry ? 
toNodeMetricName(metricName) : metricName;
+      LongCounter counter = primaryMetricsContext.longCounter(finalMetricName, 
description);
       return new AttributedLongCounter(counter, finalPrimaryAttrs);
     }
   }
 
   public AttributedLongUpDownCounter attributedLongUpDownCounter(
-      MetricNameProvider metricNameProvider, String description, Attributes 
additionalAttributes) {
+      String metricName, String description, Attributes additionalAttributes) {
     Attributes finalPrimaryAttrs = appendAttributes(primaryAttributes, 
additionalAttributes);
 
     if (aggregateToNodeRegistry && nodeMetricsContext != null) {
       Attributes finalNodeAttrs = appendAttributes(nodeAttributes, 
additionalAttributes);
 
       LongUpDownCounter primaryCounter =
-          primaryMetricsContext.longUpDownCounter(
-              metricNameProvider.getPrimaryMetricName(), description);
+          primaryMetricsContext.longUpDownCounter(metricName, description);
       LongUpDownCounter nodeCounter =
-          
nodeMetricsContext.longUpDownCounter(metricNameProvider.getNodeMetricName(), 
description);
-
+          nodeMetricsContext.longUpDownCounter(toNodeMetricName(metricName), 
description);
       return new DualRegistryAttributedLongUpDownCounter(
           primaryCounter, finalPrimaryAttrs, nodeCounter, finalNodeAttrs);
     } else {
-      String metricName =
-          primaryIsNodeRegistry
-              ? metricNameProvider.getNodeMetricName()
-              : metricNameProvider.getPrimaryMetricName();
-
-      LongUpDownCounter counter = 
primaryMetricsContext.longUpDownCounter(metricName, description);
+      String finalMetricName = primaryIsNodeRegistry ? 
toNodeMetricName(metricName) : metricName;
+      LongUpDownCounter counter =
+          primaryMetricsContext.longUpDownCounter(finalMetricName, 
description);
       return new AttributedLongUpDownCounter(counter, finalPrimaryAttrs);
     }
   }
 
   public AttributedLongTimer attributedLongTimer(
-      MetricNameProvider metricNameProvider,
-      String description,
-      OtelUnit unit,
-      Attributes additionalAttributes) {
+      String metricName, String description, OtelUnit unit, Attributes 
additionalAttributes) {
     Attributes finalPrimaryAttrs = appendAttributes(primaryAttributes, 
additionalAttributes);
 
-    if (aggregateToNodeRegistry) {
+    if (aggregateToNodeRegistry && nodeMetricsContext != null) {
       Attributes finalNodeAttrs = appendAttributes(nodeAttributes, 
additionalAttributes);
-      LongHistogram coreHistogram =
-          primaryMetricsContext.longHistogram(
-              metricNameProvider.getPrimaryMetricName(), description, unit);
+      LongHistogram primaryHistogram =
+          primaryMetricsContext.longHistogram(metricName, description, unit);
       LongHistogram nodeHistogram =
-          nodeMetricsContext.longHistogram(
-              metricNameProvider.getNodeMetricName(), description, unit);
+          nodeMetricsContext.longHistogram(toNodeMetricName(metricName), 
description, unit);
       return new DualRegistryAttributedLongTimer(
-          coreHistogram, finalPrimaryAttrs, nodeHistogram, finalNodeAttrs);
+          primaryHistogram, finalPrimaryAttrs, nodeHistogram, finalNodeAttrs);
     } else {
-      String metricName =
-          primaryIsNodeRegistry
-              ? metricNameProvider.getNodeMetricName()
-              : metricNameProvider.getPrimaryMetricName();
-
-      LongHistogram histogram = 
primaryMetricsContext.longHistogram(metricName, description, unit);
+      String finalMetricName = primaryIsNodeRegistry ? 
toNodeMetricName(metricName) : metricName;
+      LongHistogram histogram =
+          primaryMetricsContext.longHistogram(finalMetricName, description, 
unit);
       return new AttributedLongTimer(histogram, finalPrimaryAttrs);
     }
   }
 
-  // Filter out core attributes and keep only category and handler if they 
exist
+  // Replace core metric name prefix to node prefix

Review Comment:
   that is a plain comment but I think should be javadoc.  (I merely mean to 
tweak the slash style; no need to document anything more)



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

Reply via email to