Properly handle disabled metrics in MP (helidon-io#8908)

* Fix MP handling of disabled metrics

Signed-off-by: Tim Quinn <tim.quinn@oracle.com>

* A little refactoring to reduce the need to duplicate some code for each metric type; add test

---------

Signed-off-by: Tim Quinn <tim.quinn@oracle.com>

microprofile/metrics/pom.xml

@@ -123,6 +123,7 @@
                                 <exclude>**/HelloWorldAsyncResponseTest.java</exclude>
                                 <exclude>**/TestMetricsOnOwnSocket.java</exclude>
                                 <exclude>**/TestDisabledMetrics.java</exclude>
+                                <exclude>**/TestSelectivelyDisabledMetrics.java</exclude>
                                 <exclude>**/TestConfigProcessing.java</exclude>
                             </excludes>
                             <systemPropertyVariables>
@@ -162,6 +163,17 @@
                             </excludes>
                         </configuration>
                     </execution>
+                    <execution>
+                        <id>test-selectively-disabled-metrics</id>
+                        <goals>
+                            <goal>test</goal>
+                        </goals>
+                        <configuration>
+                            <includes>
+                                <include>**/TestSelectivelyDisabledMetric.java</include>
+                            </includes>
+                        </configuration>
+                    </execution>
                     <execution>
                         <id>test-config-with-top-level-tags</id>
                         <goals>

microprofile/metrics/src/main/java/io/helidon/microprofile/metrics/HelidonCounter.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018, 2023 Oracle and/or its affiliates.
+ * Copyright (c) 2018, 2024 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -54,6 +54,12 @@ static HelidonCounter create(String scope,
                                   delegate);
     }
 
+    static HelidonCounter create(io.helidon.metrics.api.Counter delegate) {
+        return new HelidonCounter(resolvedScope(delegate),
+                                  Registry.metadata(delegate),
+                                  delegate);
+    }
+
     @Override
     public void inc() {
         delegate.increment();

microprofile/metrics/src/main/java/io/helidon/microprofile/metrics/HelidonGauge.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018, 2023 Oracle and/or its affiliates.
+ * Copyright (c) 2018, 2024 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -85,6 +85,12 @@ static <N extends Number> HelidonGauge<N> create(String scope,
                                                 delegate);
     }
 
+    static <N extends Number> HelidonGauge<N> create(io.helidon.metrics.api.Gauge<N> delegate) {
+        return new HelidonGauge.DelegateBased<>(resolvedScope(delegate),
+                                                Registry.metadata(delegate),
+                                                delegate);
+    }
+
     static HelidonGauge<Long> create(String scope,
                                      Metadata metadata,
                                      io.helidon.metrics.api.FunctionalCounter delegate) {

microprofile/metrics/src/main/java/io/helidon/microprofile/metrics/HelidonHistogram.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018, 2023 Oracle and/or its affiliates.
+ * Copyright (c) 2018, 2024 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -57,6 +57,12 @@ static HelidonHistogram create(String scope,
                                     delegate);
     }
 
+    static HelidonHistogram create(io.helidon.metrics.api.DistributionSummary delegate) {
+        return new HelidonHistogram(resolvedScope(delegate),
+                                    Registry.metadata(delegate),
+                                    delegate);
+    }
+
     @Override
     public long getSum() {
         return (long) delegate.totalAmount();

microprofile/metrics/src/main/java/io/helidon/microprofile/metrics/HelidonTimer.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018, 2023 Oracle and/or its affiliates.
+ * Copyright (c) 2018, 2024 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -67,6 +67,13 @@ static HelidonTimer create(MeterRegistry meterRegistry,
                                 delegate);
     }
 
