Skip to content
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.

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

Make DistributionStatisticConfig extensible #3854

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import io.micrometer.core.instrument.internal.Mergeable;

import java.time.Duration;
import java.util.Arrays;
import java.util.NavigableSet;
import java.util.TreeSet;
import java.util.stream.LongStream;
Expand Down Expand Up @@ -71,6 +72,25 @@ public class DistributionStatisticConfig implements Mergeable<DistributionStatis
@Nullable
private Integer bufferLength;

public DistributionStatisticConfig() {
}

public DistributionStatisticConfig(DistributionStatisticConfig original) {
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 sure I getting why this is needed, should not this be the concern of merge instead?

Copy link
Author

Choose a reason for hiding this comment

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

I thought it's the only way that that the subclass's constructor can copy all fields to a subclass instance, when merging

percentileHistogram = original.percentileHistogram;
if (original.percentiles != null) {
percentiles = Arrays.copyOf(original.percentiles, original.percentiles.length);
}
percentilePrecision = original.percentilePrecision;
if (original.serviceLevelObjectives != null) {
serviceLevelObjectives = Arrays.copyOf(original.serviceLevelObjectives,
original.serviceLevelObjectives.length);
}
minimumExpectedValue = original.minimumExpectedValue;
maximumExpectedValue = original.maximumExpectedValue;
expiry = original.expiry;
bufferLength = original.bufferLength;
}

