Skip to content

Support more Micrometer histogram properties #14139

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 1 commit into from
Closed

Support more Micrometer histogram properties #14139

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
25 changes: 25 additions & 0 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 @@ -25,6 +25,7 @@
* {@link ConfigurationProperties} for configuring Micrometer-based metrics.
*
* @author Jon Schneider
* @author Alexander Abramov
* @since 2.0.0
*/
@ConfigurationProperties("management.metrics")
Expand Down Expand Up @@ -199,6 +200,22 @@ public static class Distribution {
*/
private final Map<String, ServiceLevelAgreementBoundary[]> sla = new LinkedHashMap<>();

/**
* The minimum value that this distribution summary is expected to observe.
* Controls the number of buckets shipped by percentilesHistogram. 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> minimumExpectedValue = new LinkedHashMap<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

While ServiceLevelAgreementBoundary is a convenient type to deserialize this value, minimumExpectedValue is unconnected from SLAs conceptually.

Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT @snicoll?

Copy link
Contributor Author

@alexanderabramov alexanderabramov Aug 20, 2018 •
edited by philwebb
Loading

Choose a reason for hiding this comment

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

Agree. Which alternative do you recommend:

  • copy ServiceLevelAgreementBoundary to a better named class with exactly the same code
  • rename ServiceLevelAgreementBoundary
  • other?

Copy link
Contributor

Choose a reason for hiding this comment

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

ping @snicoll

Copy link
Member

Choose a reason for hiding this comment

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

I frankly have no opinion as I am not sure I've grasped those concepts fully just yet. I've flagged for team attention to get this one more visibility. We have a call today so we should provide a smarter feedback soon.

Thanks for your patience.

Copy link
Member

Choose a reason for hiding this comment

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

It's unfortunate that the ideal type is either a long or a Duration. Separate properties for distribution summaries (long) and timers (Duration) would offer more type-safety in the configuration and could also simplify the property descriptions. I'm not sure what those properties would be called though.

Failing that, I'd type the value as String and then convert that to a long or Duration using the conversion service. That should ensure the same conversion behaviour as if the properties themselves had been more strongly typed.

Copy link
Member

Choose a reason for hiding this comment

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

I guess that ship has sailed with the sla property (or we can make this compromise only there). The problem by having two target types depending on the meter type is that we can't really get proper IDE support for this.

I am not sure how we could find a proper name for those and the convenience of doing it on a per meter id is quite appealing. So a String to long or Duration looks like the way to go.

@alexanderabramov are you interested to revisit the PR short term? RC1 is due soon.


/**
* The maximum value that this distribution summary is expected to observe.
* Controls the number of buckets shipped by percentilesHistogram. 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> maximumExpectedValue = new LinkedHashMap<>();

public Map<String, Boolean> getPercentilesHistogram() {
return this.percentilesHistogram;
}
Expand All @@ -211,6 +228,14 @@ public Map<String, ServiceLevelAgreementBoundary[]> getSla() {
return this.sla;
}

public Map<String, ServiceLevelAgreementBoundary> getMinimumExpectedValue() {
return this.minimumExpectedValue;
}

public Map<String, ServiceLevelAgreementBoundary> getMaximumExpectedValue() {
return this.maximumExpectedValue;
}

}

}
9 changes: 9 additions & 0 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 @@ -39,6 +39,7 @@
* @author Jon Schneider
* @author Phillip Webb
* @author Stephane Nicoll
* @author Alexander Abramov
* @since 2.0.0
*/
public class PropertiesMeterFilter implements MeterFilter {
Expand Down Expand Up @@ -84,6 +85,10 @@ public DistributionStatisticConfig configure(Meter.Id id,
lookup(distribution.getPercentilesHistogram(), id, null))
.percentiles(lookup(distribution.getPercentiles(), id, null))
.sla(convertSla(id.getType(), lookup(distribution.getSla(), id, null)))
.minimumExpectedValue(convertSla(id.getType(),
lookup(distribution.getMinimumExpectedValue(), id, null)))
.maximumExpectedValue(convertSla(id.getType(),
lookup(distribution.getMaximumExpectedValue(), id, null)))
.build().merge(config);
}

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

private Long convertSla(Meter.Type meterType, ServiceLevelAgreementBoundary sla) {
return (sla != null) ? sla.getValue(meterType) : null;
}

private <T> T lookup(Map<String, T> values, Id id, T defaultValue) {
if (values.isEmpty()) {
return defaultValue;
Expand Down
105 changes: 105 additions & 0 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 @@ -16,6 +16,7 @@

package org.springframework.boot.actuate.autoconfigure.metrics;

import java.time.Duration;
import java.util.Collections;

import io.micrometer.core.instrument.Meter;
Expand Down Expand Up @@ -296,6 +297,110 @@ public void configureWhenSlaLongShouldOnlyApplyToTimerAndDistributionSummary() {
.getSlaBoundaries()).isNullOrEmpty();
}

@Test
public void configureWhenHasMinimumExpectedValueShouldSetMinimumExpectedToValue() {
PropertiesMeterFilter filter = new PropertiesMeterFilter(
createProperties("distribution.minimum-expected-value.[spring.boot]=10"));
assertThat(filter.configure(createMeterId("spring.boot"),
DistributionStatisticConfig.DEFAULT).getMinimumExpectedValue())
.isEqualTo(Duration.ofMillis(10).toNanos());
}

@Test
public void configureWhenHasHigherMinimumExpectedValueShouldSetMinimumExpectedValueToValue() {
PropertiesMeterFilter filter = new PropertiesMeterFilter(
createProperties("distribution.minimum-expected-value.spring=10"));
assertThat(filter.configure(createMeterId("spring.boot"),
DistributionStatisticConfig.DEFAULT).getMinimumExpectedValue())
.isEqualTo(Duration.ofMillis(10).toNanos());
}

@Test
public void configureWhenHasHigherMinimumExpectedValueAndLowerShouldSetMinimumExpectedValueToHigher() {
PropertiesMeterFilter filter = new PropertiesMeterFilter(
createProperties("distribution.minimum-expected-value.spring=10",
"distribution.minimum-expected-value.[spring.boot]=50"));
assertThat(filter.configure(createMeterId("spring.boot"),
DistributionStatisticConfig.DEFAULT).getMinimumExpectedValue())
.isEqualTo(Duration.ofMillis(50).toNanos());
}

@Test
public void configureWhenAllMinimumExpectedValueSetShouldSetMinimumExpectedValueToValue() {
PropertiesMeterFilter filter = new PropertiesMeterFilter(
createProperties("distribution.minimum-expected-value.all=10"));
assertThat(filter.configure(createMeterId("spring.boot"),
DistributionStatisticConfig.DEFAULT).getMinimumExpectedValue())
.isEqualTo(Duration.ofMillis(10).toNanos());
}

@Test
public void configureWhenMinimumExpectedValueDurationShouldOnlyApplyToTimer() {
PropertiesMeterFilter filter = new PropertiesMeterFilter(
createProperties("distribution.minimum-expected-value.all=10ms"));
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)
.getMinimumExpectedValue()).isEqualTo(Duration.ofMillis(10).toNanos());
assertThat(filter.configure(summary, DistributionStatisticConfig.DEFAULT)
.getMinimumExpectedValue()).isEqualTo(1L);
assertThat(filter.configure(counter, DistributionStatisticConfig.DEFAULT)
.getMinimumExpectedValue()).isEqualTo(1L);
}

