line / armeria

Your go-to microservice framework for any situation, from the creator of Netty et al. You can build any type of microservice leveraging your favorite technologies, including gRPC, Thrift, Kotlin, Retrofit, Reactive Streams, Spring Boot and Dropwizard.
https://armeria.dev
Apache License 2.0
4.83k stars 919 forks source link

Review and adjust the default `DistributionStatisticConfig` #4792

Open trustin opened 1 year ago

trustin commented 1 year ago

Currently, Armeria uses the default DistributionStatisticConfig of:

DistributionStatisticConfig
  .builder()
  .percentilesHistogram(false)
  .sla()
  .percentiles(new double[] { 0, 0.5, 0.75, 0.9, 0.95, 0.98, 0.99, 0.999, 1.0 })
  .percentilePrecision(2)
  .minimumExpectedValue(1L)
  .maximumExpectedValue(Long.MAX_VALUE)
  .expiry(Duration.ofMinutes(3))
  .bufferLength(3)
  .build();

When we define the above defaults, our intention was to rotate the buffer every minute, as explained in the comments:

    /**
     * Export the percentile values only by default. We specify all properties so that we get consistent values
     * even if Micrometer changes its defaults. Most notably, we changed {@code percentilePrecision},
     * {@code expiry} and {@code bufferLength} due to the following reasons:
     * <ul>
     *   <li>The default {@code percentilePrecision} of 1 is way too inaccurate.</li>
     *   <li>Histogram buckets should be rotated every minute rather than every some-arbitrary-seconds
     *       because that fits better to human's mental model of time. Micrometer's 2 minutes / 3 buffers
     *       (i.e. rotate every 40 seconds) does not make much sense.</li>
     * </ul>
     */

However, according to TimeWindowMax.rotate(): https://github.com/micrometer-metrics/micrometer/blob/v1.7.1/micrometer-core/src/main/java/io/micrometer/core/instrument/distribution/TimeWindowMax.java#L118-L145

it seems to me that the buffers are rotated every 3 minutes.

Action item: Double check if the buffers are rotated every minute with the current defaults. Change the defaults if we were mistaken.

trustin commented 1 year ago

@anuraaga @mauhiz Do you remember anything about this when we chose the above defaults?

mauhiz commented 1 year ago

Was I involved? I always thought this logic to be weird :D

mauhiz commented 1 year ago

By the way: I think the current trend is to define a MeterFilter to configure most of these properties, not to set them directly on DistributionConfig. https://micrometer.io/docs/concepts#_configuring_distribution_statistics

imasahiro commented 1 year ago

ref: https://github.com/line/armeria/pull/1226 (let me revisit here next monday)