Skip to content

Drop support for "all" from management.metrics.distribution.sla #14684

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

Closed
wants to merge 5 commits into from
Closed

Drop support for "all" from management.metrics.distribution.sla #14684

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b68ab29
arop support for "all" from management.metrics.distribution.sla
ayudovin Oct 4, 2018
659ca93
drop support for "all" from management.metrics.distribution.sla
ayudovin Oct 4, 2018
40396b8
drop support for all from configuring the SLA
ayudovin Oct 4, 2018
5ec9e20
Merge branch 'master' of https://github.com/spring-projects/spring-bo…
ayudovin Oct 4, 2018
48c99eb
updating way of dropping support for all from configuring the SLA
ayudovin Oct 4, 2018
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
7 changes: 3 additions & 4 deletions ...c/main/java/org/springframework/boot/actuate/autoconfigure/metrics/MetricsProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,10 +192,9 @@ public static class Distribution {

/**
* Specific SLA boundaries for meter IDs starting-with the specified name. The
* longest match wins, the key `all` can also be used to configure all meters.
* Counters will be published for each specified boundary. Values can be specified
* as a long or as a Duration value (for timer meters, defaulting to ms if no unit
* specified).
* longest match wins. Counters will be published for each specified boundary.
* Values can be specified as a long or as a Duration value (for timer meters,
* defaulting to ms if no unit specified).
*/
private final Map<String, ServiceLevelAgreementBoundary[]> sla = new LinkedHashMap<>();

Expand Down
22 changes: 14 additions & 8 deletions ...in/java/org/springframework/boot/actuate/autoconfigure/metrics/PropertiesMeterFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import java.util.Arrays;
import java.util.Map;
import java.util.Objects;
import java.util.function.Function;
import java.util.stream.Collectors;

import io.micrometer.core.instrument.Meter;
Expand All @@ -39,6 +40,7 @@
* @author Jon Schneider
* @author Phillip Webb
* @author Stephane Nicoll
* @author Artsiom Yudovin
* @since 2.0.0
*/
public class PropertiesMeterFilter implements MeterFilter {
Expand Down Expand Up @@ -66,7 +68,8 @@ private static MeterFilter createMapFilter(Map<String, String> tags) {

@Override
public MeterFilterReply accept(Meter.Id id) {
boolean enabled = lookup(this.properties.getEnable(), id, true);
boolean enabled = lookup(this.properties.getEnable(), id, true,
(all) -> this.properties.getEnable().getOrDefault(all, null));
Copy link
Member

@wilkinsona wilkinsona Oct 4, 2018

Choose a reason for hiding this comment

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

The defaults are out of sync here. One it true and the other is null. This means we're missing a test as, with these changes, the following test will fail with a NullPointerException:

@Test
public void acceptWhenHasNoMatchingEnabledPropertyShouldReturnNeutral() {
	PropertiesMeterFilter filter = new PropertiesMeterFilter(
			createProperties("enable.something.else=false"));
	assertThat(filter.accept(createMeterId("spring.boot")))
			.isEqualTo(MeterFilterReply.NEUTRAL);
}

return enabled ? MeterFilterReply.NEUTRAL : MeterFilterReply.DENY;
}

Expand All @@ -79,11 +82,13 @@ public Id map(Id id) {
public DistributionStatisticConfig configure(Meter.Id id,
DistributionStatisticConfig config) {
Distribution distribution = this.properties.getDistribution();
return DistributionStatisticConfig.builder()
.percentilesHistogram(
lookup(distribution.getPercentilesHistogram(), id, null))
.percentiles(lookup(distribution.getPercentiles(), id, null))
.sla(convertSla(id.getType(), lookup(distribution.getSla(), id, null)))
return DistributionStatisticConfig.builder().percentilesHistogram(lookup(
distribution.getPercentilesHistogram(), id, null,
(all) -> distribution.getPercentilesHistogram().getOrDefault(all, null)))
Copy link
Member

@wilkinsona wilkinsona Oct 4, 2018

Choose a reason for hiding this comment

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

I'm not too keen on the repetition of distribution.getPercentilesHistogram().

I think it would be better to have a lookup method and another method that's something like lookupWithFallbackToAll. Both could delegate to another method to do the actual lookup and could then either just apply the default value, or could fall back to looking for all and only then applying the default value if needed.

That would change this code to look something like this:

return DistributionStatisticConfig.builder()
		.percentilesHistogram(lookupWithFallbackToAll(
				distribution.getPercentilesHistogram(), id, null))
		.percentiles(
				lookupWithFallbackToAll(distribution.getPercentiles(), id, null))
		.sla(convertSla(id.getType(), lookup(distribution.getSla(), id, null)))
		.build().merge(config);

.percentiles(lookup(distribution.getPercentiles(), id, null,
(all) -> distribution.getPercentiles().getOrDefault(all, null)))
.sla(convertSla(id.getType(),
lookup(distribution.getSla(), id, null, (all) -> null)))
.build().merge(config);
}

Expand All @@ -97,7 +102,8 @@ private long[] convertSla(Meter.Type meterType, ServiceLevelAgreementBoundary[]
return (converted.length != 0) ? converted : null;
}

private <T> T lookup(Map<String, T> values, Id id, T defaultValue) {
private <T> T lookup(Map<String, T> values, Id id, T defaultValue,
Copy link
Member

@wilkinsona wilkinsona Oct 4, 2018

Choose a reason for hiding this comment

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

This doesn't look quite right as defaultValue is never used.

Function<String, T> all) {
if (values.isEmpty()) {
return defaultValue;
}
Expand All @@ -110,7 +116,7 @@ private <T> T lookup(Map<String, T> values, Id id, T defaultValue) {
int lastDot = name.lastIndexOf('.');
name = (lastDot != -1) ? name.substring(0, lastDot) : "";
}
return values.getOrDefault("all", defaultValue);
return all.apply("all");
}

}
40 changes: 1 addition & 39 deletions ...va/org/springframework/boot/actuate/autoconfigure/metrics/PropertiesMeterFilterTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
*
* @author Phillip Webb
* @author Jon Schneider
* @author Artsiom Yudovin
*/
public class PropertiesMeterFilterTests {

Expand Down Expand Up @@ -253,45 +254,6 @@ public void configureWhenHasHigherSlaAndLowerShouldSetSlaToHigher() {
.containsExactly(4000000, 5000000, 6000000);
}

@Test
public void configureWhenAllSlaSetShouldSetSlaToValue() {
PropertiesMeterFilter filter = new PropertiesMeterFilter(
createProperties("distribution.sla.all=1,2,3"));
assertThat(filter.configure(createMeterId("spring.boot"),
DistributionStatisticConfig.DEFAULT).getSlaBoundaries())
.containsExactly(1000000, 2000000, 3000000);
}

@Test
public void configureWhenSlaDurationShouldOnlyApplyToTimer() {
PropertiesMeterFilter filter = new PropertiesMeterFilter(
createProperties("distribution.sla.all=1ms,2ms,3ms"));
Meter.Id timer = createMeterId("spring.boot", Meter.Type.TIMER);
Meter.Id summary = createMeterId("spring.boot", Meter.Type.DISTRIBUTION_SUMMARY);
Meter.Id counter = createMeterId("spring.boot", Meter.Type.COUNTER);
assertThat(filter.configure(timer, DistributionStatisticConfig.DEFAULT)
.getSlaBoundaries()).containsExactly(1000000, 2000000, 3000000);
assertThat(filter.configure(summary, DistributionStatisticConfig.DEFAULT)
.getSlaBoundaries()).isNullOrEmpty();
assertThat(filter.configure(counter, DistributionStatisticConfig.DEFAULT)
.getSlaBoundaries()).isNullOrEmpty();
}

@Test
public void configureWhenSlaLongShouldOnlyApplyToTimerAndDistributionSummary() {
PropertiesMeterFilter filter = new PropertiesMeterFilter(
createProperties("distribution.sla.all=1,2,3"));
Meter.Id timer = createMeterId("spring.boot", Meter.Type.TIMER);
Meter.Id summary = createMeterId("spring.boot", Meter.Type.DISTRIBUTION_SUMMARY);
Meter.Id counter = createMeterId("spring.boot", Meter.Type.COUNTER);
assertThat(filter.configure(timer, DistributionStatisticConfig.DEFAULT)
.getSlaBoundaries()).containsExactly(1000000, 2000000, 3000000);
assertThat(filter.configure(summary, DistributionStatisticConfig.DEFAULT)
.getSlaBoundaries()).containsExactly(1, 2, 3);
assertThat(filter.configure(counter, DistributionStatisticConfig.DEFAULT)
.getSlaBoundaries()).isNullOrEmpty();
}

private Id createMeterId(String name) {
Meter.Type meterType = Type.TIMER;
return createMeterId(name, meterType);
Expand Down
2 changes: 1 addition & 1 deletion ...project/spring-boot-docs/src/main/asciidoc/appendix-application-properties.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1381,7 +1381,7 @@ content into your application. Rather, pick only the properties that you need.
# METRICS
management.metrics.distribution.percentiles-histogram.*= # Whether meter IDs starting with the specified name should publish percentile histograms.
management.metrics.distribution.percentiles.*= # Specific computed non-aggregable percentiles to ship to the backend for meter IDs starting-with the specified name.
management.metrics.distribution.sla.*= # Specific SLA boundaries for meter IDs starting-with the specified name. The longest match wins, the key `all` can also be used to configure all meters.
management.metrics.distribution.sla.*= # Specific SLA boundaries for meter IDs starting-with the specified name. The longest match wins.
management.metrics.enable.*= # Whether meter IDs starting-with the specified name should be enabled. The longest match wins, the key `all` can also be used to configure all meters.
management.metrics.export.atlas.batch-size=10000 # Number of measurements per request to use for this backend. If more measurements are found, then multiple requests will be made.
management.metrics.export.atlas.config-refresh-frequency=10s # Frequency for refreshing config settings from the LWC service.
Expand Down