Allow distribution config customization #4829

echo304 · 2023-04-15T02:13:48Z

Motivation:
As of now, users need to use MoreMeters.setDistributionStatisticConfig(config) to specify the distribution config.
It prevents users from configuring different configs depending on the client.
This PR is to resolve above issue.

Modifications:

Add distributionStatisticConfig getter to FlagsProvider and DefaultFlagsProvider
Add distributionStatisticConfig setter and getter to AbstractMetricCollectingBuilder

Result:

Closes Provide a way to set DistributionSummary config via builder #4781
User can specify custom Distribution Config with builder

…uilder

echo304 · 2023-04-15T02:15:23Z

To be honest, I'm not sure if this is right direction. 😱
Any advice/review/comment about the change is welcome!

minwoox · 2023-04-17T08:46:26Z

core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java

+
+    @Override
+    public DistributionStatisticConfig distributionStatisticConfig() {
+        return DistributionStatisticConfig.DEFAULT;


We don't use DistributionStatisticConfig.DEFAULT as default.
We use

DistributionStatisticConfig.builder() .percentilesHistogram(false) .sla() .percentiles(PERCENTILES) .percentilePrecision(2) .minimumExpectedValue(1L) .maximumExpectedValue(Long.MAX_VALUE) .expiry(Duration.ofMinutes(3)) .bufferLength(3) .build();

from MoreMeters.distStatCfg as default instead. So we need to:

Define the default config in MoreMeters.DEFAULT_DIST_STAT_CFG
private static volatile DistributionStatisticConfig DEFAULT_DIST_STAT_CFG = DistributionStatisticConfig.builder() .percentilesHistogram(false) .sla() .percentiles(PERCENTILES) .percentilePrecision(2) .minimumExpectedValue(1L) .maximumExpectedValue(Long.MAX_VALUE) .expiry(Duration.ofMinutes(3)) .bufferLength(3) .build();

Use the value from the DefaultFlagsProvider:
@Override public DistributionStatisticConfig distributionStatisticConfig() { return MoreMeters.defaultDistributionStatisticConfig(); }

Set the Flags.distributionStatisticConfig() to distStatCfg.
private static volatile DistributionStatisticConfig distStatCfg = Flags.distributionStatisticConfig();

In this way, we can add this new API without breaking the behavior.

Now, we need to use the MoreMeters.distributionStatisticConfig() as the default value for the AbstractMetricCollectingBuilder:

public abstract class AbstractMetricCollectingBuilder { private DistributionStatisticConfig distributionStatisticConfig = MoreMeters.distributionStatisticConfig; }

The distributionStatisticConfig will be passed to the constructor of MetricCollectingClient and MetricCollectingService and it will be also passed to RequestMetricSupport.setup(...).
It will be eventually used to create the summary here: https://github.com/line/armeria/blob/main/core/src/main/java/com/linecorp/armeria/internal/common/metric/RequestMetricSupport.java#L221
(MoreMeters.newDistributionSummary() method no longer need to be in MoreMeters. We can move it to RequestMetricSupport as a private method.)

Please let me know if there's anything unclear. 😄

Thanks for the feedback!

@minwoox
I implemented as you mentioned above but setting MoreMeters.distStatCfg from Flags.distributionStatisticConfig() doesn't work well on some tests.
It complains that MoteMeters.distStatCfg is null. So I checked the failed test from local environment and I got this.

In theory, Flags.DISTRIBUTION_STATISTIC_CONFIG should be populated by DefaultFlagsProvider.distributionStatisticConfig() and thus MoreMeters.distStatCfg should be populated by `Flags.distributionStatisticConfig().
But in fact, it's null. Maybe there is some timing issue...or circular referencing...or whatever unknown problem.

Any idea?

Sorry about the problem. 😅
The DEFAULT_DIST_STAT_CFG is not instantiated before defaultDistributionStatisticConfig() is called. 😓
Let's move it to another class:

package com.linecorp.armeria.internal.common.metric; ... public final class DistributionStatisticConfigUtil { public static final DistributionStatisticConfig DEFAULT_DIST_STAT_CFG = DistributionStatisticConfig.builder()... ... } // DefaultFlagProvider @Override public DistributionStatisticConfig distributionStatisticConfig() { return DistributionStatisticConfigUtil.DEFAULT_DIST_STAT_CFG; }

- Use MoreMeters default distribution config as default

…igUtil

core/src/main/java/com/linecorp/armeria/common/metric/AbstractMetricCollectingBuilder.java

- allow MetricCollecting{Client,RpcClient,Service} to receive DistributionStatisticConfig

trustin

Thanks for the PR, @echo304! Left some comments you might want to take a look. 🙇

trustin · 2023-05-02T12:42:40Z

core/src/main/java/com/linecorp/armeria/common/Flags.java

@@ -1343,6 +1347,16 @@ public static long defaultUnhandledExceptionsReportIntervalMillis() {
        return DEFAULT_UNHANDLED_EXCEPTIONS_REPORT_INTERVAL_MILLIS;
    }

+    /**
+     * Returns the {@link DistributionStatisticConfig} where armeria utilizes.


Suggested change

* Returns the {@link DistributionStatisticConfig} where armeria utilizes.

* Returns the default {@link DistributionStatisticConfig} of the {@link Timer}s and {@link DistributionSummary}s created by Armeria.

trustin · 2023-05-02T12:43:42Z

core/src/main/java/com/linecorp/armeria/common/Flags.java

+    /**
+     * Returns the {@link DistributionStatisticConfig} where armeria utilizes.
+     *
+     * <p>The default value of this flag is {@link DistributionStatisticConfig#DEFAULT}</p>


This needs an update. We could paste some code here:

<p>The default value of this flag is as follows: <pre>{@code DistributionStatisticConfig .builder() ... .build(); }</pre>

trustin · 2023-05-02T12:44:12Z

core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java

+    /**
+     * Returns the {@link DistributionStatisticConfig} where armeria utilizes.
+     *
+     * <p>The default value of this flag is {@link DistributionStatisticConfig#DEFAULT}</p>
+     */


Ditto - please keep in sync with Flags.distributionStatisticConfig().

trustin · 2023-05-02T12:45:03Z

...c/main/java/com/linecorp/armeria/internal/common/metric/DistributionStatisticConfigUtil.java

+public final class DistributionStatisticConfigUtil {
+ private static final double[] PERCENTILES = { 0, 0.5, 0.75, 0.9, 0.95, 0.98, 0.99, 0.999, 1.0 };
+
+ public static final DistributionStatisticConfig DEFAULT_DIST_STAT_CFG =
+         DistributionStatisticConfig.builder()


Could you reformat this file?

trustin · 2023-05-02T12:45:26Z

...c/main/java/com/linecorp/armeria/internal/common/metric/DistributionStatisticConfigUtil.java

+import io.micrometer.core.instrument.distribution.DistributionStatisticConfig;
+
+public final class DistributionStatisticConfigUtil {
+ private static final double[] PERCENTILES = { 0, 0.5, 0.75, 0.9, 0.95, 0.98, 0.99, 0.999, 1.0 };


It's used only once, so I guess we can inline it.

core/src/main/java/com/linecorp/armeria/server/metric/MetricCollectingService.java

trustin · 2023-05-02T12:48:04Z

Approved by mistake. Sorry! 🙏

- Fix comments - Reformat a file

minwoox

Let's keep moving on. 😆

core/src/main/java/com/linecorp/armeria/client/metric/AbstractMetricCollectingClient.java

- Override MoreMeters.distributionStatisticConfig from RequestMetricSupport.setup

echo304 · 2023-05-12T06:13:57Z

core/src/main/java/com/linecorp/armeria/internal/common/metric/RequestMetricSupport.java

@@ -66,6 +68,8 @@ public static void setup(
                          RequestLogProperty.NAME,
                          RequestLogProperty.SESSION)
           .thenAccept(log -> onRequest(log, meterIdPrefixFunction, server, successFunction));
+
+        MoreMeters.setDistributionStatisticConfig(distributionStatisticConfig);


Instead of your advice(#4829 (comment)), I've override a distributionStatisticConfig from here.

Do you think this is too dangerous approach?
@minwoox

It is not thread safe. The two requests having different DistributionStatisticConfigs update the global DistributionStatisticConfig concurrently, the result is unpredictable.

That makes sense to me 😞

As @ikhoon said, we can't just set the config here.
Is there any reason that you are reluctant to use the config in the onResponse method?

Let's pass it to the setup method in execute(...) so we can use it when creating the distribution:

RequestMetricSupport.setup(ctx, REQUEST_METRICS_SET, meterIdPrefixFunction, false, successFunction != null ? successFunction::test : ctx.options().successFunction(), distributionStatisticConfig);

In RequestMetricSupport

private static void onRequest( RequestLog log, MeterIdPrefixFunction meterIdPrefixFunction, boolean server, SuccessFunction successFunction, DistributionStatisticConfig distStatCfg) { ...} private static void onResponse( RequestLog log, MeterIdPrefixFunction meterIdPrefixFunction, boolean server, SuccessFunction successFunction, DistributionStatisticConfig distStatCfg) { ... new DefaultClientRequestMetrics(distStatCfg); ... new DefaultServiceRequestMetrics(distStatCfg); ... }

@minwoox I looked into DefaultClientRequestMetrics class but it seems like there is no injection point as of now.
Because the constructor of the class calls MoreMeters.newTimer() but to pass given distStatCfg I need to change current implementation of newTimer().
That's why I tried to use setter of MoreMeters.

If you meant that I do modify newTimer method signature and its logic, just let me know. I will do it :)

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java

ikhoon · 2023-05-22T11:01:08Z

core/src/main/java/com/linecorp/armeria/common/metric/AbstractMetricCollectingBuilder.java

@@ -36,6 +38,9 @@ public abstract class AbstractMetricCollectingBuilder {
    @Nullable
    private BiPredicate<? super RequestContext, ? super RequestLog> successFunction;

+    @Nullable


Suggested change

@Nullable

ikhoon · 2023-05-22T11:01:18Z

core/src/main/java/com/linecorp/armeria/common/metric/AbstractMetricCollectingBuilder.java

+    /**
+     * Returns the {@code distributionStatisticConfig}.
+     */
+    @Nullable


Suggested change

@Nullable

# Conflicts: # core/src/main/java/com/linecorp/armeria/internal/common/metric/RequestMetricSupport.java # core/src/main/java/com/linecorp/armeria/server/metric/MetricCollectingService.java

minwoox

Left only minor comments. I think it's almost done. 😄

minwoox · 2023-07-11T10:16:59Z

core/src/main/java/com/linecorp/armeria/client/metric/AbstractMetricCollectingClient.java


    AbstractMetricCollectingClient(
            Client<I, O> delegate, MeterIdPrefixFunction meterIdPrefixFunction,
-            @Nullable BiPredicate<? super RequestContext, ? super RequestLog> successFunction) {
+            @Nullable BiPredicate<? super RequestContext, ? super RequestLog> successFunction,
+            @Nullable DistributionStatisticConfig distributionStatisticConfig) {


Suggested change

@Nullable DistributionStatisticConfig distributionStatisticConfig) {

DistributionStatisticConfig distributionStatisticConfig) {

minwoox · 2023-07-11T10:17:16Z

core/src/main/java/com/linecorp/armeria/client/metric/MetricCollectingClient.java

-                           @Nullable BiPredicate<? super RequestContext, ? super RequestLog> successFunction) {
-        super(delegate, meterIdPrefixFunction, successFunction);
+                           @Nullable BiPredicate<? super RequestContext, ? super RequestLog> successFunction,
+                           @Nullable DistributionStatisticConfig distributionStatisticConfig) {


Suggested change

@Nullable DistributionStatisticConfig distributionStatisticConfig) {

DistributionStatisticConfig distributionStatisticConfig) {

minwoox · 2023-07-11T10:17:43Z

core/src/main/java/com/linecorp/armeria/client/metric/MetricCollectingRpcClient.java

-            @Nullable BiPredicate<? super RequestContext, ? super RequestLog> successFunction) {
-        super(delegate, meterIdPrefixFunction, successFunction);
+            @Nullable BiPredicate<? super RequestContext, ? super RequestLog> successFunction,
+            @Nullable DistributionStatisticConfig distributionStatisticConfig) {


Suggested change

@Nullable DistributionStatisticConfig distributionStatisticConfig) {

DistributionStatisticConfig distributionStatisticConfig) {

minwoox · 2023-07-11T10:22:19Z

core/src/main/java/com/linecorp/armeria/common/Flags.java

+     * DistributionStatisticConfig.builder()
+     *     .percentilesHistogram(false)
+     *     .sla()
+     *     .percentiles(PERCENTILES)
+     *     .percentilePrecision(2)
+     *     .minimumExpectedValue(1L)
+     *     .maximumExpectedValue(Long.MAX_VALUE)
+     *     .expiry(Duration.ofMinutes(3))
+     *     .bufferLength(3)
+     *     .build();


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

minwoox · 2023-07-11T10:26:25Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java

        requireNonNull(registry, "registry");
        requireNonNull(name, "name");
        requireNonNull(tags, "tags");
+        requireNonNull(distStatCfgOverride, "distributionStatisticConfig");


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.

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

minwoox · 2023-07-11T10:26:49Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java

        requireNonNull(registry, "registry");
        requireNonNull(name, "name");
        requireNonNull(tags, "tags");
+        requireNonNull(distStatCfgOverride, "distributionStatisticConfig");


minwoox · 2023-07-11T11:34:23Z

core/src/main/java/com/linecorp/armeria/server/metric/MetricCollectingService.java


    MetricCollectingService(HttpService delegate,
                            MeterIdPrefixFunction meterIdPrefixFunction,
-                            @Nullable BiPredicate<? super RequestContext, ? super RequestLog> successFunction) {
+                            @Nullable BiPredicate<? super RequestContext, ? super RequestLog> successFunction,
+                            @Nullable DistributionStatisticConfig distributionStatisticConfig) {


Suggested change

@Nullable DistributionStatisticConfig distributionStatisticConfig) {

DistributionStatisticConfig distributionStatisticConfig) {

minwoox · 2023-07-11T11:35:41Z

core/src/test/java/com/linecorp/armeria/internal/common/metric/RequestMetricSupportTest.java

        RequestMetricSupport.setup(ctx, REQUEST_METRICS_SET, meterIdPrefixFunction, false,
-                                   SuccessFunction.ofDefault());
+                                   SuccessFunction.ofDefault(), distributionStatisticConfig);


Let's inline:

Suggested change

SuccessFunction.ofDefault(), distributionStatisticConfig);

SuccessFunction.ofDefault(), DEFAULT_DIST_STAT_CFG);

minwoox · 2023-07-11T11:35:50Z

core/src/test/java/com/linecorp/armeria/internal/common/metric/RequestMetricSupportTest.java

        RequestMetricSupport.setup(ctx, REQUEST_METRICS_SET, meterIdPrefixFunction, true,
-                                   SuccessFunction.ofDefault());
+                                   SuccessFunction.ofDefault(), distributionStatisticConfig);


Suggested change

SuccessFunction.ofDefault(), distributionStatisticConfig);

SuccessFunction.ofDefault(), DEFAULT_DIST_STAT_CFG);

Please also fix the below changes as well. 😉

echo304 · 2023-07-13T07:35:12Z

...c/main/java/com/linecorp/armeria/internal/common/metric/DistributionStatisticConfigUtil.java

+                                       .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)


I replaced deprecated methods as you mentioned here
Does it make sense to you?
@minwoox

Yeah, it looks good! 👍

minwoox

Left only minor comments. Thanks @echo304! 👍 👍

minwoox · 2023-07-20T09:19:54Z

core/src/main/java/com/linecorp/armeria/client/metric/AbstractMetricCollectingClient.java

        super(delegate);
        this.meterIdPrefixFunction = requireNonNull(meterIdPrefixFunction, "meterIdPrefixFunction");
        this.successFunction = successFunction;
+        this.distributionStatisticConfig = distributionStatisticConfig;


Let's add requireNonNull for consistency.

minwoox · 2023-07-20T09:21:14Z

core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java

+     *     .percentilesHistogram(false)
+     *     .sla()
+     *     .percentiles(PERCENTILES)
+     *     .percentilePrecision(2)
+     *     .minimumExpectedValue(1L)
+     *     .maximumExpectedValue(Long.MAX_VALUE)
+     *     .expiry(Duration.ofMinutes(3))


It seems like we should also fix this. 😉

minwoox · 2023-07-20T09:23:26Z

core/src/main/java/com/linecorp/armeria/server/metric/MetricCollectingService.java

        super(delegate);
        this.meterIdPrefixFunction = requireNonNull(meterIdPrefixFunction, "meterIdPrefixFunction");
        this.successFunction = successFunction;
+        this.distributionStatisticConfig = distributionStatisticConfig;


minwoox · 2023-07-20T09:23:59Z

core/src/test/java/com/linecorp/armeria/internal/common/metric/RequestMetricSupportTest.java

        RequestMetricSupport.setup(sctx, REQUEST_METRICS_SET,
                                   MeterIdPrefixFunction.ofDefault("foo"), true,
-                                   SuccessFunction.ofDefault());
+                                   SuccessFunction.ofDefault(), distributionStatisticConfig);


Let's just inline. 😉

minwoox · 2023-07-20T09:24:04Z

core/src/test/java/com/linecorp/armeria/internal/common/metric/RequestMetricSupportTest.java

@@ -388,7 +391,7 @@ void serviceAndClientContext() {
                                        .build();
            RequestMetricSupport.setup(cctx, AttributeKey.valueOf("differentKey"),
                                       MeterIdPrefixFunction.ofDefault("bar"), false,
-                                       SuccessFunction.ofDefault());
+                                       SuccessFunction.ofDefault(), distributionStatisticConfig);


minwoox · 2023-07-20T09:24:14Z

core/src/test/java/com/linecorp/armeria/internal/common/metric/RequestMetricSupportTest.java

@@ -439,12 +442,15 @@ void customSuccessFunction() {
                                    .build();

        final MeterIdPrefixFunction meterIdPrefixFunction = MeterIdPrefixFunction.ofDefault("foo");
+        final DistributionStatisticConfig distributionStatisticConfig = DEFAULT_DIST_STAT_CFG;


minwoox

Left only minor comments. Thanks @echo304! 👍 👍

codecov · 2023-07-29T13:19:13Z

Codecov Report

Attention: Patch coverage is 90.10989% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 74.05%. Comparing base (0ab1721) to head (a7388cc).
Report is 1 commits behind head on main.

Files	Patch %	Lines
...com/linecorp/armeria/common/metric/MoreMeters.java	80.76%	5 Missing ⚠️
...lient/metric/MetricCollectingRpcClientBuilder.java	50.00%	2 Missing ⚠️
...a/client/metric/MetricCollectingClientBuilder.java	66.66%	1 Missing ⚠️
.../server/metric/MetricCollectingServiceBuilder.java	66.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #4829      +/-   ##
============================================
+ Coverage     74.03%   74.05%   +0.01%     
- Complexity    20854    20865      +11     
============================================
  Files          1807     1808       +1     
  Lines         76754    76790      +36     
  Branches       9790     9792       +2     
============================================
+ Hits          56827    56865      +38     
  Misses        15301    15301              
+ Partials       4626     4624       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ikhoon · 2023-07-31T03:29:58Z

Could you add more tests for DistributionStatisticConfig set via FlagsProvider and AbstractMetricCollectingBuilder?

ikhoon · 2023-07-31T03:20:41Z

core/src/main/java/com/linecorp/armeria/common/Flags.java

+     * DistributionStatisticConfig.builder()
+     *     .percentilesHistogram(false)
+     *     .serviceLevelObjectives()
+     *     .percentiles(PERCENTILES)


Should we inline PERCENTILES to show the actual value?

ikhoon · 2023-07-31T03:21:11Z

core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java

+     * DistributionStatisticConfig.builder()
+     *     .percentilesHistogram(false)
+     *     .serviceLevelObjectives()
+     *     .percentiles(PERCENTILES)


Should we inline PERCENTILES to show the actual value?

ikhoon · 2023-07-31T03:22:58Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java

-                                       .expiry(Duration.ofMinutes(3))
-                                       .bufferLength(3)
-                                       .build();
+    private static volatile DistributionStatisticConfig distStatCfg = Flags.distributionStatisticConfig();


Suggested change

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

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

ikhoon · 2023-07-31T03:30:48Z

Please check the lint failure.

- Fix style error

ikhoon · 2023-08-10T11:45:31Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java

        requireNonNull(registry, "registry");
        requireNonNull(name, "name");
        requireNonNull(tags, "tags");
+        requireNonNull(distStatsConfig, "distStatCfg");


Suggested change

requireNonNull(distStatsConfig, "distStatCfg");

requireNonNull(distStatsConfig, "distStatsConfig");

ikhoon · 2023-08-10T11:46:12Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java

        requireNonNull(registry, "registry");
        requireNonNull(name, "name");
        requireNonNull(tags, "tags");
+        requireNonNull(distStatsConfig, "distStatCfg");


Suggested change

requireNonNull(distStatsConfig, "distStatCfg");

requireNonNull(distStatsConfig, "distStatsConfig");

ikhoon · 2023-08-10T11:54:31Z

core/src/test/java/com/linecorp/armeria/common/metric/AbstractMetricCollectingBuilderTest.java

+        assertThat(abstractMetricCollectingBuilder.distributionStatisticConfig())
+                .isEqualTo(distConfig);
+    }
+}


Should we add an EOL character?

Suggested change

}

}

jrhee17

Mostly looks good. Left two comments 🙇

jrhee17 · 2023-08-14T09:28:03Z

core/src/main/java/com/linecorp/armeria/internal/common/metric/RequestMetricSupport.java

-        DefaultClientRequestMetrics(MeterRegistry parent, MeterIdPrefix idPrefix) {
-            super(parent, idPrefix);
+        DefaultClientRequestMetrics(MeterRegistry parent, MeterIdPrefix idPrefix,
+                                    DistributionStatisticConfig distributionStatisticConfig) {


Shouldn't we also use this distributionStatisticConfig for timers/distributions in this class?

armeria/core/src/main/java/com/linecorp/armeria/internal/common/metric/RequestMetricSupport.java

Lines 316 to 323 in 337c09c

connectionAcquisitionDuration = newTimer(

parent, idPrefix.name("connection.acquisition.duration"), idPrefix.tags());

dnsResolutionDuration = newTimer(

parent, idPrefix.name("dns.resolution.duration"), idPrefix.tags());

socketConnectDuration = newTimer(

parent, idPrefix.name("socket.connect.duration"), idPrefix.tags());

pendingAcquisitionDuration = newTimer(

parent, idPrefix.name("pending.acquisition.duration"), idPrefix.tags());

armeria/core/src/main/java/com/linecorp/armeria/internal/common/metric/RequestMetricSupport.java

Lines 369 to 386 in 337c09c

public DistributionSummary successAttempts() {

if (successAttempts != null) {

return successAttempts;

}

return successAttempts = newDistributionSummary(parent,

idPrefix.name("actual.requests.attempts"),

idPrefix.tags("result", "success"));

}

@Override

public DistributionSummary failureAttempts() {

if (failureAttempts != null) {

return failureAttempts;

}

return failureAttempts = newDistributionSummary(parent,

idPrefix.name("actual.requests.attempts"),

idPrefix.tags("result", "failure"));

}

jrhee17 · 2023-08-14T09:37:44Z

core/src/main/java/com/linecorp/armeria/common/metric/AbstractMetricCollectingBuilder.java

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


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

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

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

The builder isn't final 😅

armeria/it/builders/src/test/java/com/linecorp/armeria/OverriddenBuilderMethodsReturnTypeTest.java

Line 51 in 5df7f16

Modifier.isFinal(clazz.getModifiers()))

github-actions · 2024-03-26T15:24:00Z

🔍 Build Scan® (commit: `a7388cc`)

Job name	Status	Build Scan®
build-windows-latest-jdk-19	✅	https://ge.armeria.dev/s/uwpulstfa2l62
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/pngpyohm227ia
build-self-hosted-unsafe-jdk-19-snapshot-blockhound	✅	https://ge.armeria.dev/s/ur757zjodp3o4
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/csqizcnp3yobm
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/mm4gsvw2stn3a
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/ug2uhk3tfnnim
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/6xze55mbpqhl2
build-macos-12-jdk-19	✅	https://ge.armeria.dev/s/li63ajfoe7pvm

jrhee17

👍 👍 👍

ikhoon

Nice work! 🚀🚀

echo304 added 2 commits April 15, 2023 11:09

Add distributionConfig setter and getter to AbstractMetricCollectingB…

6b103e0

…uilder

Add distributionConfig getter to FlagsProvider and give default value

76a7461

echo304 requested review from ikhoon, jrhee17, minwoox and trustin as code owners April 15, 2023 02:13

echo304 added 2 commits April 16, 2023 22:32

Add distributionConfig property and getter to Flags

524f005

Fix style error

55f0616

minwoox reviewed Apr 17, 2023

View reviewed changes

echo304 added 2 commits April 18, 2023 22:24

Reflect code review

7c26ec9

- Use MoreMeters default distribution config as default

Move default DistributionStatisticConfig to DistributionStatisticConf…

b706bec

…igUtil

ikhoon added the new feature label Apr 30, 2023

ikhoon reviewed Apr 30, 2023

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/metric/AbstractMetricCollectingBuilder.java Show resolved Hide resolved

Reflect code reviews

fd44022

- allow MetricCollecting{Client,RpcClient,Service} to receive DistributionStatisticConfig

echo304 requested review from ikhoon and minwoox May 2, 2023 00:33

trustin approved these changes May 2, 2023

View reviewed changes

trustin self-requested a review May 2, 2023 12:47

Reflect code reviews

f30bb03

- Fix comments - Reformat a file

minwoox reviewed May 9, 2023

View reviewed changes

core/src/main/java/com/linecorp/armeria/client/metric/AbstractMetricCollectingClient.java Outdated Show resolved Hide resolved

Reflect code review

e8fd096

- Override MoreMeters.distributionStatisticConfig from RequestMetricSupport.setup

echo304 commented May 12, 2023

View reviewed changes

Revert non thread-safe code

ddba25c

echo304 commented May 16, 2023

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/metric/MoreMeters.java Show resolved Hide resolved

Let AbstractRequestMetrics accept distStatCfg as argument

daff026

ikhoon reviewed May 22, 2023

View reviewed changes

echo304 added 2 commits May 22, 2023 22:36

Remove unnecessary @nullable annotation

27c59c9

Merge branch 'main' into allow-distribution-config-customization

08b756d

# Conflicts: # core/src/main/java/com/linecorp/armeria/internal/common/metric/RequestMetricSupport.java # core/src/main/java/com/linecorp/armeria/server/metric/MetricCollectingService.java

Merge branch 'main' into allow-distribution-config-customization

85788da

minwoox reviewed Jul 11, 2023

View reviewed changes

Reflect code review

b18280c

echo304 commented Jul 13, 2023

View reviewed changes

minwoox approved these changes Jul 20, 2023

View reviewed changes

Reflect code review

b8cff4a

ikhoon reviewed Jul 31, 2023

View reviewed changes

ikhoon added this to the 1.25.0 milestone Jul 31, 2023

echo304 and others added 3 commits July 31, 2023 16:09

Reflect code review

969c5d4

- Fix style error

Add test cases

755c120

Merge branch 'main' into allow-distribution-config-customization

ff7fae8

ikhoon reviewed Aug 10, 2023

View reviewed changes

Merge branch 'main' into allow-distribution-config-customization

337c09c

jrhee17 reviewed Aug 14, 2023

View reviewed changes

minwoox modified the milestones: 1.25.0, 1.26.0 Aug 18, 2023

minwoox modified the milestones: 1.26.0, 1.27.0 Oct 11, 2023

ikhoon modified the milestones: 1.27.0, 1.28.0 Jan 16, 2024

jrhee17 added 2 commits March 26, 2024 23:54

Merge branch 'main' into allow-distribution-config-customization

c664ebf

address comments by @ikhoon and @jrhee17

a7388cc

jrhee17 approved these changes Mar 28, 2024

View reviewed changes

ikhoon approved these changes Apr 2, 2024

View reviewed changes

jrhee17 merged commit 9b936c6 into line:main Apr 2, 2024
18 checks passed

	* Returns the {@link DistributionStatisticConfig} where armeria utilizes.
	* Returns the default {@link DistributionStatisticConfig} of the {@link Timer}s and {@link DistributionSummary}s created by Armeria.

	@Nullable DistributionStatisticConfig distributionStatisticConfig) {
	DistributionStatisticConfig distributionStatisticConfig) {

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

	SuccessFunction.ofDefault(), distributionStatisticConfig);
	SuccessFunction.ofDefault(), DEFAULT_DIST_STAT_CFG);

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

	requireNonNull(distStatsConfig, "distStatCfg");
	requireNonNull(distStatsConfig, "distStatsConfig");

	connectionAcquisitionDuration = newTimer(
	parent, idPrefix.name("connection.acquisition.duration"), idPrefix.tags());
	dnsResolutionDuration = newTimer(
	parent, idPrefix.name("dns.resolution.duration"), idPrefix.tags());
	socketConnectDuration = newTimer(
	parent, idPrefix.name("socket.connect.duration"), idPrefix.tags());
	pendingAcquisitionDuration = newTimer(
	parent, idPrefix.name("pending.acquisition.duration"), idPrefix.tags());

	public DistributionSummary successAttempts() {
	if (successAttempts != null) {
	return successAttempts;
	}
	return successAttempts = newDistributionSummary(parent,
	idPrefix.name("actual.requests.attempts"),
	idPrefix.tags("result", "success"));
	}

	@Override
	public DistributionSummary failureAttempts() {
	if (failureAttempts != null) {
	return failureAttempts;
	}
	return failureAttempts = newDistributionSummary(parent,
	idPrefix.name("actual.requests.attempts"),
	idPrefix.tags("result", "failure"));
	}

Allow distribution config customization #4829

Allow distribution config customization #4829

Conversation

echo304 commented Apr 15, 2023

echo304 commented Apr 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin commented May 2, 2023

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon May 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

echo304 May 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 29, 2023 • edited

Codecov Report

ikhoon commented Jul 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon commented Jul 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

jrhee17 Aug 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 26, 2024 • edited

🔍 Build Scan® (commit: a7388cc)

jrhee17 left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

ikhoon May 12, 2023 •

edited

echo304 May 16, 2023 •

edited

codecov bot commented Jul 29, 2023 •

edited

jrhee17 Aug 14, 2023 •

edited

github-actions bot commented Mar 26, 2024 •

edited

🔍 Build Scan® (commit: `a7388cc`)