Skip to content

Commit 08c9e43

Browse files
lenin-jaganathanjonatan-ivanov
lenin-jaganathan
authored and
jonatan-ivanov
committed
Polish StepBucketHistogram
* eliminate null check on noValue * to support exporting cumulative buckets
1 parent eb54740 commit 08c9e43

File tree

7 files changed

+125
-69
lines changed

7 files changed

+125
-69
lines changed

‎implementations/micrometer-registry-otlp/src/main/java/io/micrometer/registry/otlp/OtlpMeterRegistry.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -413,7 +413,7 @@ static io.micrometer.core.instrument.distribution.Histogram getHistogram(Clock c
413413
.merge(distributionStatisticConfig), true, false);
414414
}
415415
else if (AggregationTemporality.isDelta(aggregationTemporality) && stepMillis > 0) {
416-
return new StepBucketHistogram(clock, stepMillis, distributionStatisticConfig, true);
416+
return new StepBucketHistogram(clock, stepMillis, distributionStatisticConfig, true, false);
417417
}
418418
}
419419

‎implementations/micrometer-registry-otlp/src/test/java/io/micrometer/registry/otlp/OtlpDeltaMeterRegistryTest.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ void timerWithHistogram() {
151151
timer.record(111, TimeUnit.MILLISECONDS);
152152

153153
HistogramDataPoint histogramDataPoint = writeToMetric(timer).getHistogram().getDataPoints(0);
154-
assertThat(histogramDataPoint.getExplicitBoundsCount()).isZero();
154+
assertThat(histogramDataPoint.getExplicitBoundsCount()).isEqualTo(4);
155155
this.stepOverNStep(1);
156156
assertHistogram(writeToMetric(timer), TimeUnit.MINUTES.toNanos(1), TimeUnit.MINUTES.toNanos(2), "milliseconds",
157157
3, 198, 111);
@@ -243,7 +243,7 @@ void distributionSummaryWithHistogram() {
243243
assertHistogram(writeToMetric(ds), 0, TimeUnit.MINUTES.toNanos(1), "bytes", 0, 0, 0);
244244

245245
HistogramDataPoint histogramDataPoint = writeToMetric(ds).getHistogram().getDataPoints(0);
246-
assertThat(histogramDataPoint.getExplicitBoundsCount()).isZero();
246+
assertThat(histogramDataPoint.getExplicitBoundsCount()).isEqualTo(4);
247247
this.stepOverNStep(1);
248248
assertHistogram(writeToMetric(ds), TimeUnit.MINUTES.toNanos(1), TimeUnit.MINUTES.toNanos(2), "bytes", 3, 198,
249249
111);

‎micrometer-core/src/main/java/io/micrometer/core/instrument/distribution/FixedBoundaryHistogram.java

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
package io.micrometer.core.instrument.distribution;
1717

1818
import java.util.Arrays;
19+
import java.util.Iterator;
1920
import java.util.concurrent.atomic.AtomicLongArray;
2021

2122
class FixedBoundaryHistogram {
@@ -24,9 +25,12 @@ class FixedBoundaryHistogram {
2425

2526
private final double[] buckets;
2627

27-
FixedBoundaryHistogram(double[] buckets) {
28+
private final boolean isCumulativeBucketCounts;
29+
30+
FixedBoundaryHistogram(double[] buckets, boolean isCumulativeBucketCounts) {
2831
this.buckets = buckets;
2932
this.values = new AtomicLongArray(buckets.length);
33+
this.isCumulativeBucketCounts = isCumulativeBucketCounts;
3034
}
3135

3236
long countAtValue(double value) {
@@ -68,4 +72,28 @@ else if (buckets[mid] > key)
6872
return low < buckets.length ? low : -1;
6973
}
7074

75+
Iterator<CountAtBucket> countsAtValues(Iterator<Double> values) {
76+
return new Iterator<CountAtBucket>() {
77+
private double cumulativeCount = 0.0;
78+
79+
@Override
80+
public boolean hasNext() {
81+
return values.hasNext();
82+
}
83+
84+
@Override
85+
public CountAtBucket next() {
86+
double value = values.next();
87+
double count = countAtValue(value);
88+
if (isCumulativeBucketCounts) {
89+
cumulativeCount += count;
90+
return new CountAtBucket(value, cumulativeCount);
91+
}
92+
else {
93+
return new CountAtBucket(value, count);
94+
}
95+
}
96+
};
97+
}
98+
7199
}

‎micrometer-core/src/main/java/io/micrometer/core/instrument/distribution/StepBucketHistogram.java

Lines changed: 29 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@
1919
import io.micrometer.core.instrument.config.InvalidConfigurationException;
2020
import io.micrometer.core.instrument.step.StepValue;
2121

22+
import java.util.Arrays;
23+
import java.util.Iterator;
2224
import java.util.NavigableSet;
2325
import java.util.Objects;
2426
import java.util.function.Supplier;
@@ -32,26 +34,18 @@
3234
*/
3335
public class StepBucketHistogram extends StepValue<CountAtBucket[]> implements Histogram {
3436

35-
private static final CountAtBucket[] EMPTY_COUNTS = new CountAtBucket[0];
36-
3737
private final FixedBoundaryHistogram fixedBoundaryHistogram;
3838

3939
private final double[] buckets;
4040

4141
public StepBucketHistogram(Clock clock, long stepMillis, DistributionStatisticConfig distributionStatisticConfig,
42-
boolean supportsAggregablePercentiles) {
43-
super(clock, stepMillis);
42+
boolean supportsAggregablePercentiles, boolean isCumulativeBucketCounts) {
43+
super(clock, stepMillis, getEmptyCounts(
44+
getBucketsFromDistributionStatisticConfig(distributionStatisticConfig, supportsAggregablePercentiles)));
4445

45-
if (distributionStatisticConfig.getMaximumExpectedValueAsDouble() == null
46-
|| distributionStatisticConfig.getMinimumExpectedValueAsDouble() == null) {
47-
throw new InvalidConfigurationException(
48-
"minimumExpectedValue and maximumExpectedValue should be greater than 0.");
49-
}
50-
NavigableSet<Double> histogramBuckets = distributionStatisticConfig
51-
.getHistogramBuckets(supportsAggregablePercentiles);
52-
53-
this.buckets = histogramBuckets.stream().filter(Objects::nonNull).mapToDouble(Double::doubleValue).toArray();
54-
this.fixedBoundaryHistogram = new FixedBoundaryHistogram(buckets);
46+
this.buckets = getBucketsFromDistributionStatisticConfig(distributionStatisticConfig,
47+
supportsAggregablePercentiles);
48+
this.fixedBoundaryHistogram = new FixedBoundaryHistogram(buckets, isCumulativeBucketCounts);
5549
}
5650

5751
@Override
@@ -74,8 +68,10 @@ protected Supplier<CountAtBucket[]> valueSupplier() {
7468
return () -> {
7569
CountAtBucket[] countAtBuckets = new CountAtBucket[buckets.length];
7670
synchronized (fixedBoundaryHistogram) {
77-
for (int i = 0; i < buckets.length; i++) {
78-
countAtBuckets[i] = new CountAtBucket(buckets[i], fixedBoundaryHistogram.countAtValue(buckets[i]));
71+
final Iterator<CountAtBucket> iterator = fixedBoundaryHistogram
72+
.countsAtValues(Arrays.stream(buckets).iterator());
73+
for (int i = 0; i < countAtBuckets.length; i++) {
74+
countAtBuckets[i] = iterator.next();
7975
}
8076
fixedBoundaryHistogram.reset();
8177
}
@@ -85,13 +81,28 @@ protected Supplier<CountAtBucket[]> valueSupplier() {
8581

8682
@Override
8783
protected CountAtBucket[] noValue() {
88-
if (buckets == null)
89-
return EMPTY_COUNTS;
84+
return getEmptyCounts(buckets);
85+
}
86+
87+
private static CountAtBucket[] getEmptyCounts(double[] buckets) {
9088
CountAtBucket[] countAtBuckets = new CountAtBucket[buckets.length];
9189
for (int i = 0; i < buckets.length; i++) {
9290
countAtBuckets[i] = new CountAtBucket(buckets[i], 0);
9391
}
9492
return countAtBuckets;
9593
}
9694

95+
private static double[] getBucketsFromDistributionStatisticConfig(
96+
DistributionStatisticConfig distributionStatisticConfig, boolean supportsAggregablePercentiles) {
97+
if (distributionStatisticConfig.getMaximumExpectedValueAsDouble() == null
98+
|| distributionStatisticConfig.getMinimumExpectedValueAsDouble() == null) {
99+
throw new InvalidConfigurationException(
100+
"minimumExpectedValue and maximumExpectedValue should be greater than 0.");
101+
}
102+
NavigableSet<Double> histogramBuckets = distributionStatisticConfig
103+
.getHistogramBuckets(supportsAggregablePercentiles);
104+
105+
return histogramBuckets.stream().filter(Objects::nonNull).mapToDouble(Double::doubleValue).toArray();
106+
}
107+
97108
}

‎micrometer-core/src/main/java/io/micrometer/core/instrument/distribution/TimeWindowFixedBoundaryHistogram.java

Lines changed: 6 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ public class TimeWindowFixedBoundaryHistogram extends AbstractTimeWindowHistogra
3636

3737
private final double[] buckets;
3838

39-
private final boolean cumulativeBucketCounts;
39+
private final boolean isCumulativeBucketCounts;
4040

4141
public TimeWindowFixedBoundaryHistogram(Clock clock, DistributionStatisticConfig config,
4242
boolean supportsAggregablePercentiles) {
@@ -48,14 +48,14 @@ public TimeWindowFixedBoundaryHistogram(Clock clock, DistributionStatisticConfig
4848
* @param clock clock
4949
* @param config distribution statistic configuration
5050
* @param supportsAggregablePercentiles whether it supports aggregable percentiles
51-
* @param cumulativeBucketCounts whether it uses cumulative bucket counts
51+
* @param isCumulativeBucketCounts whether it uses cumulative bucket counts
5252
* @since 1.9.0
5353
*/
5454
public TimeWindowFixedBoundaryHistogram(Clock clock, DistributionStatisticConfig config,
55-
boolean supportsAggregablePercentiles, boolean cumulativeBucketCounts) {
55+
boolean supportsAggregablePercentiles, boolean isCumulativeBucketCounts) {
5656
super(clock, config, FixedBoundaryHistogram.class, supportsAggregablePercentiles);
5757

58-
this.cumulativeBucketCounts = cumulativeBucketCounts;
58+
this.isCumulativeBucketCounts = isCumulativeBucketCounts;
5959

6060
NavigableSet<Double> histogramBuckets = distributionStatisticConfig
6161
.getHistogramBuckets(supportsAggregablePercentiles);
@@ -71,7 +71,7 @@ public TimeWindowFixedBoundaryHistogram(Clock clock, DistributionStatisticConfig
7171

7272
@Override
7373
FixedBoundaryHistogram newBucket() {
74-
return new FixedBoundaryHistogram(this.buckets);
74+
return new FixedBoundaryHistogram(this.buckets, isCumulativeBucketCounts);
7575
}
7676

7777
@Override
@@ -114,27 +114,7 @@ void resetAccumulatedHistogram() {
114114
*/
115115
@Override
116116
Iterator<CountAtBucket> countsAtValues(Iterator<Double> values) {
117-
return new Iterator<CountAtBucket>() {
118-
private double cumulativeCount = 0.0;
119-
120-
@Override
121-
public boolean hasNext() {
122-
return values.hasNext();
123-
}
124-
125-
@Override
126-
public CountAtBucket next() {
127-
double value = values.next();
128-
double count = currentHistogram().countAtValue(value);
129-
if (cumulativeBucketCounts) {
130-
cumulativeCount += count;
131-
return new CountAtBucket(value, cumulativeCount);
132-
}
133-
else {
134-
return new CountAtBucket(value, count);
135-
}
136-
}
137-
};
117+
return currentHistogram().countsAtValues(values);
138118
}
139119

140120
@Override

‎micrometer-core/src/main/java/io/micrometer/core/instrument/step/StepValue.java

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
*/
1616
package io.micrometer.core.instrument.step;
1717

18+
import io.micrometer.common.lang.Nullable;
1819
import io.micrometer.core.instrument.Clock;
1920

2021
import java.util.concurrent.atomic.AtomicLong;
@@ -34,13 +35,18 @@ public abstract class StepValue<V> {
3435

3536
private final long stepMillis;
3637

37-
private AtomicLong lastInitPos;
38+
private final AtomicLong lastInitPos;
3839

39-
private volatile V previous = noValue();
40+
private volatile V previous;
4041

4142
public StepValue(final Clock clock, final long stepMillis) {
43+
this(clock, stepMillis, null);
44+
}
45+
46+
protected StepValue(final Clock clock, final long stepMillis, @Nullable final V initValue) {
4247
this.clock = clock;
4348
this.stepMillis = stepMillis;
49+
this.previous = initValue == null ? noValue() : initValue;
4450
lastInitPos = new AtomicLong(clock.wallTime() / stepMillis);
4551
}
4652

‎micrometer-core/src/test/java/io/micrometer/core/instrument/distribution/StepHistogramTest.java

Lines changed: 50 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -42,23 +42,21 @@ private static DistributionStatisticConfig getConfig(boolean percentilesHistogra
4242
void aggregablePercentilesTrue_AddsBuckets() {
4343
boolean supportsAggregablePercentiles = true;
4444
try (StepBucketHistogram histogram = new StepBucketHistogram(clock, step.toMillis(),
45-
distributionStatisticConfig, supportsAggregablePercentiles)) {
46-
// TODO I don't think we should need this to have the correct buckets (even if
47-
// their counts are all 0)
45+
distributionStatisticConfig, supportsAggregablePercentiles, true)) {
46+
assertThat(histogram.takeSnapshot(0, 0, 0).histogramCounts()).isNotEmpty();
4847
clock.add(step);
49-
assertThat(histogram.takeSnapshot(0, 0, 0).histogramCounts().length).isGreaterThan(0);
48+
assertThat(histogram.takeSnapshot(0, 0, 0).histogramCounts()).isNotEmpty();
5049
}
5150
}
5251

5352
@Test
5453
void aggregablePercentilesFalse_NoBuckets() {
5554
boolean supportsAggregablePercentiles = false;
5655
try (StepBucketHistogram histogram = new StepBucketHistogram(clock, step.toMillis(),
57-
distributionStatisticConfig, supportsAggregablePercentiles)) {
58-
// TODO I don't think we should need this to have the correct buckets (even if
59-
// their counts are all 0)
56+
distributionStatisticConfig, supportsAggregablePercentiles, true)) {
57+
assertThat(histogram.takeSnapshot(0, 0, 0).histogramCounts()).isEmpty();
6058
clock.add(step);
61-
assertThat(histogram.takeSnapshot(0, 0, 0).histogramCounts().length).isZero();
59+
assertThat(histogram.takeSnapshot(0, 0, 0).histogramCounts()).isEmpty();
6260
}
6361
}
6462

@@ -70,9 +68,8 @@ void sloWithAggregablePercentilesFalse_onlySloBucket() {
7068
.build()
7169
.merge(distributionStatisticConfig);
7270
try (StepBucketHistogram histogram = new StepBucketHistogram(clock, step.toMillis(), config,
73-
supportsAggregablePercentiles)) {
74-
// TODO I don't think we should need this to have the correct buckets (even if
75-
// their counts are all 0)
71+
supportsAggregablePercentiles, true)) {
72+
assertThat(histogram.takeSnapshot(0, 0, 0).histogramCounts().length).isOne();
7673
clock.add(step);
7774
assertThat(histogram.takeSnapshot(0, 0, 0).histogramCounts().length).isOne();
7875
}
@@ -88,9 +85,8 @@ void sloWithAggregablePercentilesTrue_sloBucketPlusPercentilesHistogramBuckets()
8885
.build()
8986
.merge(distributionStatisticConfig);
9087
try (StepBucketHistogram histogram = new StepBucketHistogram(clock, step.toMillis(), config,
91-
supportsAggregablePercentiles)) {
92-
// TODO I don't think we should need this to have the correct buckets (even if
93-
// their counts are all 0)
88+
supportsAggregablePercentiles, true)) {
89+
assertThat(histogram.takeSnapshot(0, 0, 0).histogramCounts()).contains(new CountAtBucket(slo, 0));
9490
clock.add(step);
9591
assertThat(histogram.takeSnapshot(0, 0, 0).histogramCounts()).contains(new CountAtBucket(slo, 0));
9692
}
@@ -106,9 +102,8 @@ void sloWithPercentileHistogramFalse_onlySloBucket() {
106102
.build()
107103
.merge(getConfig(false));
108104
try (StepBucketHistogram histogram = new StepBucketHistogram(clock, step.toMillis(), config,
109-
supportsAggregablePercentiles)) {
110-
// TODO I don't think we should need this to have the correct buckets (even if
111-
// their counts are all 0)
105+
supportsAggregablePercentiles, true)) {
106+
assertThat(histogram.takeSnapshot(0, 0, 0).histogramCounts()).contains(new CountAtBucket(slo, 0));
112107
clock.add(step);
113108
assertThat(histogram.takeSnapshot(0, 0, 0).histogramCounts()).contains(new CountAtBucket(slo, 0));
114109
}
@@ -123,7 +118,7 @@ void bucketCountRollover() {
123118
.build()
124119
.merge(distributionStatisticConfig);
125120
try (StepBucketHistogram histogram = new StepBucketHistogram(clock, step.toMillis(), config,
126-
supportsAggregablePercentiles)) {
121+
supportsAggregablePercentiles, false)) {
127122
histogram.recordDouble(slo - 1);
128123
histogram.recordDouble(slo);
129124
histogram.recordDouble(slo + 1);
@@ -142,13 +137,49 @@ void bucketCountRollover() {
142137
}
143138
}
144139

140+
@Test
141+
void bucketCountRolloverCumulativeBucket() {
142+
boolean supportsAggregablePercentiles = true;
143+
double slo = 15.0;
144+
DistributionStatisticConfig config = DistributionStatisticConfig.builder()
145+
.serviceLevelObjectives(slo)
146+
.build()
147+
.merge(distributionStatisticConfig);
148+
try (StepBucketHistogram histogram = new StepBucketHistogram(clock, step.toMillis(), config,
149+
supportsAggregablePercentiles, true)) {
150+
histogram.recordDouble(slo - 1);
151+
histogram.recordDouble(slo);
152+
histogram.recordDouble(slo + 1);
153+
assertThat(histogram.takeSnapshot(0, 0, 0).histogramCounts()).allMatch(bucket -> bucket.count() == 0);
154+
clock.add(step);
155+
Arrays.stream(histogram.takeSnapshot(0, 0, 0).histogramCounts()).forEach(bucket -> {
156+
// In case of cumulative buckets, value at bucket denotes 0 to bucket and
157+
// not between 2 buckets.
158+
if (bucket.bucket() < slo - 1) {
159+
assertThat(bucket.count()).isZero();
160+
}
161+
else if (bucket.bucket() == slo - 1) {
162+
assertThat(bucket.count()).isOne();
163+
}
164+
else if (bucket.bucket() == slo) {
165+
assertThat(bucket.count()).isEqualTo(2);
166+
}
167+
else {
168+
assertThat(bucket.count()).isEqualTo(3);
169+
}
170+
});
171+
clock.add(step);
172+
assertThat(histogram.takeSnapshot(0, 0, 0).histogramCounts()).allMatch(bucket -> bucket.count() == 0);
173+
}
174+
}
175+
145176
@Test
146177
void doesNotSupportPercentiles() {
147178
DistributionStatisticConfig config = DistributionStatisticConfig.builder()
148179
.percentiles(0.5, 0.9)
149180
.build()
150181
.merge(distributionStatisticConfig);
151-
try (StepBucketHistogram histogram = new StepBucketHistogram(clock, step.toMillis(), config, false)) {
182+
try (StepBucketHistogram histogram = new StepBucketHistogram(clock, step.toMillis(), config, false, true)) {
152183
histogram.recordDouble(10.0);
153184
clock.add(step);
154185
assertThat(histogram.takeSnapshot(0, 0, 0).percentileValues()).isEmpty();

0 commit comments

Comments
 (0)