Support more Micrometer histogram properties #14139

alexanderabramov · 2018-08-19T19:46:34Z

Adds configuration properties for Micrometer histogram settings:
"minimumExpectedValue", "maximumExpectedValue"

snicoll · 2018-08-20T07:53:31Z

paging @jkschneider for a review.

jkschneider · 2018-08-20T12:55:37Z

.../src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/MetricsProperties.java

+		 * 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<>();


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

WDYT @snicoll?

Agree. Which alternative do you recommend:

copy ServiceLevelAgreementBoundary to a better named class with exactly the same code

rename ServiceLevelAgreementBoundary

other?

ping @snicoll

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.

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.

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.

jkschneider · 2018-08-20T12:58:11Z

.../java/org/springframework/boot/actuate/autoconfigure/metrics/PropertiesMeterFilterTests.java

+				"distribution.maximum-expected-value.[spring.boot]=5000"));
+		assertThat(filter.configure(createMeterId("spring.boot"),
+				DistributionStatisticConfig.DEFAULT).getMaximumExpectedValue())
+						.isEqualTo(5000000000L);


For those that aren't used to seeing the difference in significant digits between nanoseconds and milliseconds, it might be more clear to write Duration.ofMillis(5000).toNanos()

jkschneider · 2018-08-20T13:00:28Z

spring-boot-project/spring-boot-docs/src/main/asciidoc/appendix-application-properties.adoc

@@ -1365,6 +1365,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.


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

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.

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?

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

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

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.

Oh, I see. Many thanks.

Adds configuration properties for Micrometer histogram settings: "minimumExpectedValue", "maximumExpectedValue"

alexanderabramov · 2018-09-04T18:46:40Z

rebased to latest master
made PropertiesMeterFilterTests more readable by using Duration.ofMillis().toNanos()

alexanderabramov · 2018-10-02T10:09:46Z

@snicoll @jkschneider So what are the next steps on this PR?

snicoll · 2018-10-09T09:04:54Z

@alexanderabramov there is some additional discussion in the comments above. It would be nice to know if you intend to look at them soon as RC1 is due next week.

This commit adds configuration properties for Micrometer histogram settings: "minimumExpectedValue" and "maximumExpectedValue". See spring-projectsgh-14139

Closes spring-projectsgh-14139

snicoll · 2018-10-12T09:47:44Z

I've started to review and polish the PR since RC1 is around the corner.

I was trying to find a common type for the meter value and renamed ServiceLevelAgreementBoundary to MeterValue. I am not sure that type makes sense (as it's very generic).

I may have found a side effect in the binder while doing that so I am investigating that. Perhaps we'll have to postpone this one I am afraid.

wilkinsona · 2018-10-12T10:14:28Z

I was trying to find a common type for the meter value and renamed ServiceLevelAgreementBoundary to MeterValue. I am not sure that type makes sense (as it's very generic).

As we just discussed on Slack, I think it's worth having two separate types here as, conceptually, the two things are quite different. Given that we have ServiceLevelAgreementBoundary, I'd call it something like DistributionSummaryBoundary.

I may have found a side effect in the binder while doing that so I am investigating that

Unless of course this forces us to use a String anyway…

This commit adds configuration properties for Micrometer histogram settings: "minimumExpectedValue" and "maximumExpectedValue". See gh-14139

* pr/14139: Polish "Improve Micrometer histogram properties support" Improve Micrometer histogram properties support

snicoll · 2018-10-12T12:25:31Z

@alexanderabramov thank you very much for making your first contribution to Spring Boot. I've merged it with a polish commit.

I didn't want to force users to wraps their meter IDs with brackets so I went ahead with a String type for now. We can reconsider and create a dedicated type when/if we fix #14796.

See spring-projects/spring-boot#14139

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 19, 2018

jkschneider reviewed Aug 20, 2018

View reviewed changes

Support more Micrometer histogram properties

3b89a8d

Adds configuration properties for Micrometer histogram settings: "minimumExpectedValue", "maximumExpectedValue"

snicoll added for: team-attention An issue we'd like other members of the team to review status: waiting-for-feedback We need additional information before we can continue and removed for: team-attention An issue we'd like other members of the team to review labels Oct 3, 2018

snicoll self-assigned this Oct 12, 2018

snicoll added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 12, 2018

snicoll added a commit to snicoll/spring-boot that referenced this pull request Oct 12, 2018

Polish "Improve Micrometer histogram properties support"

a507738

Closes spring-projectsgh-14139

snicoll removed the status: waiting-for-feedback We need additional information before we can continue label Oct 12, 2018

snicoll mentioned this pull request Oct 12, 2018

Allow to define that a binding target represents a scalar value #14796

Open

snicoll added this to the 2.1.0.RC1 milestone Oct 12, 2018

snicoll pushed a commit that referenced this pull request Oct 12, 2018

Improve Micrometer histogram properties support

c1c79ab

This commit adds configuration properties for Micrometer histogram settings: "minimumExpectedValue" and "maximumExpectedValue". See gh-14139

snicoll closed this in 0ff1b25 Oct 12, 2018

snicoll added a commit that referenced this pull request Oct 12, 2018

Merge pull request #14139 from alexanderabramov

e37145a

* pr/14139: Polish "Improve Micrometer histogram properties support" Improve Micrometer histogram properties support

izeye added a commit to izeye/micrometer that referenced this pull request Oct 17, 2018

Support distribution's min/max expected values via properties

73f414a

See spring-projects/spring-boot#14139

izeye mentioned this pull request Oct 17, 2018

Support distribution's min/max expected values via properties micrometer-metrics/micrometer#943

Merged

jkschneider pushed a commit to micrometer-metrics/micrometer that referenced this pull request Oct 17, 2018

Support distribution's min/max expected values via properties

38ca76a

See spring-projects/spring-boot#14139

Support more Micrometer histogram properties #14139

Support more Micrometer histogram properties #14139

Uh oh!

Conversation

alexanderabramov commented Aug 19, 2018

Uh oh!

snicoll commented Aug 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexanderabramov Aug 20, 2018 • edited by philwebb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkschneider Aug 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexanderabramov commented Sep 4, 2018

Uh oh!

alexanderabramov commented Oct 2, 2018

Uh oh!

snicoll commented Oct 9, 2018

Uh oh!

snicoll commented Oct 12, 2018

Uh oh!

wilkinsona commented Oct 12, 2018

Uh oh!

snicoll commented Oct 12, 2018

Uh oh!

Uh oh!

alexanderabramov Aug 20, 2018 •

edited by philwebb

Loading

jkschneider Aug 20, 2018 •

edited

Loading