Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ignore noop children when polling composite meters #4093

Open
wants to merge 1 commit into
base: 1.9.x
Choose a base branch
from
Open

Ignore noop children when polling composite meters #4093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions ...er-core/src/main/java/io/micrometer/core/instrument/composite/AbstractCompositeMeter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@
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()) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be on the "hot path" (i.e.: recording a measurement or retrieving meters) but it will be called every time a value is fetched for publishing.

I'm thinking if there might be another way to implement this: modifying the children collection so that it has some priority ordering so the non-noop meters in it take precedence and will be "in front" of the collection (if any). That solution though might be more problematic since it seems a bit more complicated and it can be on a "hotter" path (meter creation).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you seen this PR? The approach is completely different but certainly more efficient. We should understand if there is any hidden side effect if the meter is not added..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess #1442 is also connected.
I would not do what is in #1443, it is somewhat similar to the priority ordering solution I explained before so its path is "hotter" and it omits meters from the registry.

if (!(next instanceof NoopMeter)) {
return next;
}
}

// There are no child meters. Return a lazily instantiated no-op meter.
final T noopMeter = this.noopMeter;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are at this point, the children collection was either empty or contains only noop meters. In the second case we could return it the first value (like we did before this change) but I'm not sure it worth the hassle.

Expand Down
32 changes: 32 additions & 0 deletions ...ore/src/test/java/io/micrometer/core/instrument/composite/CompositeMeterRegistryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
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
}

}