@Test
public void configureWhenHasMaximumExpectedValueShouldSetMaximumExpectedToValue() {
PropertiesMeterFilter filter = new PropertiesMeterFilter(createProperties(
"distribution.maximum-expected-value.[spring.boot]=5000"));
assertThat(filter.configure(createMeterId("spring.boot"),
DistributionStatisticConfig.DEFAULT).getMaximumExpectedValue())
.isEqualTo(Duration.ofMillis(5000).toNanos());
}

@Test
public void configureWhenHasHigherMaximumExpectedValueShouldSetMaximumExpectedValueToValue() {
PropertiesMeterFilter filter = new PropertiesMeterFilter(
createProperties("distribution.maximum-expected-value.spring=5000"));
assertThat(filter.configure(createMeterId("spring.boot"),
DistributionStatisticConfig.DEFAULT).getMaximumExpectedValue())
.isEqualTo(Duration.ofMillis(5000).toNanos());
}

@Test
public void configureWhenHasHigherMaximumExpectedValueAndLowerShouldSetMaximumExpectedValueToHigher() {
PropertiesMeterFilter filter = new PropertiesMeterFilter(
createProperties("distribution.maximum-expected-value.spring=5000",
"distribution.maximum-expected-value.[spring.boot]=10000"));
assertThat(filter.configure(createMeterId("spring.boot"),
DistributionStatisticConfig.DEFAULT).getMaximumExpectedValue())
.isEqualTo(Duration.ofMillis(10000).toNanos());
}

