Skip to content

Commit 950480d

Browse files
pulkitmehraphilwebb
authored andcommitted
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
1 parent 2a2908e commit 950480d

File tree

2 files changed

+66
-10
lines changed

2 files changed

+66
-10
lines changed

spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/MetricsEndpoint.java

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616

1717
package org.springframework.boot.actuate.metrics;
1818

19-
import java.util.ArrayList;
19+
import java.util.Collection;
2020
import java.util.Collections;
2121
import java.util.HashMap;
2222
import java.util.HashSet;
@@ -81,15 +81,15 @@ private String getName(Meter meter) {
8181
public MetricResponse metric(@Selector String requiredMetricName,
8282
@Nullable List<String> tag) {
8383
List<Tag> tags = parseTags(tag);
84-
List<Meter> meters = new ArrayList<>();
85-
collectMeters(meters, this.registry, requiredMetricName, tags);
84+
Collection<Meter> meters = findFirstMatchingMeters(this.registry,
85+
requiredMetricName, tags);
8686
if (meters.isEmpty()) {
8787
return null;
8888
}
8989
Map<Statistic, Double> samples = getSamples(meters);
9090
Map<String, Set<String>> availableTags = getAvailableTags(meters);
9191
tags.forEach((t) -> availableTags.remove(t.getKey()));
92-
Meter.Id meterId = meters.get(0).getId();
92+
Meter.Id meterId = meters.iterator().next().getId();
9393
return new MetricResponse(requiredMetricName, meterId.getDescription(),
9494
meterId.getBaseUnit(), asList(samples, Sample::new),
9595
asList(availableTags, AvailableTag::new));
@@ -112,18 +112,25 @@ private Tag parseTag(String tag) {
112112
return Tag.of(parts[0], parts[1]);
113113
}
114114

115-
private void collectMeters(List<Meter> meters, MeterRegistry registry, String name,
115+
private Collection<Meter> findFirstMatchingMeters(MeterRegistry registry, String name,
116116
Iterable<Tag> tags) {
117117
if (registry instanceof CompositeMeterRegistry) {
118-
((CompositeMeterRegistry) registry).getRegistries()
119-
.forEach((member) -> collectMeters(meters, member, name, tags));
118+
return ((CompositeMeterRegistry) registry).getRegistries().stream()
119+
.map((r) -> findFirstMatchingMeters(r, name, tags))
120+
.filter((match) -> !match.isEmpty()).findFirst()
121+
.orElse(Collections.emptyList());
122+
120123
}
121124
else {
122-
meters.addAll(registry.find(name).tags(tags).meters());
125+
Collection<Meter> metersFound = registry.find(name).tags(tags).meters();
126+
if (!metersFound.isEmpty()) {
127+
return metersFound;
128+
}
123129
}
130+
return Collections.emptyList();
124131
}
125132

126-
private Map<Statistic, Double> getSamples(List<Meter> meters) {
133+
private Map<Statistic, Double> getSamples(Collection<Meter> meters) {
127134
Map<Statistic, Double> samples = new LinkedHashMap<>();
128135
meters.forEach((meter) -> mergeMeasurements(samples, meter));
129136
return samples;
@@ -138,7 +145,7 @@ private BiFunction<Double, Double, Double> mergeFunction(Statistic statistic) {
138145
return Statistic.MAX.equals(statistic) ? Double::max : Double::sum;
139146
}
140147

141-
private Map<String, Set<String>> getAvailableTags(List<Meter> meters) {
148+
private Map<String, Set<String>> getAvailableTags(Collection<Meter> meters) {
142149
Map<String, Set<String>> availableTags = new HashMap<>();
143150
meters.forEach((meter) -> mergeAvailableTags(availableTags, meter));
144151
return availableTags;

spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/MetricsEndpointTests.java

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,55 @@ public void metricValuesAreTheSumOfAllTimeSeriesMatchingTags() {
9595
assertThat(getCount(response)).hasValue(4.0);
9696
}
9797

98+
@Test
99+
public void findFirstMatchingMetersFromNestedRegistries() {
100+
CompositeMeterRegistry composite = new CompositeMeterRegistry();
101+
SimpleMeterRegistry reg1 = new SimpleMeterRegistry();
102+
CompositeMeterRegistry reg2 = new CompositeMeterRegistry();
103+
SimpleMeterRegistry reg3 = new SimpleMeterRegistry();
104+
105+
// 1st level nesting
106+
composite.add(reg1);
107+
108+
// 2st level nesting
109+
reg2.add(reg3);
110+
composite.add(reg2);
111+
112+
// 2nd level registry has metrics
113+
reg3.counter("cache", "result", "hit", "host", "1").increment(2);
114+
reg3.counter("cache", "result", "miss", "host", "1").increment(2);
115+
reg3.counter("cache", "result", "hit", "host", "2").increment(2);
116+
117+
MetricsEndpoint endpoint = new MetricsEndpoint(composite);
118+
119+
MetricsEndpoint.MetricResponse response = endpoint.metric("cache",
120+
Collections.emptyList());
121+
assertThat(response.getName()).isEqualTo("cache");
122+
assertThat(availableTagKeys(response)).containsExactly("result", "host");
123+
assertThat(getCount(response)).hasValue(6.0);
124+
125+
response = endpoint.metric("cache", Collections.singletonList("result:hit"));
126+
assertThat(availableTagKeys(response)).containsExactly("host");
127+
assertThat(getCount(response)).hasValue(4.0);
128+
}
129+
130+
@Test
131+
public void matchingMeterNotFoundInNestedRegistries() {
132+
CompositeMeterRegistry composite = new CompositeMeterRegistry();
133+
CompositeMeterRegistry reg2 = new CompositeMeterRegistry();
134+
SimpleMeterRegistry reg3 = new SimpleMeterRegistry();
135+
136+
// nested registries
137+
reg2.add(reg3);
138+
composite.add(reg2);
139+
140+
MetricsEndpoint endpoint = new MetricsEndpoint(composite);
141+
142+
MetricsEndpoint.MetricResponse response = endpoint.metric("invalid.metric.name",
143+
Collections.emptyList());
144+
assertThat(response).isNull();
145+
}
146+
98147
@Test
99148
public void metricTagValuesAreDeduplicated() {
100149
this.registry.counter("cache", "host", "1", "region", "east", "result", "hit");

0 commit comments

Comments
 (0)