-
Notifications
You must be signed in to change notification settings - Fork 896
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
Changes from 15 commits
6b103e0
76a7461
524f005
55f0616
7c26ec9
b706bec
fd44022
f30bb03
e8fd096
ddba25c
daff026
27c59c9
08b756d
85788da
b18280c
b8cff4a
969c5d4
755c120
ff7fae8
337c09c
c664ebf
a7388cc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -388,6 +391,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 | ||
|
@@ -1378,6 +1384,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) | ||
* .serviceLevelObjectives() | ||
* .percentiles(PERCENTILES) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we inline |
||
* .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); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -1033,4 +1034,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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we inline |
||
* .percentilePrecision(2) | ||
* .minimumExpectedValue(1L) | ||
* .maximumExpectedValue(Long.MAX_VALUE) | ||
* .expiry(Duration.ofMinutes(3)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
---|---|---|---|---|
|
@@ -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. | ||||
*/ | ||||
|
@@ -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}. | ||||
*/ | ||||
|
@@ -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( | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't our builder test ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The builder isn't final 😅 armeria/it/builders/src/test/java/com/linecorp/armeria/OverriddenBuilderMethodsReturnTypeTest.java Line 51 in 5df7f16
|
||||
DistributionStatisticConfig distributionStatisticConfig) { | ||||
this.distributionStatisticConfig = requireNonNull(distributionStatisticConfig, | ||||
"distributionStatisticConfig"); | ||||
return this; | ||||
} | ||||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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; | ||||||
|
@@ -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 { | ||||||
|
@@ -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(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
/** | ||||||
* Sets the {@link DistributionStatisticConfig} to use when the factory methods in {@link MoreMeters} create | ||||||
|
@@ -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 distStatsConfig the {@link DistributionStatisticConfig} to use | ||||||
*/ | ||||||
public static DistributionSummary newDistributionSummary(MeterRegistry registry, | ||||||
String name, Iterable<Tag> tags, | ||||||
DistributionStatisticConfig distStatsConfig) { | ||||||
requireNonNull(registry, "registry"); | ||||||
requireNonNull(name, "name"); | ||||||
requireNonNull(tags, "tags"); | ||||||
requireNonNull(distStatsConfig, "distStatCfg"); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
final Builder builder = | ||||||
DistributionSummary.builder(name) | ||||||
.tags(tags) | ||||||
.publishPercentiles(distStatCfg.getPercentiles()) | ||||||
.publishPercentileHistogram(distStatCfg.isPercentileHistogram()) | ||||||
.percentilePrecision(distStatCfg.getPercentilePrecision()) | ||||||
.distributionStatisticBufferLength(distStatCfg.getBufferLength()) | ||||||
.distributionStatisticExpiry(distStatCfg.getExpiry()); | ||||||
.publishPercentiles(distStatsConfig.getPercentiles()) | ||||||
.publishPercentileHistogram(distStatsConfig.isPercentileHistogram()) | ||||||
.percentilePrecision(distStatsConfig.getPercentilePrecision()) | ||||||
.distributionStatisticBufferLength(distStatsConfig.getBufferLength()) | ||||||
.distributionStatisticExpiry(distStatsConfig.getExpiry()); | ||||||
|
||||||
if (MICROMETER_1_5) { | ||||||
builder.maximumExpectedValue(distStatCfg.getMaximumExpectedValueAsDouble()) | ||||||
.minimumExpectedValue(distStatCfg.getMinimumExpectedValueAsDouble()) | ||||||
.serviceLevelObjectives(distStatCfg.getServiceLevelObjectiveBoundaries()); | ||||||
builder.maximumExpectedValue(distStatsConfig.getMaximumExpectedValueAsDouble()) | ||||||
.minimumExpectedValue(distStatsConfig.getMinimumExpectedValueAsDouble()) | ||||||
.serviceLevelObjectives(distStatsConfig.getServiceLevelObjectiveBoundaries()); | ||||||
} else { | ||||||
final Double maxExpectedValueNanos = distStatCfg.getMaximumExpectedValueAsDouble(); | ||||||
final Double minExpectedValueNanos = distStatCfg.getMinimumExpectedValueAsDouble(); | ||||||
final Double maxExpectedValueNanos = distStatsConfig.getMaximumExpectedValueAsDouble(); | ||||||
final Double minExpectedValueNanos = distStatsConfig.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 = distStatsConfig.getServiceLevelObjectiveBoundaries(); | ||||||
if (slas != null) { | ||||||
builder.sla(Arrays.stream(slas).mapToLong(sla -> (long) sla).toArray()); | ||||||
} | ||||||
|
@@ -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 distStatsConfig 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 distStatsConfig) { | ||||||
requireNonNull(registry, "registry"); | ||||||
requireNonNull(name, "name"); | ||||||
requireNonNull(tags, "tags"); | ||||||
requireNonNull(distStatsConfig, "distStatCfg"); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
final Double maxExpectedValueNanos = distStatCfg.getMaximumExpectedValueAsDouble(); | ||||||
final Double minExpectedValueNanos = distStatCfg.getMinimumExpectedValueAsDouble(); | ||||||
final Double maxExpectedValueNanos = distStatsConfig.getMaximumExpectedValueAsDouble(); | ||||||
final Double minExpectedValueNanos = distStatsConfig.getMinimumExpectedValueAsDouble(); | ||||||
final Duration maxExpectedValue = | ||||||
maxExpectedValueNanos != null ? Duration.ofNanos(maxExpectedValueNanos.longValue()) : null; | ||||||
final Duration minExpectedValue = | ||||||
|
@@ -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(distStatsConfig.getPercentiles()) | ||||||
.publishPercentileHistogram(distStatsConfig.isPercentileHistogram()) | ||||||
.percentilePrecision(distStatsConfig.getPercentilePrecision()) | ||||||
.distributionStatisticBufferLength(distStatsConfig.getBufferLength()) | ||||||
.distributionStatisticExpiry(distStatsConfig.getExpiry()) | ||||||
.register(registry); | ||||||
} | ||||||
|
||||||
|
There was a problem hiding this comment.
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.