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

Allow distribution config customization #4829

Merged
merged 22 commits into from
Apr 2, 2024
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
6b103e0
Add distributionConfig setter and getter to AbstractMetricCollectingB…
echo304 Apr 15, 2023
76a7461
Add distributionConfig getter to FlagsProvider and give default value
echo304 Apr 15, 2023
524f005
Add distributionConfig property and getter to Flags
echo304 Apr 16, 2023
55f0616
Fix style error
echo304 Apr 17, 2023
7c26ec9
Reflect code review
echo304 Apr 18, 2023
b706bec
Move default DistributionStatisticConfig to DistributionStatisticConf…
echo304 Apr 21, 2023
fd44022
Reflect code reviews
echo304 May 1, 2023
f30bb03
Reflect code reviews
echo304 May 2, 2023
e8fd096
Reflect code review
echo304 May 12, 2023
ddba25c
Revert non thread-safe code
echo304 May 16, 2023
daff026
Let AbstractRequestMetrics accept distStatCfg as argument
echo304 May 18, 2023
27c59c9
Remove unnecessary @Nullable annotation
echo304 May 22, 2023
08b756d
Merge branch 'main' into allow-distribution-config-customization
echo304 Jul 6, 2023
85788da
Merge branch 'main' into allow-distribution-config-customization
minwoox Jul 11, 2023
b18280c
Reflect code review
echo304 Jul 13, 2023
b8cff4a
Reflect code review
echo304 Jul 29, 2023
969c5d4
Reflect code review
echo304 Jul 31, 2023
755c120
Add test cases
echo304 Jul 31, 2023
ff7fae8
Merge branch 'main' into allow-distribution-config-customization
jrhee17 Aug 4, 2023
337c09c
Merge branch 'main' into allow-distribution-config-customization
ikhoon Aug 14, 2023
c664ebf
Merge branch 'main' into allow-distribution-config-customization
jrhee17 Mar 26, 2024
a7388cc
address comments by @ikhoon and @jrhee17
jrhee17 Mar 26, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import com.linecorp.armeria.internal.common.metric.RequestMetricSupport;

import io.micrometer.core.instrument.MeterRegistry;
import io.micrometer.core.instrument.distribution.DistributionStatisticConfig;
import io.netty.util.AttributeKey;

