Skip to content

Commit

Permalink
minimumExpectedValue from distribution config is included in histog…
Browse files Browse the repository at this point in the history 
…ram (#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.
  • Loading branch information
@lenin-jaganathan
lenin-jaganathan authored Aug 21, 2024
1 parent 56bea5a commit c7d16cd
Show file tree
Hide file tree
Showing 6 changed files with 59 additions and 41 deletions.
26 changes: 19 additions & 7 deletions ...ry-otlp/src/main/java/io/micrometer/registry/otlp/internal/Base2ExponentialHistogram.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 4 additions & 6 deletions ...istry-otlp/src/test/java/io/micrometer/registry/otlp/OtlpCumulativeMeterRegistryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -564,15 +564,14 @@ void testExponentialHistogramWithTimer() {
.tags(Tags.of(meterTag))
.publishPercentileHistogram()
.register(registryWithExponentialHistogram);
timer.record(Duration.ofMillis(1));
timer.record(Duration.ofMillis(100));
timer.record(Duration.ofMillis(1000));

Metric metric = writeToMetric(timer);
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);
Expand All @@ -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);
Expand All @@ -608,15 +607,14 @@ void testExponentialHistogramDs() {
.tags(Tags.of(meterTag))
.publishPercentileHistogram()
.register(registryWithExponentialHistogram);
ds.record(1);
ds.record(100);
ds.record(1000);

Metric metric = writeToMetric(ds);
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);
Expand All @@ -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);
Expand Down
6 changes: 2 additions & 4 deletions ...r-registry-otlp/src/test/java/io/micrometer/registry/otlp/OtlpDeltaMeterRegistryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));

Expand All @@ -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);
Expand Down Expand Up @@ -567,7 +566,6 @@ void testExponentialHistogramDs() {
.tags(Tags.of(meterTag))
.publishPercentileHistogram()
.register(registryWithExponentialHistogram);
ds.record(1);
ds.record(100);
ds.record(1000);

Expand All @@ -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);
Expand Down
9 changes: 4 additions & 5 deletions ...ometer-registry-otlp/src/test/java/io/micrometer/registry/otlp/OtlpMeterRegistryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
47 changes: 29 additions & 18 deletions ...tlp/src/test/java/io/micrometer/registry/otlp/internal/Base2ExponentialHistogramTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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();
}
Expand Down
2 changes: 1 addition & 1 deletion ...rc/test/java/io/micrometer/registry/otlp/internal/DeltaBase2ExponentialHistogramTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ void setUp() {

@Test
void snapshotShouldBeSameForOneStep() {
deltaBase2ExponentialHistogram.recordDouble(1.0);
deltaBase2ExponentialHistogram.recordDouble(0.5);
deltaBase2ExponentialHistogram.recordDouble(2.0);

ExponentialHistogramSnapShot exponentialHistogramSnapShot = deltaBase2ExponentialHistogram
Expand Down

0 comments on commit c7d16cd

Please sign in to comment.