Copilot commented on code in PR #13438:
URL: https://github.com/apache/gravitino/pull/13438#discussion_r4073216201
##########
core/src/main/java/org/apache/gravitino/metrics/MetricsSystem.java:
##########
@@ -223,4 +244,71 @@ private void registerMetricsToPrometheusRegistry() {
public MetricsServlet getPrometheusServlet() {
return new MetricsServlet(prometheusRegistry);
}
+
+ /**
+ * Forwards metrics added to or removed from a source's registry to the
shared registry under
+ * {@code "{metricsSourceName}.{metricName}"} while the source is
registered. Held by {@link
+ * MetricsSystem} so the link can be severed on unregister.
+ */
+ private class SourceMetricsListener extends MetricRegistryListener.Base {
+ private final String prefix;
+
+ SourceMetricsListener(String metricsSourceName) {
+ this.prefix = metricsSourceName + ".";
+ }
+
+ private String prefixed(String name) {
+ return prefix + name;
+ }
+
+ @Override
+ public void onGaugeAdded(String name, Gauge<?> gauge) {
+ metricRegistry.register(prefixed(name), gauge);
+ }
Review Comment:
`MetricRegistry.register(...)` throws `IllegalArgumentException` if a metric
with the same name already exists. With listener-based forwarding, this can
happen if the shared registry already contains the prefixed name (e.g.,
external registration, partial cleanup, or concurrent add/remove). To make
forwarding robust, defensively handle duplicates (e.g., remove existing before
register if safe, or catch `IllegalArgumentException` and log/skip) for all
`on*Added` methods.
##########
core/src/main/java/org/apache/gravitino/metrics/MetricsSystem.java:
##########
@@ -114,6 +129,12 @@ public synchronized void unregister(MetricsSource
metricsSource) {
return;
}
this.metricSources.remove(metricsSource.getMetricsSourceName());
+ MetricRegistryListener listener =
sourceListeners.remove(metricsSource.getMetricsSourceName());
+ if (listener != null) {
+ // Sever the live link so a stale source cannot re-inject lazily created
+ // metrics into the shared registry after being unregistered.
+ metricsSource.getMetricRegistry().removeListener(listener);
+ }
metricRegistry.removeMatching(
MetricFilter.startsWith(metricsSource.getMetricsSourceName() + "."));
Review Comment:
There’s a race where a source thread can add a metric at the same time
`unregister()` runs: the listener callback may `register()` into the shared
registry after `removeListener()` (if already in-flight) and/or after
`removeMatching()`, leaving a ghost metric behind. Consider serializing
listener callbacks with unregister/register by synchronizing the listener’s
`on*Added/on*Removed` bodies on `MetricsSystem.this` (or using a lock), and/or
adding an `AtomicBoolean active` in `SourceMetricsListener` that is set false
before `removeListener()` and checked inside callbacks to no-op when inactive.
--
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]
