From c7d16cd332dfb9046d205195293f1ccea0589b8b Mon Sep 17 00:00:00 2001 From: Lenin Jaganathan <32874349+lenin-jaganathan@users.noreply.github.com> Date: Wed, 21 Aug 2024 12:50:46 +0530 Subject: [PATCH] `minimumExpectedValue` from distribution config is included in histogram (#5394) Prior to this change, recording the exact minimumExpectedValue would result in it being included in the zero count for the histogram rather than the positive buckets. --- .../internal/Base2ExponentialHistogram.java | 26 +++++++--- .../otlp/OtlpCumulativeMeterRegistryTest.java | 10 ++-- .../otlp/OtlpDeltaMeterRegistryTest.java | 6 +-- .../registry/otlp/OtlpMeterRegistryTest.java | 9 ++-- .../Base2ExponentialHistogramTest.java | 47 ++++++++++++------- .../DeltaBase2ExponentialHistogramTest.java | 2 +- 6 files changed, 59 insertions(+), 41 deletions(-) diff --git a/implementations/micrometer-registry-otlp/src/main/java/io/micrometer/registry/otlp/internal/Base2ExponentialHistogram.java b/implementations/micrometer-registry-otlp/src/main/java/io/micrometer/registry/otlp/internal/Base2ExponentialHistogram.java index dd4efb33e2..8f58b3c794 100644 --- a/implementations/micrometer-registry-otlp/src/main/java/io/micrometer/registry/otlp/internal/Base2ExponentialHistogram.java +++ b/implementations/micrometer-registry-otlp/src/main/java/io/micrometer/registry/otlp/internal/Base2ExponentialHistogram.java @@ -73,25 +73,37 @@ public abstract class Base2ExponentialHistogram implements Histogram { * maxBucketsCount. * @param maxBucketsCount - maximum number of buckets that can be used for * distribution. - * @param zeroThreshold - values less than or equal to this are considered in zero - * count and recorded in the histogram. If less than 0, this is rounded to zero. In - * case of recording time, this should be in nanoseconds. + * @param minimumExpectedValue - values less than this are considered in zero count + * and not recorded in the histogram. If less than 0, this is rounded to zero. In case + * of recording time, this should be in nanoseconds. * @param baseUnit - an Optional TimeUnit. If set to a non-null unit, the recorded * values are converted to this unit. */ - Base2ExponentialHistogram(int maxScale, int maxBucketsCount, double zeroThreshold, @Nullable TimeUnit baseUnit) { + Base2ExponentialHistogram(int maxScale, int maxBucketsCount, double minimumExpectedValue, + @Nullable TimeUnit baseUnit) { this.maxScale = maxScale; this.scale = maxScale; this.maxBucketsCount = maxBucketsCount; this.baseUnit = baseUnit; - // Convert the zeroThreshold to baseUnit. - this.zeroThreshold = Math.max(baseUnit != null ? TimeUtils.nanosToUnit(zeroThreshold, baseUnit) : zeroThreshold, - 0.0); + this.zeroThreshold = getZeroThreshHoldFromMinExpectedValue(minimumExpectedValue, baseUnit); this.circularCountHolder = new CircularCountHolder(maxBucketsCount); this.base2IndexProvider = IndexProviderFactory.getIndexProviderForScale(scale); } + /** + * Convert the minimumExpectedValue to zeroThreshold. Micrometer's + * minimumExpectedValue should be included as part of distribution whereas Exponential + * Histogram will exclude the zeroThreshold from distribution. Hence, we find the next + * smallest value from minimumExpectedValue and use it as zeroThreshold. + */ + private static double getZeroThreshHoldFromMinExpectedValue(final double minimumExpectedValue, + final @Nullable TimeUnit baseUnit) { + double minValueScaledToTime = baseUnit != null ? TimeUtils.nanosToUnit(minimumExpectedValue, baseUnit) + : minimumExpectedValue; + return Math.max(Math.nextDown(minValueScaledToTime), 0.0); + } + /** * Returns the latest snapshot of recordings from * {@link Base2ExponentialHistogram#takeExponentialHistogramSnapShot()} and not the diff --git a/implementations/micrometer-registry-otlp/src/test/java/io/micrometer/registry/otlp/OtlpCumulativeMeterRegistryTest.java b/implementations/micrometer-registry-otlp/src/test/java/io/micrometer/registry/otlp/OtlpCumulativeMeterRegistryTest.java index a1cedb14bd..799088da81 100644 --- a/implementations/micrometer-registry-otlp/src/test/java/io/micrometer/registry/otlp/OtlpCumulativeMeterRegistryTest.java +++ b/implementations/micrometer-registry-otlp/src/test/java/io/micrometer/registry/otlp/OtlpCumulativeMeterRegistryTest.java @@ -564,7 +564,6 @@ void testExponentialHistogramWithTimer() { .tags(Tags.of(meterTag)) .publishPercentileHistogram() .register(registryWithExponentialHistogram); - timer.record(Duration.ofMillis(1)); timer.record(Duration.ofMillis(100)); timer.record(Duration.ofMillis(1000)); @@ -572,7 +571,7 @@ void testExponentialHistogramWithTimer() { assertThat(metric.getExponentialHistogram().getDataPointsCount()).isPositive(); ExponentialHistogramDataPoint exponentialHistogramDataPoint = metric.getExponentialHistogram().getDataPoints(0); - assertExponentialHistogram(metric, 3, 1101, 0.0, 1, 5); + assertExponentialHistogram(metric, 2, 1100, 0.0, 0, 5); ExponentialHistogramDataPoint.Buckets buckets = exponentialHistogramDataPoint.getPositive(); assertThat(buckets.getOffset()).isEqualTo(212); assertThat(buckets.getBucketCountsCount()).isEqualTo(107); @@ -590,7 +589,7 @@ void testExponentialHistogramWithTimer() { assertThat(exponentialHistogramDataPoint.getTimeUnixNano() - previousEndTime) .isEqualTo(otlpConfig().step().toNanos()); - assertExponentialHistogram(metric, 4, 11101, 0.0, 1, 4); + assertExponentialHistogram(metric, 3, 11100, 0.0, 0, 4); buckets = exponentialHistogramDataPoint.getPositive(); assertThat(buckets.getOffset()).isEqualTo(106); @@ -608,7 +607,6 @@ void testExponentialHistogramDs() { .tags(Tags.of(meterTag)) .publishPercentileHistogram() .register(registryWithExponentialHistogram); - ds.record(1); ds.record(100); ds.record(1000); @@ -616,7 +614,7 @@ void testExponentialHistogramDs() { assertThat(metric.getExponentialHistogram().getDataPointsCount()).isPositive(); ExponentialHistogramDataPoint exponentialHistogramDataPoint = metric.getExponentialHistogram().getDataPoints(0); - assertExponentialHistogram(metric, 3, 1101, 0.0, 1, 5); + assertExponentialHistogram(metric, 2, 1100, 0.0, 0, 5); ExponentialHistogramDataPoint.Buckets buckets = exponentialHistogramDataPoint.getPositive(); assertThat(buckets.getOffset()).isEqualTo(212); assertThat(buckets.getBucketCountsCount()).isEqualTo(107); @@ -634,7 +632,7 @@ void testExponentialHistogramDs() { assertThat(exponentialHistogramDataPoint.getTimeUnixNano() - previousEndTime) .isEqualTo(otlpConfig().step().toNanos()); - assertExponentialHistogram(metric, 4, 11101, 0.0, 1, 4); + assertExponentialHistogram(metric, 3, 11100, 0.0, 0, 4); buckets = exponentialHistogramDataPoint.getPositive(); assertThat(buckets.getOffset()).isEqualTo(106); diff --git a/implementations/micrometer-registry-otlp/src/test/java/io/micrometer/registry/otlp/OtlpDeltaMeterRegistryTest.java b/implementations/micrometer-registry-otlp/src/test/java/io/micrometer/registry/otlp/OtlpDeltaMeterRegistryTest.java index 992f9f235b..a42098a339 100644 --- a/implementations/micrometer-registry-otlp/src/test/java/io/micrometer/registry/otlp/OtlpDeltaMeterRegistryTest.java +++ b/implementations/micrometer-registry-otlp/src/test/java/io/micrometer/registry/otlp/OtlpDeltaMeterRegistryTest.java @@ -513,7 +513,6 @@ void testExponentialHistogramWithTimer() { .tags(Tags.of(meterTag)) .publishPercentileHistogram() .register(registryWithExponentialHistogram); - timer.record(Duration.ofMillis(1)); timer.record(Duration.ofMillis(100)); timer.record(Duration.ofMillis(1000)); @@ -524,7 +523,7 @@ void testExponentialHistogramWithTimer() { Metric metric = writeToMetric(timer); assertThat(metric.getExponentialHistogram().getDataPointsCount()).isPositive(); ExponentialHistogramDataPoint exponentialHistogramDataPoint = metric.getExponentialHistogram().getDataPoints(0); - assertExponentialHistogram(metric, 3, 1101, 1000.0, 1, 5); + assertExponentialHistogram(metric, 2, 1100, 1000.0, 0, 5); ExponentialHistogramDataPoint.Buckets buckets = exponentialHistogramDataPoint.getPositive(); assertThat(buckets.getOffset()).isEqualTo(212); assertThat(buckets.getBucketCountsCount()).isEqualTo(107); @@ -567,7 +566,6 @@ void testExponentialHistogramDs() { .tags(Tags.of(meterTag)) .publishPercentileHistogram() .register(registryWithExponentialHistogram); - ds.record(1); ds.record(100); ds.record(1000); @@ -578,7 +576,7 @@ void testExponentialHistogramDs() { Metric metric = writeToMetric(ds); assertThat(metric.getExponentialHistogram().getDataPointsCount()).isPositive(); ExponentialHistogramDataPoint exponentialHistogramDataPoint = metric.getExponentialHistogram().getDataPoints(0); - assertExponentialHistogram(metric, 3, 1101, 1000.0, 1, 5); + assertExponentialHistogram(metric, 2, 1100, 1000.0, 0, 5); ExponentialHistogramDataPoint.Buckets buckets = exponentialHistogramDataPoint.getPositive(); assertThat(buckets.getOffset()).isEqualTo(212); assertThat(buckets.getBucketCountsCount()).isEqualTo(107); diff --git a/implementations/micrometer-registry-otlp/src/test/java/io/micrometer/registry/otlp/OtlpMeterRegistryTest.java b/implementations/micrometer-registry-otlp/src/test/java/io/micrometer/registry/otlp/OtlpMeterRegistryTest.java index 16e739fab8..e86e67d6c2 100644 --- a/implementations/micrometer-registry-otlp/src/test/java/io/micrometer/registry/otlp/OtlpMeterRegistryTest.java +++ b/implementations/micrometer-registry-otlp/src/test/java/io/micrometer/registry/otlp/OtlpMeterRegistryTest.java @@ -308,15 +308,14 @@ void testZeroCountForExponentialHistogram() { ExponentialHistogramDataPoint dataPoint = writeToMetric(timerWithZero1ms).getExponentialHistogram() .getDataPoints(0); - assertThat(dataPoint.getZeroCount()).isEqualTo(2); + assertThat(dataPoint.getZeroCount()).isEqualTo(1); assertThat(dataPoint.getCount()).isEqualTo(2); - assertThat(dataPoint.getPositive().getBucketCountsCount()).isZero(); + assertThat(dataPoint.getPositive().getBucketCountsCount()).isEqualTo(1); dataPoint = writeToMetric(timerWithZero1ns).getExponentialHistogram().getDataPoints(0); - assertThat(dataPoint.getZeroCount()).isEqualTo(1); + assertThat(dataPoint.getZeroCount()).isZero(); assertThat(dataPoint.getCount()).isEqualTo(2); - assertThat(dataPoint.getPositive().getBucketCountsCount()).isEqualTo(1); - assertThat(dataPoint.getPositive().getBucketCountsList()).isEqualTo(List.of(1L)); + assertThat(dataPoint.getPositive().getBucketCountsCount()).isGreaterThan(1); } @Test diff --git a/implementations/micrometer-registry-otlp/src/test/java/io/micrometer/registry/otlp/internal/Base2ExponentialHistogramTest.java b/implementations/micrometer-registry-otlp/src/test/java/io/micrometer/registry/otlp/internal/Base2ExponentialHistogramTest.java index 6d4899ebd9..53b65ef3c4 100644 --- a/implementations/micrometer-registry-otlp/src/test/java/io/micrometer/registry/otlp/internal/Base2ExponentialHistogramTest.java +++ b/implementations/micrometer-registry-otlp/src/test/java/io/micrometer/registry/otlp/internal/Base2ExponentialHistogramTest.java @@ -67,10 +67,9 @@ void testRecordTimeBased() { // recordDouble(2). ExponentialHistogramSnapShot currentSnapshot = base2ExponentialHistogram.getCurrentValuesSnapshot(); - assertThat(currentSnapshot.zeroCount()).isEqualTo(1); - assertThat(currentSnapshot.scale()).isEqualTo(MAX_SCALE); - assertThat(getAllBucketsCountSum(currentSnapshot)).isEqualTo(1); - assertThat(base2ExponentialHistogram.getCurrentValuesSnapshot().positive().offset()).isEqualTo(1023); + assertThat(currentSnapshot.zeroCount()).isZero(); + assertThat(currentSnapshot.scale()).isLessThan(MAX_SCALE); + assertThat(getAllBucketsCountSum(currentSnapshot)).isEqualTo(2); } @Test @@ -86,17 +85,14 @@ void testRecordTimeBasedInSeconds() { base2ExponentialHistogram.recordLong(Duration.ofMillis(50).toNanos()); ExponentialHistogramSnapShot currentSnapshot = base2ExponentialHistogram.getCurrentValuesSnapshot(); - assertThat(currentSnapshot.zeroCount()).isEqualTo(1); - assertThat(currentSnapshot.scale()).isEqualTo(MAX_SCALE); - assertThat(getAllBucketsCountSum(currentSnapshot)).isEqualTo(1); - assertThat(base2ExponentialHistogram.getCurrentValuesSnapshot().positive().offset()).isEqualTo(-4426); + assertThat(currentSnapshot.zeroCount()).isZero(); + assertThat(currentSnapshot.scale()).isLessThan(MAX_SCALE); + assertThat(getAllBucketsCountSum(currentSnapshot)).isEqualTo(2); base2ExponentialHistogram.recordLong(Duration.ofMillis(90).toNanos()); currentSnapshot = base2ExponentialHistogram.getCurrentValuesSnapshot(); - assertThat(currentSnapshot.zeroCount()).isEqualTo(1); - assertThat(currentSnapshot.scale()).isEqualTo(4); - assertThat(getAllBucketsCountSum(currentSnapshot)).isEqualTo(2); - assertThat(base2ExponentialHistogram.getCurrentValuesSnapshot().positive().offset()).isEqualTo(-70); + assertThat(currentSnapshot.scale()).isEqualTo(1); + assertThat(getAllBucketsCountSum(currentSnapshot)).isEqualTo(3); } @Test @@ -106,9 +102,22 @@ void testZeroThreshHold() { base2ExponentialHistogram.recordDouble(0.5); ExponentialHistogramSnapShot currentSnapshot = base2ExponentialHistogram.getCurrentValuesSnapshot(); - assertThat(currentSnapshot.zeroCount()).isEqualTo(3); + assertThat(currentSnapshot.zeroThreshold()).isLessThan(1).isGreaterThan(0); + assertThat(currentSnapshot.zeroCount()).isEqualTo(2); assertThat(currentSnapshot.scale()).isEqualTo(MAX_SCALE); - assertThat(getAllBucketsCountSum(currentSnapshot)).isZero(); + assertThat(getAllBucketsCountSum(currentSnapshot)).isEqualTo(1); + + Base2ExponentialHistogram base2ExponentialHistogramWithZeroAsMin = new CumulativeBase2ExponentialHistogram( + MAX_SCALE, MAX_BUCKETS_COUNT, 0.0, null); + base2ExponentialHistogramWithZeroAsMin.recordDouble(0.0); + base2ExponentialHistogramWithZeroAsMin.recordDouble(Math.nextUp(0.0)); + + ExponentialHistogramSnapShot snapshotWithZeroAsMin = base2ExponentialHistogramWithZeroAsMin + .getCurrentValuesSnapshot(); + assertThat(snapshotWithZeroAsMin.zeroThreshold()).isEqualTo(0.0); + assertThat(snapshotWithZeroAsMin.zeroCount()).isEqualTo(1); + assertThat(snapshotWithZeroAsMin.scale()).isEqualTo(MAX_SCALE); + assertThat(getAllBucketsCountSum(snapshotWithZeroAsMin)).isEqualTo(1); } @Test @@ -229,19 +238,21 @@ void testUpscaleForNegativeScale() { @Test void reset() { + base2ExponentialHistogram.recordDouble(Math.nextDown(1.0)); base2ExponentialHistogram.recordDouble(1); base2ExponentialHistogram.recordDouble(2); ExponentialHistogramSnapShot currentSnapshot = base2ExponentialHistogram.getCurrentValuesSnapshot(); + final int intialScale = currentSnapshot.scale(); assertThat(currentSnapshot.zeroCount()).isEqualTo(1); - assertThat(currentSnapshot.scale()).isEqualTo(MAX_SCALE); - assertThat(currentSnapshot.positive().offset()).isEqualTo(1023); - assertThat(getAllBucketsCountSum(currentSnapshot)).isEqualTo(1); + assertThat(currentSnapshot.scale()).isLessThan(MAX_SCALE); + assertThat(currentSnapshot.positive().offset()).isNotZero(); + assertThat(getAllBucketsCountSum(currentSnapshot)).isEqualTo(2); base2ExponentialHistogram.reset(); currentSnapshot = base2ExponentialHistogram.getCurrentValuesSnapshot(); assertThat(currentSnapshot.zeroCount()).isZero(); - assertThat(currentSnapshot.scale()).isEqualTo(MAX_SCALE); + assertThat(currentSnapshot.scale()).isEqualTo(intialScale); assertThat(currentSnapshot.positive().offset()).isZero(); assertThat(getAllBucketsCountSum(currentSnapshot)).isZero(); } diff --git a/implementations/micrometer-registry-otlp/src/test/java/io/micrometer/registry/otlp/internal/DeltaBase2ExponentialHistogramTest.java b/implementations/micrometer-registry-otlp/src/test/java/io/micrometer/registry/otlp/internal/DeltaBase2ExponentialHistogramTest.java index 2855a1e8e1..9a90ed0028 100644 --- a/implementations/micrometer-registry-otlp/src/test/java/io/micrometer/registry/otlp/internal/DeltaBase2ExponentialHistogramTest.java +++ b/implementations/micrometer-registry-otlp/src/test/java/io/micrometer/registry/otlp/internal/DeltaBase2ExponentialHistogramTest.java @@ -42,7 +42,7 @@ void setUp() { @Test void snapshotShouldBeSameForOneStep() { - deltaBase2ExponentialHistogram.recordDouble(1.0); + deltaBase2ExponentialHistogram.recordDouble(0.5); deltaBase2ExponentialHistogram.recordDouble(2.0); ExponentialHistogramSnapShot exponentialHistogramSnapShot = deltaBase2ExponentialHistogram