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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

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

wants to merge 5 commits into from

Conversation

ayudovin
Copy link
Contributor

@ayudovin ayudovin commented Oct 4, 2018

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 4, 2018
wilkinsona
wilkinsona requested changes Oct 4, 2018
View reviewed changes
Copy link
Member

@wilkinsona wilkinsona left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, but the proposed changes are broader than we want. We only want to drop support for all from configuring the SLA. It should still continue to work as it does today for the other properties.

.../java/org/springframework/boot/actuate/autoconfigure/metrics/PropertiesMeterFilterTests.java Outdated
@@ -115,14 +116,6 @@ public void acceptWhenHasHigherEnableTrueExactEnableFalseShouldReturnDeny() {
.isEqualTo(MeterFilterReply.DENY);
}

@Test
public void acceptWhenHasAllEnableFalseShouldReturnDeny() {
Copy link
Member

Choose a reason for hiding this comment

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

This test should be kept and should continue to pass. We only want to drop support for all when configuring the SLA. It should be kept for controlling which meters are enabled.

.../java/org/springframework/boot/actuate/autoconfigure/metrics/PropertiesMeterFilterTests.java Outdated
@@ -181,14 +174,6 @@ public void configureWhenHasHigherHistogramFalseAndLowerTrueShouldSetPercentiles
DistributionStatisticConfig.DEFAULT).isPercentileHistogram()).isTrue();
}

@Test
public void configureWhenAllHistogramTrueSetPercentilesHistogramToTrue() {
Copy link
Member

Choose a reason for hiding this comment

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

This test should be kept and should continue to pass.

.../java/org/springframework/boot/actuate/autoconfigure/metrics/PropertiesMeterFilterTests.java Outdated
@@ -217,15 +202,6 @@ public void configureWhenHasHigherPercentilesAndLowerShouldSetPercentilesToHighe
3.5, 4);
}

@Test
public void configureWhenAllPercentilesSetShouldSetPercentilesToValue() {
Copy link
Member

Choose a reason for hiding this comment

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

This test should be kept and should continue to pass.

@ayudovin
Copy link
Contributor Author

ayudovin commented Oct 4, 2018

@wilkinsona, thank you for your comments. I have updated follow your advice.

wilkinsona
wilkinsona requested changes Oct 4, 2018
View reviewed changes
Copy link
Member

@wilkinsona wilkinsona left a comment

Choose a reason for hiding this comment

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

Thanks for the updates but we're not quite there yet. I've left a few comments describing what I think needs to be changed. Can you please take a look?

.../main/java/org/springframework/boot/actuate/autoconfigure/metrics/PropertiesMeterFilter.java Outdated
@@ -97,7 +102,8 @@ public DistributionStatisticConfig configure(Meter.Id id,
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

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.

.../main/java/org/springframework/boot/actuate/autoconfigure/metrics/PropertiesMeterFilter.java Outdated
.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

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);

.../main/java/org/springframework/boot/actuate/autoconfigure/metrics/PropertiesMeterFilter.java Outdated
@@ -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

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);
}

@ayudovin
Copy link
Contributor Author

ayudovin commented Oct 4, 2018

@wilkinsona, I took a look and added changes by your comments

@wilkinsona wilkinsona mentioned this pull request Oct 5, 2018
Closed
@wilkinsona wilkinsona self-assigned this Oct 5, 2018
@wilkinsona wilkinsona mentioned this pull request Oct 5, 2018
Closed
@wilkinsona wilkinsona added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 5, 2018
@wilkinsona wilkinsona added this to the 2.1.x milestone Oct 5, 2018
wilkinsona pushed a commit that referenced this pull request Oct 5, 2018
@ayudovin @wilkinsona
Drop support for "all" from management.metrics.distribution.sla
ccb964e 
See gh-14684
@wilkinsona wilkinsona closed this in c9da881 Oct 5, 2018
wilkinsona added a commit that referenced this pull request Oct 5, 2018
@wilkinsona
Merge pull request #14684 from Artsiom Yudovin
b0e1dc2 
* gh-14684:
  Polish "Drop support for "all" from management.metrics.distribution.sla"
  Drop support for "all" from management.metrics.distribution.sla
@snicoll snicoll modified the milestones: 2.1.x, 2.1.0.RC1 Oct 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wilkinsona wilkinsona wilkinsona requested changes

Assignees

@wilkinsona wilkinsona

Labels
type: enhancement A general enhancement
Projects
None yet
Milestone
2.1.0.RC1
Development

Successfully merging this pull request may close these issues.
4 participants
@ayudovin @wilkinsona @snicoll @spring-projects-issues