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.

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

Allow distribution config customization #4829

Merged
merged 22 commits into from
Apr 2, 2024
Merged

Allow distribution config customization #4829

Show file tree
Hide file tree
Changes from 4 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
6 changes: 6 additions & 0 deletions core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java
View file
Open in desktop
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 @@ -417,4 +418,9 @@ public MeterRegistry meterRegistry() {
public Long defaultUnhandledExceptionsReportIntervalMillis() {
return DEFAULT_UNHANDLED_EXCEPTIONS_REPORT_INTERVAL_MILLIS;
}

@Override
public DistributionStatisticConfig distributionStatisticConfig() {
return DistributionStatisticConfig.DEFAULT;
Copy link
Member

Choose a reason for hiding this comment

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

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. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.
스크린샷 2023-04-21 오후 1 22 26

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

}
}
14 changes: 14 additions & 0 deletions core/src/main/java/com/linecorp/armeria/common/Flags.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@

import io.micrometer.core.instrument.MeterRegistry;
import io.micrometer.core.instrument.Metrics;
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 +387,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 +1347,16 @@ public static long defaultUnhandledExceptionsReportIntervalMillis() {
return DEFAULT_UNHANDLED_EXCEPTIONS_REPORT_INTERVAL_MILLIS;
}

/**
* Returns the {@link DistributionStatisticConfig} where armeria utilizes.
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
* Returns the {@link DistributionStatisticConfig} where armeria utilizes.
* 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 {@link DistributionStatisticConfig#DEFAULT}</p>
Copy link
Member

Choose a reason for hiding this comment

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

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>

*/
@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
12 changes: 12 additions & 0 deletions core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java
View file
Open in desktop
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,15 @@ default MeterRegistry meterRegistry() {
default Long defaultUnhandledExceptionsReportIntervalMillis() {
return null;
}

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

Choose a reason for hiding this comment

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

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

@Nullable
@UnstableApi
default DistributionStatisticConfig distributionStatisticConfig() {
return null;
}
}
23 changes: 23 additions & 0 deletions core/src/main/java/com/linecorp/armeria/common/metric/AbstractMetricCollectingBuilder.java
View file
Open in desktop
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;

/**
* 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 😅

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

Line 51 in 5df7f16

Modifier.isFinal(clazz.getModifiers()))

DistributionStatisticConfig distributionStatisticConfig) {
this.distributionStatisticConfig = requireNonNull(distributionStatisticConfig,
"distributionStatisticConfig");
return this;
}
}