From e795826041e3a756a7a937ebcba2c6cfb3371437 Mon Sep 17 00:00:00 2001
From: Jonatan Ivanov <jonatan.ivanov@gmail.com>
Date: Thu, 21 Sep 2023 17:45:11 -0700
Subject: [PATCH] Ignore noop children when polling composite meters

When a CompositeMeterRegistry is used and one of its CompositeMeters
is polled (its value is fetched), the returned value can depend on
the order of the registries inside of the composite if the composite
contains a registry that has any NoopMeters.

Example: a CompositeMeterRegistry contains two registries, A and B.
We create a counter in the composite and increment it once.
After this both A and B contain one counter but lets say that
in A the counter is ignored so it will be noop.
When the count called on CompositeCounter, it can return
either 0 (if NoopCounter was used)
or 1 (if the non-noop counter was used).

In order to fix this, we can ignore the NoopMeters
when Meters are polled in a composite registry.

Closes gh-1441
---
 .../composite/AbstractCompositeMeter.java     | 12 ++++---
 .../composite/CompositeMeterRegistryTest.java | 32 +++++++++++++++++++
 2 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/micrometer-core/src/main/java/io/micrometer/core/instrument/composite/AbstractCompositeMeter.java b/micrometer-core/src/main/java/io/micrometer/core/instrument/composite/AbstractCompositeMeter.java
index 92ebc857cd..e9d5c58cbb 100644
--- a/micrometer-core/src/main/java/io/micrometer/core/instrument/composite/AbstractCompositeMeter.java
+++ b/micrometer-core/src/main/java/io/micrometer/core/instrument/composite/AbstractCompositeMeter.java
@@ -15,14 +15,14 @@
  */
 package io.micrometer.core.instrument.composite;
 
-import io.micrometer.common.lang.Nullable;
 import io.micrometer.core.instrument.AbstractMeter;
 import io.micrometer.core.instrument.Meter;
 import io.micrometer.core.instrument.MeterRegistry;
+import io.micrometer.core.instrument.noop.NoopMeter;
+import io.micrometer.core.lang.Nullable;
 
 import java.util.Collections;
 import java.util.IdentityHashMap;
-import java.util.Iterator;
 import java.util.Map;
 import java.util.concurrent.atomic.AtomicBoolean;
 
@@ -49,9 +49,11 @@ final Iterable<T> getChildren() {
     }
 
     T firstChild() {
-        final Iterator<T> i = children.values().iterator();
-        if (i.hasNext())
-            return i.next();
+        for (T next : children.values()) {
+            if (!(next instanceof NoopMeter)) {
+                return next;
+            }
+        }
 
         // There are no child meters. Return a lazily instantiated no-op meter.
         final T noopMeter = this.noopMeter;
diff --git a/micrometer-core/src/test/java/io/micrometer/core/instrument/composite/CompositeMeterRegistryTest.java b/micrometer-core/src/test/java/io/micrometer/core/instrument/composite/CompositeMeterRegistryTest.java
index 4d95274858..4a07b63dc8 100644
--- a/micrometer-core/src/test/java/io/micrometer/core/instrument/composite/CompositeMeterRegistryTest.java
+++ b/micrometer-core/src/test/java/io/micrometer/core/instrument/composite/CompositeMeterRegistryTest.java
@@ -28,18 +28,23 @@
 import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.RepeatedTest;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
 import reactor.core.publisher.Flux;
 import reactor.core.scheduler.Schedulers;
 
 import java.time.Duration;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Stream;
 
+import static java.util.Arrays.asList;
 import static java.util.Collections.emptyList;
 import static java.util.Collections.singletonList;
 import static org.assertj.core.api.Assertions.assertThat;
@@ -493,4 +498,31 @@ void meterRemovalPropagatesToChildRegistryWithModifyingFilter() {
         assertThat(this.simple.getMeters()).isEmpty();
     }
 
+    @Issue("#1441")
+    @ParameterizedTest
+    @MethodSource("registriesProvider")
+    void whenMetersArePolledNoopChildrenShouldBeIgnored(List<MeterRegistry> registries) {
+        // this means that firstChild() practically should be firstNonNoopChild()
+        CompositeMeterRegistry composite = new CompositeMeterRegistry(Clock.SYSTEM, registries);
+        Counter counter = composite.counter("my.counter");
+        counter.increment();
+        assertThat(counter.count()).isEqualTo(1);
+    }
+
+    static Stream<List<MeterRegistry>> registriesProvider() {
+        // Since the order is non-deterministic, the best effort is testing both orders
+        SimpleMeterRegistry denyAllRegistry1 = new SimpleMeterRegistry();
+        denyAllRegistry1.config().meterFilter(MeterFilter.deny());
+
+        SimpleMeterRegistry denyAllRegistry2 = new SimpleMeterRegistry();
+        denyAllRegistry2.config().meterFilter(MeterFilter.deny());
+
+        // @formatter:off
+        return Stream.of(
+                asList(denyAllRegistry1, new SimpleMeterRegistry()), // denyAll,  allowAll
+                asList(new SimpleMeterRegistry(), denyAllRegistry2)  // allowAll, denyAll
+        );
+        // @formatter:on
+    }
+
 }