/**
Expand All @@ -46,20 +47,24 @@ abstract class AbstractMetricCollectingClient<I extends Request, O extends Respo
private final MeterIdPrefixFunction meterIdPrefixFunction;
@Nullable
private final BiPredicate<? super RequestContext, ? super RequestLog> successFunction;
private final DistributionStatisticConfig distributionStatisticConfig;

AbstractMetricCollectingClient(
Client<I, O> delegate, MeterIdPrefixFunction meterIdPrefixFunction,
@Nullable BiPredicate<? super RequestContext, ? super RequestLog> successFunction) {
@Nullable BiPredicate<? super RequestContext, ? super RequestLog> successFunction,
@Nullable DistributionStatisticConfig distributionStatisticConfig) {
minwoox marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@Nullable DistributionStatisticConfig distributionStatisticConfig) {
DistributionStatisticConfig distributionStatisticConfig) {

super(delegate);
this.meterIdPrefixFunction = requireNonNull(meterIdPrefixFunction, "meterIdPrefixFunction");
this.successFunction = successFunction;
this.distributionStatisticConfig = distributionStatisticConfig;
Copy link
Member

Choose a reason for hiding this comment

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

Let's add requireNonNull for consistency.

}

@Override
public final O execute(ClientRequestContext ctx, I req) throws Exception {
RequestMetricSupport.setup(ctx, REQUEST_METRICS_SET, meterIdPrefixFunction, false,
successFunction != null ? successFunction::test
: ctx.options().successFunction());
: ctx.options().successFunction(),
distributionStatisticConfig);
return unwrap().execute(ctx, req);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import com.linecorp.armeria.common.metric.MeterIdPrefixFunction;

import io.micrometer.core.instrument.MeterRegistry;
import io.micrometer.core.instrument.distribution.DistributionStatisticConfig;

/**
* Decorates an {@link HttpClient} to collect metrics into {@link MeterRegistry}.
Expand Down Expand Up @@ -65,7 +66,8 @@ public static MetricCollectingClientBuilder builder(MeterIdPrefixFunction meterI
}

MetricCollectingClient(HttpClient delegate, MeterIdPrefixFunction meterIdPrefixFunction,
@Nullable BiPredicate<? super RequestContext, ? super RequestLog> successFunction) {
super(delegate, meterIdPrefixFunction, successFunction);
@Nullable BiPredicate<? super RequestContext, ? super RequestLog> successFunction,
@Nullable DistributionStatisticConfig distributionStatisticConfig) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@Nullable DistributionStatisticConfig distributionStatisticConfig) {
DistributionStatisticConfig distributionStatisticConfig) {

super(delegate, meterIdPrefixFunction, successFunction, distributionStatisticConfig);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ public MetricCollectingClientBuilder successFunction(
*/
public MetricCollectingClient build(HttpClient delegate) {
requireNonNull(delegate, "delegate");
return new MetricCollectingClient(delegate, meterIdPrefixFunction(), successFunction());
return new MetricCollectingClient(delegate, meterIdPrefixFunction(), successFunction(),
distributionStatisticConfig());
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import com.linecorp.armeria.common.metric.MeterIdPrefixFunction;

import io.micrometer.core.instrument.MeterRegistry;
import io.micrometer.core.instrument.distribution.DistributionStatisticConfig;

/**
* Decorates an {@link RpcClient} to collect metrics into {@link MeterRegistry}.
Expand Down Expand Up @@ -65,7 +66,8 @@ public static MetricCollectingRpcClientBuilder builder(MeterIdPrefixFunction met

MetricCollectingRpcClient(
RpcClient delegate, MeterIdPrefixFunction meterIdPrefixFunction,
@Nullable BiPredicate<? super RequestContext, ? super RequestLog> successFunction) {
super(delegate, meterIdPrefixFunction, successFunction);
@Nullable BiPredicate<? super RequestContext, ? super RequestLog> successFunction,
@Nullable DistributionStatisticConfig distributionStatisticConfig) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@Nullable DistributionStatisticConfig distributionStatisticConfig) {
DistributionStatisticConfig distributionStatisticConfig) {

super(delegate, meterIdPrefixFunction, successFunction, distributionStatisticConfig);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ public MetricCollectingRpcClientBuilder successFunction(
*/
public MetricCollectingRpcClient build(RpcClient delegate) {
requireNonNull(delegate, "delegate");
return new MetricCollectingRpcClient(delegate, meterIdPrefixFunction(), successFunction());
return new MetricCollectingRpcClient(delegate, meterIdPrefixFunction(), successFunction(),
distributionStatisticConfig());
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,12 @@

import com.linecorp.armeria.common.util.Sampler;
import com.linecorp.armeria.common.util.TransportType;
import com.linecorp.armeria.internal.common.metric.DistributionStatisticConfigUtil;
import com.linecorp.armeria.server.TransientServiceOption;

import io.micrometer.core.instrument.MeterRegistry;
import io.micrometer.core.instrument.Metrics;
import io.micrometer.core.instrument.distribution.DistributionStatisticConfig;

/**
* Implementation of {@link FlagsProvider} which provides default values to {@link Flags}.
Expand Down Expand Up @@ -417,4 +419,9 @@ public MeterRegistry meterRegistry() {
public Long defaultUnhandledExceptionsReportIntervalMillis() {
return DEFAULT_UNHANDLED_EXCEPTIONS_REPORT_INTERVAL_MILLIS;
}

@Override
public DistributionStatisticConfig distributionStatisticConfig() {
return DistributionStatisticConfigUtil.DEFAULT_DIST_STAT_CFG;
}
}
29 changes: 29 additions & 0 deletions core/src/main/java/com/linecorp/armeria/common/Flags.java
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,11 @@
import com.linecorp.armeria.server.file.HttpFile;
import com.linecorp.armeria.server.logging.LoggingService;

import io.micrometer.core.instrument.DistributionSummary;
import io.micrometer.core.instrument.MeterRegistry;
import io.micrometer.core.instrument.Metrics;
import io.micrometer.core.instrument.Timer;
import io.micrometer.core.instrument.distribution.DistributionStatisticConfig;
import io.netty.buffer.ByteBufAllocator;
import io.netty.channel.ChannelOption;
import io.netty.channel.EventLoopGroup;
Expand Down Expand Up @@ -386,6 +389,9 @@ public final class Flags {
getValue(FlagsProvider::defaultUnhandledExceptionsReportIntervalMillis,
"defaultUnhandledExceptionsReportIntervalMillis", value -> value >= 0);

private static final DistributionStatisticConfig DISTRIBUTION_STATISTIC_CONFIG =
getValue(FlagsProvider::distributionStatisticConfig, "distributionStatisticConfig");

/**
* Returns the specification of the {@link Sampler} that determines whether to retain the stack
* trace of the exceptions that are thrown frequently by Armeria. A sampled exception will have the stack
Expand Down Expand Up @@ -1343,6 +1349,29 @@ public static long defaultUnhandledExceptionsReportIntervalMillis() {
return DEFAULT_UNHANDLED_EXCEPTIONS_REPORT_INTERVAL_MILLIS;
}

/**
* Returns the default {@link DistributionStatisticConfig} of the {@link Timer}s and
* {@link DistributionSummary}s created by Armeria.
*
* <p>The default value of this flag is as follows:
* <pre>{@code
* DistributionStatisticConfig.builder()
* .percentilesHistogram(false)
* .sla()
* .percentiles(PERCENTILES)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we inline PERCENTILES to show the actual value?

* .percentilePrecision(2)
* .minimumExpectedValue(1L)
* .maximumExpectedValue(Long.MAX_VALUE)
* .expiry(Duration.ofMinutes(3))
* .bufferLength(3)
* .build();
Copy link
Member

Choose a reason for hiding this comment

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

It seems like some of the methods are deprecated. Could you fix it, please? 🙏

* }</pre>
*/
@UnstableApi
public static DistributionStatisticConfig distributionStatisticConfig() {
return DISTRIBUTION_STATISTIC_CONFIG;
}

@Nullable
private static String nullableCaffeineSpec(Function<FlagsProvider, String> method, String flagName) {
return caffeineSpec(method, flagName, true);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@

import io.micrometer.core.instrument.MeterRegistry;
import io.micrometer.core.instrument.Metrics;
import io.micrometer.core.instrument.distribution.DistributionStatisticConfig;
import io.netty.channel.ChannelOption;
import io.netty.channel.EventLoopGroup;
import io.netty.handler.codec.http2.Http2Exception;
Expand Down Expand Up @@ -1003,4 +1004,27 @@ default MeterRegistry meterRegistry() {
default Long defaultUnhandledExceptionsReportIntervalMillis() {
return null;
}

/**
* Returns the {@link DistributionStatisticConfig} where armeria utilizes.
*
* <p>The default value of this flag is as follows:
* <pre>{@code
* DistributionStatisticConfig.builder()
* .percentilesHistogram(false)
* .sla()
* .percentiles(PERCENTILES)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we inline PERCENTILES to show the actual value?

* .percentilePrecision(2)
* .minimumExpectedValue(1L)
* .maximumExpectedValue(Long.MAX_VALUE)
* .expiry(Duration.ofMinutes(3))
Copy link
Member

Choose a reason for hiding this comment

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

It seems like we should also fix this. 😉

* .bufferLength(3)
* .build();
* }</pre>
*/
@Nullable
@UnstableApi
default DistributionStatisticConfig distributionStatisticConfig() {
return null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
import com.linecorp.armeria.common.logging.RequestLog;
import com.linecorp.armeria.server.ServerBuilder;

import io.micrometer.core.instrument.distribution.DistributionStatisticConfig;

/**
* Builds an implementing class of {@link AbstractMetricCollectingBuilder} instance.
*/
Expand All @@ -36,6 +38,9 @@ public abstract class AbstractMetricCollectingBuilder {
@Nullable
private BiPredicate<? super RequestContext, ? super RequestLog> successFunction;

@Nullable
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@Nullable

private DistributionStatisticConfig distributionStatisticConfig = MoreMeters.distributionStatisticConfig();
ikhoon marked this conversation as resolved.
Show resolved Hide resolved

/**
* Creates a new instance with the specified {@link MeterIdPrefixFunction}.
*/
Expand Down Expand Up @@ -83,4 +88,22 @@ public AbstractMetricCollectingBuilder successFunction(
this.successFunction = requireNonNull(successFunction, "successFunction");
return this;
}

/**
* Returns the {@code distributionStatisticConfig}.
*/
@Nullable
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@Nullable

protected final DistributionStatisticConfig distributionStatisticConfig() {
return distributionStatisticConfig;
}

/**
* Defines a custom {@link DistributionStatisticConfig} to use for the distribution config.
*/
public AbstractMetricCollectingBuilder distributionStatisticConfig(
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we also override this method in child classes?

i.e. the following won't compile:

MetricCollectingClient.builder(MeterIdPrefixFunction.ofDefault("armeria"))
                      .distributionStatisticConfig(DistributionStatisticConfig.DEFAULT)
                      .newDecorator()

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't our builder test (com.linecorp.armeria.OverriddenBuilderMethodsReturnTypeTest) catch this issue automatically? Let's make sure this PR is failed by it first.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't our builder test (com.linecorp.armeria.OverriddenBuilderMethodsReturnTypeTest) catch this issue automatically?

The builder isn't final 😅

DistributionStatisticConfig distributionStatisticConfig) {
this.distributionStatisticConfig = requireNonNull(distributionStatisticConfig,
"distributionStatisticConfig");
return this;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Streams;

import com.linecorp.armeria.common.Flags;

import io.micrometer.core.instrument.DistributionSummary;
import io.micrometer.core.instrument.DistributionSummary.Builder;
import io.micrometer.core.instrument.Measurement;
Expand All @@ -40,9 +42,6 @@
* Provides utilities for accessing {@link MeterRegistry}.
*/
public final class MoreMeters {

private static final double[] PERCENTILES = { 0, 0.5, 0.75, 0.9, 0.95, 0.98, 0.99, 0.999, 1.0 };

private static final boolean MICROMETER_1_5;

static {
Expand All @@ -62,17 +61,7 @@ public final class MoreMeters {
* (i.e. rotate every 40 seconds) does not make much sense.</li>
* </ul>
*/
private static volatile DistributionStatisticConfig distStatCfg =
DistributionStatisticConfig.builder()
.percentilesHistogram(false)
.sla()
.percentiles(PERCENTILES)
.percentilePrecision(2)
.minimumExpectedValue(1L)
.maximumExpectedValue(Long.MAX_VALUE)
.expiry(Duration.ofMinutes(3))
.bufferLength(3)
.build();
private static volatile DistributionStatisticConfig distStatCfg = Flags.distributionStatisticConfig();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private static volatile DistributionStatisticConfig distStatCfg = Flags.distributionStatisticConfig();
private static volatile DistributionStatisticConfig defaultDistStatCfg = Flags.distributionStatisticConfig();


/**
* Sets the {@link DistributionStatisticConfig} to use when the factory methods in {@link MoreMeters} create
Expand All @@ -97,33 +86,46 @@ public static DistributionStatisticConfig distributionStatisticConfig() {
*/
public static DistributionSummary newDistributionSummary(MeterRegistry registry,
String name, Iterable<Tag> tags) {
return newDistributionSummary(registry, name, tags, distStatCfg);
}

/**
* Returns a newly-registered {@link DistributionSummary} configured by
* given {@link DistributionStatisticConfig}.
*
* @param distStatCfgOverride the {@link DistributionStatisticConfig} to use
*/
public static DistributionSummary newDistributionSummary(MeterRegistry registry,
String name, Iterable<Tag> tags,
DistributionStatisticConfig distStatCfgOverride) {
requireNonNull(registry, "registry");
requireNonNull(name, "name");
requireNonNull(tags, "tags");
requireNonNull(distStatCfgOverride, "distributionStatisticConfig");
Copy link
Member

Choose a reason for hiding this comment

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

nit: The name should be the same:

Suggested change
requireNonNull(distStatCfgOverride, "distributionStatisticConfig");
requireNonNull(distStatCfgOverride, "distStatCfgOverride");

Also, how about removing Override? If you worry about naming the same with the field, we might rename it to distStatsConfig.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it!
Following your advice, I will change the name of the field to distStatsConfig and same for the string "distStatsConfig".


final Builder builder =
DistributionSummary.builder(name)
.tags(tags)
.publishPercentiles(distStatCfg.getPercentiles())
.publishPercentileHistogram(distStatCfg.isPercentileHistogram())
.percentilePrecision(distStatCfg.getPercentilePrecision())
.distributionStatisticBufferLength(distStatCfg.getBufferLength())
.distributionStatisticExpiry(distStatCfg.getExpiry());
.publishPercentiles(distStatCfgOverride.getPercentiles())
.publishPercentileHistogram(distStatCfgOverride.isPercentileHistogram())
.percentilePrecision(distStatCfgOverride.getPercentilePrecision())
.distributionStatisticBufferLength(distStatCfgOverride.getBufferLength())
.distributionStatisticExpiry(distStatCfgOverride.getExpiry());

if (MICROMETER_1_5) {
builder.maximumExpectedValue(distStatCfg.getMaximumExpectedValueAsDouble())
.minimumExpectedValue(distStatCfg.getMinimumExpectedValueAsDouble())
.serviceLevelObjectives(distStatCfg.getServiceLevelObjectiveBoundaries());
builder.maximumExpectedValue(distStatCfgOverride.getMaximumExpectedValueAsDouble())
.minimumExpectedValue(distStatCfgOverride.getMinimumExpectedValueAsDouble())
.serviceLevelObjectives(distStatCfgOverride.getServiceLevelObjectiveBoundaries());
} else {
final Double maxExpectedValueNanos = distStatCfg.getMaximumExpectedValueAsDouble();
final Double minExpectedValueNanos = distStatCfg.getMinimumExpectedValueAsDouble();
final Double maxExpectedValueNanos = distStatCfgOverride.getMaximumExpectedValueAsDouble();
final Double minExpectedValueNanos = distStatCfgOverride.getMinimumExpectedValueAsDouble();
final Long maxExpectedValue =
maxExpectedValueNanos != null ? maxExpectedValueNanos.longValue() : null;
final Long minExpectedValue =
minExpectedValueNanos != null ? minExpectedValueNanos.longValue() : null;
builder.maximumExpectedValue(maxExpectedValue);
builder.minimumExpectedValue(minExpectedValue);
final double[] slas = distStatCfg.getServiceLevelObjectiveBoundaries();
final double[] slas = distStatCfgOverride.getServiceLevelObjectiveBoundaries();
if (slas != null) {
builder.sla(Arrays.stream(slas).mapToLong(sla -> (long) sla).toArray());
}
Expand All @@ -135,12 +137,23 @@ public static DistributionSummary newDistributionSummary(MeterRegistry registry,
* Returns a newly-registered {@link Timer} configured by {@link #distributionStatisticConfig()}.
*/
public static Timer newTimer(MeterRegistry registry, String name, Iterable<Tag> tags) {
return newTimer(registry, name, tags, distStatCfg);
}

/**
* Returns a newly-registered {@link Timer} configured by given {@link DistributionStatisticConfig}.
*
* @param distStatCfgOverride the {@link DistributionStatisticConfig} to use
*/
public static Timer newTimer(MeterRegistry registry, String name, Iterable<Tag> tags,
minwoox marked this conversation as resolved.
Show resolved Hide resolved
DistributionStatisticConfig distStatCfgOverride) {
requireNonNull(registry, "registry");
requireNonNull(name, "name");
requireNonNull(tags, "tags");
requireNonNull(distStatCfgOverride, "distributionStatisticConfig");
Copy link
Member

Choose a reason for hiding this comment

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

ditto


final Double maxExpectedValueNanos = distStatCfg.getMaximumExpectedValueAsDouble();
final Double minExpectedValueNanos = distStatCfg.getMinimumExpectedValueAsDouble();
final Double maxExpectedValueNanos = distStatCfgOverride.getMaximumExpectedValueAsDouble();
final Double minExpectedValueNanos = distStatCfgOverride.getMinimumExpectedValueAsDouble();
final Duration maxExpectedValue =
maxExpectedValueNanos != null ? Duration.ofNanos(maxExpectedValueNanos.longValue()) : null;
final Duration minExpectedValue =
Expand All @@ -150,11 +163,11 @@ public static Timer newTimer(MeterRegistry registry, String name, Iterable<Tag>
.tags(tags)
.maximumExpectedValue(maxExpectedValue)
.minimumExpectedValue(minExpectedValue)
.publishPercentiles(distStatCfg.getPercentiles())
.publishPercentileHistogram(distStatCfg.isPercentileHistogram())
.percentilePrecision(distStatCfg.getPercentilePrecision())
.distributionStatisticBufferLength(distStatCfg.getBufferLength())
.distributionStatisticExpiry(distStatCfg.getExpiry())
.publishPercentiles(distStatCfgOverride.getPercentiles())
.publishPercentileHistogram(distStatCfgOverride.isPercentileHistogram())
.percentilePrecision(distStatCfgOverride.getPercentilePrecision())
.distributionStatisticBufferLength(distStatCfgOverride.getBufferLength())
.distributionStatisticExpiry(distStatCfgOverride.getExpiry())
.register(registry);
}

Expand Down