Make JvmMetrics.register idempotent with the default registry

[mimaison]

This makes JvmMetrics.register(PrometheusRegistry registry) behave like JvmMetrics.register() when the specified registry is the default registry.

[dhoard]

@mimaison can you elaborate on the need for this change?

I have concerns regarding the conditional behavior based on the PrometheusRegistry instance. Code should never call JvmMetrics.register(PrometheusRegistry.defaultRegistry) more than once.

[mimaison]

We're using this client to build a Prometheus metrics reporter plugin for Kafka (brokers and client) in Strimzi (another CNCF project). The reporter is configured per Kafka client and it's common to have multiple Kafka clients in the same JVM. The JvmMetrics.register() method is great because it allows each reporter instances to unconditionally call it without having to track if another instance already did it.

However because we can't clear the default registry, we updated our code to pass PrometheusRegistry instances. At runtime, we always use the default registry, but tests create their own PrometheusRegistry instances. With this change we could keep the unconditional logic and always call JvmMetrics.register(PrometheusRegistry). So overall it would simplify our logic.

[zeitlinger]

what if we make all calls to register idempotent - to avoid surprising behavior?

Index: prometheus-metrics-instrumentation-jvm/src/main/java/io/prometheus/metrics/instrumentation/jvm/JvmMetrics.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/prometheus-metrics-instrumentation-jvm/src/main/java/io/prometheus/metrics/instrumentation/jvm/JvmMetrics.java b/prometheus-metrics-instrumentation-jvm/src/main/java/io/prometheus/metrics/instrumentation/jvm/JvmMetrics.java
--- a/prometheus-metrics-instrumentation-jvm/src/main/java/io/prometheus/metrics/instrumentation/jvm/JvmMetrics.java	(revision 3d188e06a98297f91f01ecd9e883fe2982e61891)
+++ b/prometheus-metrics-instrumentation-jvm/src/main/java/io/prometheus/metrics/instrumentation/jvm/JvmMetrics.java	(date 1727447272314)
@@ -3,7 +3,8 @@
 import io.prometheus.metrics.config.PrometheusProperties;
 import io.prometheus.metrics.model.registry.PrometheusRegistry;
 
-import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
 
 /**
  * Registers all JVM metrics. Example usage:
@@ -13,7 +14,7 @@
  */
 public class JvmMetrics {
 
-    private static AtomicBoolean registeredWithTheDefaultRegistry = new AtomicBoolean(false);
+    private static ConcurrentMap<PrometheusRegistry, PrometheusRegistry> registered = new ConcurrentHashMap<>();
 
     public static Builder builder() {
         return new Builder(PrometheusProperties.get());
@@ -41,9 +42,7 @@
          * Only the first call will register the metrics, all subsequent calls will be ignored.
          */
         public void register() {
-            if (!registeredWithTheDefaultRegistry.getAndSet(true)) {
-                register(PrometheusRegistry.defaultRegistry);
-            }
+            register(PrometheusRegistry.defaultRegistry);
         }
 
         /**
@@ -53,6 +52,10 @@
          * throw an Exception because you are trying to register duplicate metrics.
          */
         public void register(PrometheusRegistry registry) {
+            if (registered.putIfAbsent(registry, registry) != null) {
+                return;
+            }
+
             JvmThreadsMetrics.builder(config).register(registry);
             JvmBufferPoolMetrics.builder(config).register(registry);
             JvmClassLoadingMetrics.builder(config).register(registry);

[mimaison]

Thanks for the feedback!
Tracking registration regardless of the registry is fine by me. If it's what the team prefers, I'm happy to make the changes.

[zeitlinger]

Thanks!

[mimaison]

I've pushed an update so that both register() methods are idempotent.