@Test
public void configureWhenAllMaximumExpectedValueSetShouldSetMaximumExpectedValueToValue() {
PropertiesMeterFilter filter = new PropertiesMeterFilter(
createProperties("distribution.maximum-expected-value.all=5000"));
assertThat(filter.configure(createMeterId("spring.boot"),
DistributionStatisticConfig.DEFAULT).getMaximumExpectedValue())
.isEqualTo(Duration.ofMillis(5000).toNanos());
}

@Test
public void configureWhenMaximumExpectedValueDurationShouldOnlyApplyToTimer() {
PropertiesMeterFilter filter = new PropertiesMeterFilter(
createProperties("distribution.maximum-expected-value.all=15s"));
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)
.getMaximumExpectedValue()).isEqualTo(Duration.ofMillis(15000).toNanos());
assertThat(filter.configure(summary, DistributionStatisticConfig.DEFAULT)
.getMaximumExpectedValue()).isEqualTo(Long.MAX_VALUE);
assertThat(filter.configure(counter, DistributionStatisticConfig.DEFAULT)
.getMaximumExpectedValue()).isEqualTo(Long.MAX_VALUE);
}

private Id createMeterId(String name) {
Meter.Type meterType = Type.TIMER;
return createMeterId(name, meterType);
Expand Down
2 changes: 2 additions & 0 deletions ...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 @@ -1378,6 +1378,8 @@ content into your application. Rather, pick only the properties that you need.
management.metrics.binders.processor.enabled=true # Whether to enable processor metrics.
management.metrics.binders.uptime.enabled=true # Whether to enable uptime metrics.
management.metrics.distribution.percentiles-histogram.*= # Whether meter IDs starting with the specified name should publish percentile histograms.
management.metrics.distribution.minimum-expected-value.*= # Minimum limit on the histogram buckets for IDs starting with the specified name. The longest match wins, the key `all` can also be used to configure all meters.
Copy link
Contributor

Choose a reason for hiding this comment

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

May or may not be worth mentioning that minimum/maximum can affect the accuracy of client-side percentiles if not set to realistic values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how to phrase it in a useful way, and the docs already invite the reader to check Micrometer documentation for more details.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we can support all here. A Duration can specify a unit, for example 30s. How would that be converted to a plain long for those meters that require that rather than a Duration?

Copy link
Member

Choose a reason for hiding this comment

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

We've decided to drop support for sla so we shouldn't add support for all here either.

Copy link
Contributor

Choose a reason for hiding this comment

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

Dropping sla is a usability concern, as this is going be a frequently set property. What is the reasoning behind this decision?

Copy link
Member

Choose a reason for hiding this comment

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

We’re not dropping support for sla as a whole, just all as it doesn’t make sense given the mixture of long and duration values depending on the meter type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see. Many thanks.

management.metrics.distribution.maximum-expected-value.*= # Maximum limit on the histogram buckets for IDs starting with the specified name. The longest match wins, the key `all` can also be used to configure all meters.
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.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.
Expand Down
4 changes: 4 additions & 0 deletions ...-boot-project/spring-boot-docs/src/main/asciidoc/production-ready-features.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1930,6 +1930,10 @@ The following properties allow per-meter customization:
| Whether to publish a histogram suitable for computing aggregable (across dimension)
percentile approximations.

| `management.metrics.distribution.minimum-expected-value`
| `management.metrics.distribution.maximum-expected-value`
| Publish less histogram buckets by clamping the range of expected values.

| `management.metrics.distribution.percentiles`
| Publish percentile values computed in your application

Expand Down