From 950480dc1c500507cc0551c353a3b5e4198b3f25 Mon Sep 17 00:00:00 2001 From: pmehra Date: Mon, 17 Sep 2018 16:36:40 -0400 Subject: [PATCH] Stop MetricsEndpoint from summing up same metrics Update `MetricsEndpoint` so that only the first matching meter is used when calculating the sum of of statistics. Prior this this commit the endpoint would consider all Meters. This caused incorrect statistics when multiple back-end systems were being used since the registries contained in the `CompositeMeterRegistry` would be iterated, and the same effective metric would be counted more than once. Closes gh-14497 --- .../boot/actuate/metrics/MetricsEndpoint.java | 27 ++++++---- .../actuate/metrics/MetricsEndpointTests.java | 49 +++++++++++++++++++ 2 files changed, 66 insertions(+), 10 deletions(-) diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/MetricsEndpoint.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/MetricsEndpoint.java index abb0109a66..84738ac5a5 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/MetricsEndpoint.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/MetricsEndpoint.java @@ -16,7 +16,7 @@ package org.springframework.boot.actuate.metrics; -import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; @@ -81,15 +81,15 @@ public class MetricsEndpoint { public MetricResponse metric(@Selector String requiredMetricName, @Nullable List tag) { List tags = parseTags(tag); - List meters = new ArrayList<>(); - collectMeters(meters, this.registry, requiredMetricName, tags); + Collection meters = findFirstMatchingMeters(this.registry, + requiredMetricName, tags); if (meters.isEmpty()) { return null; } Map samples = getSamples(meters); Map> availableTags = getAvailableTags(meters); tags.forEach((t) -> availableTags.remove(t.getKey())); - Meter.Id meterId = meters.get(0).getId(); + Meter.Id meterId = meters.iterator().next().getId(); return new MetricResponse(requiredMetricName, meterId.getDescription(), meterId.getBaseUnit(), asList(samples, Sample::new), asList(availableTags, AvailableTag::new)); @@ -112,18 +112,25 @@ public class MetricsEndpoint { return Tag.of(parts[0], parts[1]); } - private void collectMeters(List meters, MeterRegistry registry, String name, + private Collection findFirstMatchingMeters(MeterRegistry registry, String name, Iterable tags) { if (registry instanceof CompositeMeterRegistry) { - ((CompositeMeterRegistry) registry).getRegistries() - .forEach((member) -> collectMeters(meters, member, name, tags)); + return ((CompositeMeterRegistry) registry).getRegistries().stream() + .map((r) -> findFirstMatchingMeters(r, name, tags)) + .filter((match) -> !match.isEmpty()).findFirst() + .orElse(Collections.emptyList()); + } else { - meters.addAll(registry.find(name).tags(tags).meters()); + Collection metersFound = registry.find(name).tags(tags).meters(); + if (!metersFound.isEmpty()) { + return metersFound; + } } + return Collections.emptyList(); } - private Map getSamples(List meters) { + private Map getSamples(Collection meters) { Map samples = new LinkedHashMap<>(); meters.forEach((meter) -> mergeMeasurements(samples, meter)); return samples; @@ -138,7 +145,7 @@ public class MetricsEndpoint { return Statistic.MAX.equals(statistic) ? Double::max : Double::sum; } - private Map> getAvailableTags(List meters) { + private Map> getAvailableTags(Collection meters) { Map> availableTags = new HashMap<>(); meters.forEach((meter) -> mergeAvailableTags(availableTags, meter)); return availableTags; diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/MetricsEndpointTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/MetricsEndpointTests.java index 3fe4279a38..3ad8e82c23 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/MetricsEndpointTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/MetricsEndpointTests.java @@ -95,6 +95,55 @@ public class MetricsEndpointTests { assertThat(getCount(response)).hasValue(4.0); } + @Test + public void findFirstMatchingMetersFromNestedRegistries() { + CompositeMeterRegistry composite = new CompositeMeterRegistry(); + SimpleMeterRegistry reg1 = new SimpleMeterRegistry(); + CompositeMeterRegistry reg2 = new CompositeMeterRegistry(); + SimpleMeterRegistry reg3 = new SimpleMeterRegistry(); + + // 1st level nesting + composite.add(reg1); + + // 2st level nesting + reg2.add(reg3); + composite.add(reg2); + + // 2nd level registry has metrics + reg3.counter("cache", "result", "hit", "host", "1").increment(2); + reg3.counter("cache", "result", "miss", "host", "1").increment(2); + reg3.counter("cache", "result", "hit", "host", "2").increment(2); + + MetricsEndpoint endpoint = new MetricsEndpoint(composite); + + MetricsEndpoint.MetricResponse response = endpoint.metric("cache", + Collections.emptyList()); + assertThat(response.getName()).isEqualTo("cache"); + assertThat(availableTagKeys(response)).containsExactly("result", "host"); + assertThat(getCount(response)).hasValue(6.0); + + response = endpoint.metric("cache", Collections.singletonList("result:hit")); + assertThat(availableTagKeys(response)).containsExactly("host"); + assertThat(getCount(response)).hasValue(4.0); + } + + @Test + public void matchingMeterNotFoundInNestedRegistries() { + CompositeMeterRegistry composite = new CompositeMeterRegistry(); + CompositeMeterRegistry reg2 = new CompositeMeterRegistry(); + SimpleMeterRegistry reg3 = new SimpleMeterRegistry(); + + // nested registries + reg2.add(reg3); + composite.add(reg2); + + MetricsEndpoint endpoint = new MetricsEndpoint(composite); + + MetricsEndpoint.MetricResponse response = endpoint.metric("invalid.metric.name", + Collections.emptyList()); + assertThat(response).isNull(); + } + @Test public void metricTagValuesAreDeduplicated() { this.registry.counter("cache", "host", "1", "region", "east", "result", "hit");