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 all 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,25 @@ 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,
DistributionStatisticConfig distributionStatisticConfig) {
super(delegate);
this.meterIdPrefixFunction = requireNonNull(meterIdPrefixFunction, "meterIdPrefixFunction");
this.successFunction = successFunction;
this.distributionStatisticConfig =
requireNonNull(distributionStatisticConfig, "distributionStatisticConfig");
}

@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,
DistributionStatisticConfig distributionStatisticConfig) {
super(delegate, meterIdPrefixFunction, successFunction, distributionStatisticConfig);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
import com.linecorp.armeria.common.metric.AbstractMetricCollectingBuilder;
import com.linecorp.armeria.common.metric.MeterIdPrefixFunction;

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

/**
* Builds a {@link MetricCollectingClient} instance.
*/
Expand All @@ -41,13 +43,20 @@
return (MetricCollectingClientBuilder) super.successFunction(successFunction);
}

@Override
public MetricCollectingClientBuilder distributionStatisticConfig(
DistributionStatisticConfig distributionStatisticConfig) {
return (MetricCollectingClientBuilder) super.distributionStatisticConfig(distributionStatisticConfig);

Check warning on line 49 in core/src/main/java/com/linecorp/armeria/client/metric/MetricCollectingClientBuilder.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/client/metric/MetricCollectingClientBuilder.java#L49

Added line #L49 was not covered by tests
}

/**
* Returns a newly-created {@link MetricCollectingClient} decorating {@link HttpClient} based
* on the properties of this builder.
*/
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,
DistributionStatisticConfig distributionStatisticConfig) {
super(delegate, meterIdPrefixFunction, successFunction, distributionStatisticConfig);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
import com.linecorp.armeria.common.metric.AbstractMetricCollectingBuilder;
import com.linecorp.armeria.common.metric.MeterIdPrefixFunction;

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

/**
* Builds a {@link MetricCollectingRpcClient} instance.
*/
Expand All @@ -41,13 +43,21 @@
return (MetricCollectingRpcClientBuilder) super.successFunction(successFunction);
}

@Override
public MetricCollectingRpcClientBuilder distributionStatisticConfig(
DistributionStatisticConfig distributionStatisticConfig) {
return (MetricCollectingRpcClientBuilder)
super.distributionStatisticConfig(distributionStatisticConfig);

Check warning on line 50 in core/src/main/java/com/linecorp/armeria/client/metric/MetricCollectingRpcClientBuilder.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/com/linecorp/armeria/client/metric/MetricCollectingRpcClientBuilder.java#L49-L50

Added lines #L49 - L50 were not covered by tests
}

/**
* Returns a newly-created {@link MetricCollectingRpcClient} decorating {@link RpcClient} based
* on the properties of this builder.
*/
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 @@ -32,6 +32,7 @@

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 @@ -467,4 +468,9 @@ public MeterRegistry meterRegistry() {
public Long defaultUnhandledExceptionsReportIntervalMillis() {
return DEFAULT_UNHANDLED_EXCEPTIONS_REPORT_INTERVAL_MILLIS;
}

@Override
public DistributionStatisticConfig distributionStatisticConfig() {
return DistributionStatisticConfigUtil.DEFAULT_DIST_STAT_CFG;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* Copyright 2024 LINE Corporation
*
* LINE Corporation licenses this file to you 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 com.linecorp.armeria.common;

import java.time.Duration;

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

final class DistributionStatisticConfigUtil {
static final DistributionStatisticConfig DEFAULT_DIST_STAT_CFG =
DistributionStatisticConfig.builder()
.percentilesHistogram(false)
.serviceLevelObjectives()
.percentiles(0, 0.5, 0.75, 0.9, 0.95, 0.98, 0.99, 0.999, 1.0)
.percentilePrecision(2)
.minimumExpectedValue(1.0)
.maximumExpectedValue(Double.MAX_VALUE)
.expiry(Duration.ofMinutes(3))
.bufferLength(3)
.build();

private DistributionStatisticConfigUtil() {}
}
30 changes: 30 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 @@ -405,6 +408,9 @@ private static boolean validateTransportType(TransportType transportType, String
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 @@ -1475,6 +1481,30 @@ 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)
* .serviceLevelObjectives()
* .percentiles(
* 0, 0.5, 0.75, 0.9, 0.95, 0.98, 0.99, 0.999, 1.0)
* .percentilePrecision(2)
* .minimumExpectedValue(1.0)
* .maximumExpectedValue(Double.MAX_VALUE)
* .expiry(Duration.ofMinutes(3))
* .bufferLength(3)
* .build();
* }</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 @@ -1118,4 +1119,28 @@ 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)
* .serviceLevelObjectives()
* .percentiles(
* 0, 0.5, 0.75, 0.9, 0.95, 0.98, 0.99, 0.999, 1.0)
* .percentilePrecision(2)
* .minimumExpectedValue(1.0)
* .maximumExpectedValue(Double.MAX_VALUE)
* .expiry(Duration.ofMinutes(3))
* .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,8 @@ public abstract class AbstractMetricCollectingBuilder {
@Nullable
private BiPredicate<? super RequestContext, ? super RequestLog> successFunction;

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 +87,21 @@ public AbstractMetricCollectingBuilder successFunction(
this.successFunction = requireNonNull(successFunction, "successFunction");
return this;
}

/**
* Returns the {@code distributionStatisticConfig}.
*/
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;
}
}