Re: [PR] [refactor] Move metrics plugin tests out of pinot-common to untangle test deps [pinot]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #18259:
URL: https://github.com/apache/pinot/pull/18259#discussion_r3107622509


##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/AbstractMetrics.java:
##########
@@ -502,14 +503,27 @@ public void addValueToGlobalGauge(final G gauge, final long unitCount) {
   }
 
   /**
-   * Get the gauge metric value for the provided gauge name
-   * @param gaugeName gauge name
-   * @return gauge value. If gauge is not present return null.
+   * Get the gauge metric value for the provided gauge name.
+   * Checks the internal value map first (covers legacy setValueOf* APIs), then falls back to looking up
+   * the metric in the registry (covers supplier-based gauges created via setOrUpdateGauge).
+   * @param gaugeName gauge name (without the metric prefix)
+   * @return gauge value, or null if the gauge is not present
    */
   @Nullable
   public Long getGaugeValue(final String gaugeName) {
     AtomicLong value = _gaugeValues.get(gaugeName);
-    return value != null ? value.get() : null;
+    if (value != null) {
+      return value.get();
+    }
+    PinotMetric metric =
+        _metricsRegistry.allMetrics().get(PinotMetricUtils.makePinotMetricName(_clazz, _metricPrefix + gaugeName));
+    if (metric instanceof PinotGauge) {
+      Object gaugeValue = ((PinotGauge<?>) metric).value();
+      if (gaugeValue instanceof Number) {
+        return ((Number) gaugeValue).longValue();
+      }
+    }
+    return null;

Review Comment:
   `getGaugeValue()` fallback lookup via `_metricsRegistry.allMetrics()` is currently ineffective for supplier-based gauges: both `YammerMetricsRegistry#allMetrics()` and `DropwizardMetricsRegistry#allMetrics()` wrap underlying gauges in `YammerMetric`/`DropwizardMetric` (implement `PinotMetric` only), so the retrieved value is never a `PinotGauge` and `instanceof PinotGauge` will always be false. This means gauges registered via `setOrUpdateGauge(name, Supplier)` will still return null (e.g., merge/rollup controller gauges registered with suppliers). Consider instead tracking the returned `PinotGauge` from `setOrUpdateGauge*()` in a map keyed by gaugeName and reading `pinotGauge.value()` here, or providing a registry API that can return the concrete `PinotGauge` for a name without allocating/wrapping `allMetrics()` each call.



-- 
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@pinot.apache.org

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


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