Skip to content

Commit

Permalink
Ignore noop children when polling composite meters
Browse files Browse the repository at this point in the history
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 micrometer-metricsgh-1441
  • Loading branch information
jonatan-ivanov authored and JannickKemming1997 committed Jan 16, 2024
1 parent 74b5cfc commit e795826
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 5 deletions.
Expand Up @@ -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;

Expand All @@ -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;
Expand Down
Expand Up @@ -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;
Expand Down Expand Up @@ -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
}

}

0 comments on commit e795826

Please sign in to comment.