From 177becd516b2eff377b850d87fd09e9164bea90a Mon Sep 17 00:00:00 2001 From: Rafal Harabien Date: Mon, 1 Jul 2024 12:45:18 +0200 Subject: [PATCH] Fix percentile metrics rotating too often Before this change percentile metrics rotation interval was set to `expiry / bufferLength`. This was different from other metrics like `_max` where the interval is set to `expiry`. At the same time documentation of the `expiry` parameter says clearly that it is used as rotation interval. This inconsistent behavior was confusing for users because some metrics expired faster than the others. What's more it caused some requests to be ignored in percentile metrics if `expiry` (also called `step`) was set to the same duration as scrapping interval (e.g. 1 minute) and scrapping occurred not long after buffer rotation. In case of the default config where `bufferLength` is 3 it could result in up to 33% requests being ignored by percentile metrics. Fix this inconsistency by changing buffer rotation interval for percentiles to `expiry`. Fixes #3298 --- .../instrument/distribution/AbstractTimeWindowHistogram.java | 4 ++-- .../distribution/TimeWindowPercentileHistogramTest.java | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/micrometer-core/src/main/java/io/micrometer/core/instrument/distribution/AbstractTimeWindowHistogram.java b/micrometer-core/src/main/java/io/micrometer/core/instrument/distribution/AbstractTimeWindowHistogram.java index 40dd19561e..4e954dd99e 100644 --- a/micrometer-core/src/main/java/io/micrometer/core/instrument/distribution/AbstractTimeWindowHistogram.java +++ b/micrometer-core/src/main/java/io/micrometer/core/instrument/distribution/AbstractTimeWindowHistogram.java @@ -74,10 +74,10 @@ abstract class AbstractTimeWindowHistogram implements Histogram { ringBuffer = (T[]) Array.newInstance(bucketType, ageBuckets); - durationBetweenRotatesMillis = distributionStatisticConfig.getExpiry().toMillis() / ageBuckets; + durationBetweenRotatesMillis = distributionStatisticConfig.getExpiry().toMillis(); if (durationBetweenRotatesMillis <= 0) { rejectHistogramConfig("expiry (" + distributionStatisticConfig.getExpiry().toMillis() - + "ms) / bufferLength (" + ageBuckets + ") must be greater than 0."); + + "ms) must be greater than 0."); } currentBucket = 0; diff --git a/micrometer-core/src/test/java/io/micrometer/core/instrument/distribution/TimeWindowPercentileHistogramTest.java b/micrometer-core/src/test/java/io/micrometer/core/instrument/distribution/TimeWindowPercentileHistogramTest.java index 04764c32fd..f59fb7d0bb 100644 --- a/micrometer-core/src/test/java/io/micrometer/core/instrument/distribution/TimeWindowPercentileHistogramTest.java +++ b/micrometer-core/src/test/java/io/micrometer/core/instrument/distribution/TimeWindowPercentileHistogramTest.java @@ -162,7 +162,7 @@ private boolean percentileValueIsApproximately(ValueAtPercentile vp, double perc void timeBasedSlidingWindow() { final DistributionStatisticConfig config = DistributionStatisticConfig.builder() .percentiles(0.0, 0.5, 0.75, 0.9, 0.99, 0.999, 1.0) - .expiry(Duration.ofSeconds(4)) + .expiry(Duration.ofSeconds(1)) .bufferLength(4) .build() .merge(DistributionStatisticConfig.DEFAULT);