+    static HelidonTimer create(MeterRegistry meterRegistry, io.helidon.metrics.api.Timer delegate) {
+        return new HelidonTimer(meterRegistry,
+                                resolvedScope(delegate),
+                                Registry.metadata(delegate),
+                                delegate);
+    }
+
     @Override
     public void update(Duration duration) {
         delegate.record(duration);

microprofile/metrics/src/main/java/io/helidon/microprofile/metrics/MetricImpl.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018, 2023 Oracle and/or its affiliates.
+ * Copyright (c) 2018, 2024 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -81,6 +81,10 @@ protected static Iterable<io.helidon.metrics.api.Tag> allTags(String scope, Tag[
         return toHelidonTags(SystemTagsManager.instance().withScopeTag(iterableEntries(tags), scope));
     }
 
+    static String resolvedScope(Meter delegate) {
+        return SystemTagsManager.instance().effectiveScope(delegate.scope()).orElse(Meter.Scope.DEFAULT);
+    }
+
     /**
      * Converts an iterable of map entries (representing tag names and values) into an iterable of Helidon tags.
      *

microprofile/metrics/src/main/java/io/helidon/microprofile/metrics/Registry.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2023 Oracle and/or its affiliates.
+ * Copyright (c) 2023, 2024 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -80,6 +80,15 @@ static Registry create(String scope, MeterRegistry meterRegistry) {
         return new Registry(scope, meterRegistry);
     }
 
+    static Metadata metadata(Meter meter) {
+
+        MetadataBuilder builder = Metadata.builder().withName(meter.id().name());
+        meter.baseUnit().ifPresent(builder::withUnit);
+        meter.description().ifPresent(builder::withDescription);
+
+        return builder.build();
+    }
+
     protected static String sanitizeUnit(String unit) {
         return unit != null && !unit.equals(MetricUnits.NONE)
                 ? unit
@@ -431,14 +440,16 @@ HelidonMetric<?> onMeterAdded(Meter meter) {
 
             HelidonMetric<?> newMetric = metric(collector, existingInfo == null ? newMetadata : existingInfo.metadata, meter);
 
-            // Now update the data structures.
-            InfoPerName info = infoByName.computeIfAbsent(newMetricID.getName(),
-                                                          n -> InfoPerName.create(newMetadata, newMetricID));
+            // Now update the data structures if the meter is enabled.
+            if (meterRegistry.isMeterEnabled(meter.id().name(), meter.id().tagsMap(), meter.scope())) {
+                InfoPerName info = infoByName.computeIfAbsent(newMetricID.getName(),
+                                                              n -> InfoPerName.create(newMetadata, newMetricID));
 
-            // Inside info, metric IDs are stored in a set, so adding the first ID again does no harm.
-            info.add(newMetricID);
-            metricsById.put(newMetricID, newMetric);
-            metricsByDelegate.put(meter, newMetric);
+                // Inside info, metric IDs are stored in a set, so adding the first ID again does no harm.
+                info.add(newMetricID);
+                metricsById.put(newMetricID, newMetric);
+                metricsByDelegate.put(meter, newMetric);
+            }
 
             collector.collect().log(LOGGER);
 
@@ -573,15 +584,6 @@ private static void validateMetric(Errors.Collector collector,
         }
     }
 
-    private static Metadata metadata(Meter meter) {
-
-        MetadataBuilder builder = Metadata.builder().withName(meter.id().name());
-        meter.baseUnit().ifPresent(builder::withUnit);
-        meter.description().ifPresent(builder::withDescription);
-
-        return builder.build();
-    }
-
     private static Map<String, String> tagsWithoutSystemOrScopeTags(Iterable<io.helidon.metrics.api.Tag> tags) {
         Map<String, String> result = new TreeMap<>();
 
@@ -590,6 +592,10 @@ private static Map<String, String> tagsWithoutSystemOrScopeTags(Iterable<io.heli
         return result;
     }
 
+    private boolean isMeterEnabled(Meter meter) {
+        return meterRegistry.isMeterEnabled(meter.id().name(), meter.id().tagsMap(), meter.scope());
+    }
+
     private static Tag[] tags(Map<String, String> tags) {
         var result = new ArrayList<Tag>();
         tags.forEach((key, value) -> result.add(new Tag(key, value)));
@@ -605,9 +611,7 @@ private HelidonCounter createCounter(Metadata metadata, Tag... tags) {
     }
 
     private HelidonCounter createCounter(io.helidon.metrics.api.Counter.Builder counterBuilder) {
-        io.helidon.metrics.api.Counter delegate = meterRegistry.getOrCreate(counterBuilder);
-        HelidonCounter result = (HelidonCounter) metricsByDelegate.get(delegate);
-        return result;
+        return createMeter(counterBuilder, HelidonCounter::create);
     }
 
     @SuppressWarnings("unchecked")
@@ -634,17 +638,15 @@ private <N extends Number> HelidonGauge<N> createGauge(Metadata metadata, Suppli
 
     @SuppressWarnings("unchecked")
     private <N extends Number> HelidonGauge<N> createGauge(io.helidon.metrics.api.Gauge.Builder<N> gBuilder) {
-        io.helidon.metrics.api.Gauge<?> delegate = meterRegistry.getOrCreate(gBuilder);
-        return (HelidonGauge<N>) metricsByDelegate.get(delegate);
+        return createMeter(gBuilder, HelidonGauge::create);
     }
 
     @SuppressWarnings("unchecked")
     private <T> HelidonGauge<Long> createFunctionalCounter(io.helidon.metrics.api.FunctionalCounter.Builder<T> fcBuilder) {
-        io.helidon.metrics.api.Gauge<?> delegate = meterRegistry
-                .getOrCreate(io.helidon.metrics.api.Gauge.builder(fcBuilder.name(),
-                                                                  () -> fcBuilder.fn()
-                                                                          .apply(fcBuilder.stateObject())));
-        return (HelidonGauge<Long>) metricsByDelegate.get(delegate);
+        return createMeter(io.helidon.metrics.api.Gauge.builder(fcBuilder.name(),
+                                                                () -> fcBuilder.fn()
+                                                                        .apply(fcBuilder.stateObject())),
+                           HelidonGauge::create);
     }
 
     private HelidonHistogram createHistogram(Metadata metadata, Tag... tags) {
@@ -657,8 +659,7 @@ private HelidonHistogram createHistogram(Metadata metadata, Tag... tags) {
     }
 
     private HelidonHistogram createHistogram(io.helidon.metrics.api.DistributionSummary.Builder sBuilder) {
-        io.helidon.metrics.api.DistributionSummary delegate = meterRegistry.getOrCreate(sBuilder);
-        return (HelidonHistogram) metricsByDelegate.get(delegate);
+        return createMeter(sBuilder, HelidonHistogram::create);
     }
 
     private HelidonTimer createTimer(Metadata metadata, Tag... tags) {
@@ -670,8 +671,19 @@ private HelidonTimer createTimer(Metadata metadata, Tag... tags) {
     }
 
     private HelidonTimer createTimer(io.helidon.metrics.api.Timer.Builder tBuilder) {
-        io.helidon.metrics.api.Timer delegate = meterRegistry.getOrCreate(tBuilder);
-        return (HelidonTimer) metricsByDelegate.get(delegate);
+        return createMeter(tBuilder, d -> HelidonTimer.create(meterRegistry, d));
+    }
+
+    private <HM extends HelidonMetric<M>,
+            M extends Meter,
+            B extends Meter.Builder<B, M>> HM createMeter(B builder,
+                                                          Function<M, HM> factory) {
+        M delegate = meterRegistry.getOrCreate(builder);
+        // Disabled metrics are not in the data structures supporting our registry, so we cannot find those via metricsByDelegate.
+        // Instead just create a new wrapper around the delegate.
+        return isMeterEnabled(delegate)
+                ? (HM) metricsByDelegate.get(delegate)
+                : factory.apply(delegate);
     }
 
     private boolean removeMatchingWithResult(MetricFilter filter) {

microprofile/metrics/src/test/java/io/helidon/microprofile/metrics/TestSelectivelyDisabledMetric.java

@@ -0,0 +1,52 @@
+/*
+ * Copyright (c) 2024 Oracle and/or its affiliates.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.helidon.microprofile.metrics;
+
+import io.helidon.metrics.api.Meter;
+import io.helidon.microprofile.testing.junit5.AddBean;
+import io.helidon.microprofile.testing.junit5.AddConfigBlock;
+import io.helidon.microprofile.testing.junit5.HelidonTest;
+
+import org.eclipse.microprofile.metrics.Counter;
+import org.eclipse.microprofile.metrics.MetricID;
+import org.junit.jupiter.api.Test;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.nullValue;
+
+@HelidonTest
+@AddBean(CountedBean.class)
+@AddConfigBlock("""
+        metrics.scoping.scopes.0.name=application
+        metrics.scoping.scopes.0.filter.exclude=.*oome.*"""
+)
+class TestSelectivelyDisabledMetric {
+
+    @Test
+    void testDisabledCounter() {
+        RegistryFactory rf = RegistryFactory.getInstance();
+        Registry reg = rf.registry(Meter.Scope.APPLICATION);
+
+        MetricID metricID = new MetricID(CountedBean.DOOMED_COUNTER);
+        Counter counter = reg.getCounter(metricID);
+        assertThat("Disabled counter looked up", counter, is(nullValue()));
+
+        counter = reg.counter(metricID);
+        counter.inc();
+        assertThat("Disabled (no-op) counter after increment", counter.getCount(), is(0L));
+    }
+}