public static Builder builder() {
return new Builder();
}
Expand Down Expand Up @@ -271,7 +291,15 @@ public double[] getServiceLevelObjectiveBoundaries() {

public static class Builder {

private final DistributionStatisticConfig config = new DistributionStatisticConfig();
protected final DistributionStatisticConfig config;

public Builder() {
config = new DistributionStatisticConfig();
}

protected Builder(DistributionStatisticConfig config) {
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 sure I get this either a Builder should create an instance of DistributionStatisticConfig, not receive one.

this.config = config;
}

public Builder percentilesHistogram(@Nullable Boolean enabled) {
config.percentileHistogram = enabled;
Expand Down Expand Up @@ -455,11 +483,11 @@ public Builder bufferLength(@Nullable Integer bufferLength) {
* @return A new immutable distribution configuration.
*/
public DistributionStatisticConfig build() {
validate(config);
validate();
return config;
}

private void validate(DistributionStatisticConfig distributionStatisticConfig) {
protected void validate() {
if (config.bufferLength != null && config.bufferLength <= 0) {
rejectConfig("bufferLength (" + config.bufferLength + ") must be greater than zero");
}
Expand Down Expand Up @@ -487,8 +515,8 @@ private void validate(DistributionStatisticConfig distributionStatisticConfig) {
+ ").");
}

if (distributionStatisticConfig.getServiceLevelObjectiveBoundaries() != null) {
for (double slo : distributionStatisticConfig.getServiceLevelObjectiveBoundaries()) {
if (config.getServiceLevelObjectiveBoundaries() != null) {
for (double slo : config.getServiceLevelObjectiveBoundaries()) {
if (slo <= 0) {
rejectConfig("serviceLevelObjectiveBoundaries must contain only the values greater than 0. "
+ "Found " + slo);
Expand All @@ -497,7 +525,7 @@ private void validate(DistributionStatisticConfig distributionStatisticConfig) {
}
}

private static void rejectConfig(String msg) {
protected static void rejectConfig(String msg) {
Copy link
Contributor

Choose a reason for hiding this comment

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

DoNotCallSuggester: Methods that always throw an exception should be annotated with @DonotCall to prevent calls at compilation time vs. at runtime (note that adding @DonotCall will break any existing callers of this API).


Suggested change
protected static void rejectConfig(String msg) {
@DoNotCall("Always throws io.micrometer.core.instrument.config.InvalidConfigurationException") protected static void rejectConfig(String msg) {

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

throw new InvalidConfigurationException("Invalid distribution configuration: " + msg);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
/*
* Copyright 2023 VMware, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package io.micrometer.core.instrument.distribution;

import io.micrometer.common.lang.Nullable;
import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.*;

class DistributionStatisticConfigExtensibilityTest {

static class SubDistributionStatisticConfig extends DistributionStatisticConfig {

public static final SubDistributionStatisticConfig NONE = subBuilder().build();

@Nullable
protected String extraParam;

@Nullable
public String getExtraParam() {
return extraParam;
}

SubDistributionStatisticConfig() {
super();
}

SubDistributionStatisticConfig(DistributionStatisticConfig original) {
super(original);
if (original instanceof SubDistributionStatisticConfig) {
extraParam = ((SubDistributionStatisticConfig) original).extraParam;
}
}

@Override
public SubDistributionStatisticConfig merge(DistributionStatisticConfig parent) {
DistributionStatisticConfig mergeBySuper = super.merge(parent);
SubDistributionStatisticConfig sub = new SubDistributionStatisticConfig(mergeBySuper);
if (extraParam == null && parent instanceof SubDistributionStatisticConfig) {
sub.extraParam = ((SubDistributionStatisticConfig) parent).extraParam;
}
else {
sub.extraParam = extraParam;
}
return sub;
}

public static SubBuilder subBuilder() {
return new SubBuilder(new SubDistributionStatisticConfig());
}

public static class SubBuilder extends Builder {
Copy link
Member

Choose a reason for hiding this comment

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

Is this why you need to receive a DistributionStatisticConfig in DistributionStatisticConfig.Builder?

I don't think SubDistributionStatisticConfig.Builder should extend DistributionStatisticConfig.Builder.

Copy link
Author

Choose a reason for hiding this comment

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

Yes. Which way would you recommend?


protected SubBuilder(SubDistributionStatisticConfig config) {
super(config);
}

public SubBuilder extraParam(@Nullable String extraParam) {
((SubDistributionStatisticConfig) config).extraParam = extraParam;
return this;
}

@Override
protected void validate() {
super.validate();
if ("baz".equals(((SubDistributionStatisticConfig) config).extraParam)) {
rejectConfig("No bazzing here");
}
}

@Override
public SubDistributionStatisticConfig build() {
return (SubDistributionStatisticConfig) super.build();
}

}

}

@Test
void mergeFromSuper() {
DistributionStatisticConfig origin = DistributionStatisticConfig.builder()
.percentiles(0.5, 0.9)
.serviceLevelObjectives(1.5, 2.5)
.percentilePrecision(2)
.percentilesHistogram(true)
.build();
SubDistributionStatisticConfig extra = (SubDistributionStatisticConfig) SubDistributionStatisticConfig
.subBuilder()
.extraParam("foo")
.percentiles(0.5, 0.9, 0.99)
.build();
DistributionStatisticConfig merged = origin.merge(extra);
assertEquals(2, merged.getPercentilePrecision());
assertEquals(Boolean.TRUE, merged.isPercentileHistogram());
assertArrayEquals(new double[] { 0.5, 0.9 }, merged.getPercentiles());
assertFalse(merged instanceof SubDistributionStatisticConfig);
}

@Test
void mergeFromSub() {
DistributionStatisticConfig origin = DistributionStatisticConfig.builder()
.percentiles(0.5, 0.9)
.serviceLevelObjectives(1.5, 2.5)
.percentilePrecision(2)
.percentilesHistogram(true)
.build();
SubDistributionStatisticConfig extra = (SubDistributionStatisticConfig) SubDistributionStatisticConfig
.subBuilder()
.extraParam("foo")
.percentiles(0.5, 0.9, 0.99)
.build();
SubDistributionStatisticConfig merged = extra.merge(origin);
assertEquals(2, merged.getPercentilePrecision());
assertEquals(Boolean.TRUE, merged.isPercentileHistogram());
assertArrayEquals(new double[] { 0.5, 0.9, 0.99 }, merged.getPercentiles());
assertEquals("foo", merged.getExtraParam());
}

@Test
void mergeSubsTogether() {
SubDistributionStatisticConfig origin = (SubDistributionStatisticConfig) SubDistributionStatisticConfig
.subBuilder()
.extraParam("bar")
.percentiles(0.5, 0.9)
.serviceLevelObjectives(1.5, 2.5)
.percentilePrecision(2)
.percentilesHistogram(true)
.build();
SubDistributionStatisticConfig extra = (SubDistributionStatisticConfig) SubDistributionStatisticConfig
.subBuilder()
.extraParam("foo")
.percentiles(0.5, 0.9, 0.99)
.build();
SubDistributionStatisticConfig merged = origin.merge(extra);
assertEquals(2, merged.getPercentilePrecision());
assertEquals(Boolean.TRUE, merged.isPercentileHistogram());
assertArrayEquals(new double[] { 0.5, 0.9 }, merged.getPercentiles());
assertEquals("bar", merged.getExtraParam());
}

}