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