diff --git a/sdk-extensions/incubator/src/main/java/io/opentelemetry/sdk/extension/incubator/metric/viewconfig/ViewConfig.java b/sdk-extensions/incubator/src/main/java/io/opentelemetry/sdk/extension/incubator/metric/viewconfig/ViewConfig.java index 547f0261c9e..489e36139a1 100644 --- a/sdk-extensions/incubator/src/main/java/io/opentelemetry/sdk/extension/incubator/metric/viewconfig/ViewConfig.java +++ b/sdk-extensions/incubator/src/main/java/io/opentelemetry/sdk/extension/incubator/metric/viewconfig/ViewConfig.java @@ -207,7 +207,7 @@ static Aggregation toAggregation(String aggregationName, Map agg throw new ConfigurationException("max_buckets must be an integer", e); } if (maxBuckets != null) { - return ExponentialHistogramAggregation.create(maxBuckets); + return ExponentialHistogramAggregation.create(maxBuckets, 20); } } return aggregation; diff --git a/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/internal/aggregator/HistogramCollectBenchmark.java b/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/internal/aggregator/HistogramCollectBenchmark.java index e0ee97b7491..207a8214521 100644 --- a/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/internal/aggregator/HistogramCollectBenchmark.java +++ b/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/internal/aggregator/HistogramCollectBenchmark.java @@ -113,7 +113,8 @@ public void recordAndCollect(ThreadState threadState) { @SuppressWarnings("ImmutableEnumChecker") public enum AggregationGenerator { EXPLICIT_BUCKET_HISTOGRAM(Aggregation.explicitBucketHistogram()), - EXPONENTIAL_BUCKET_HISTOGRAM(ExponentialHistogramAggregation.getDefault()); + DEFAULT_EXPONENTIAL_BUCKET_HISTOGRAM(ExponentialHistogramAggregation.getDefault()), + ZERO_MAX_SCALE_EXPONENTIAL_BUCKET_HISTOGRAM(ExponentialHistogramAggregation.create(160, 0)); private final Aggregation aggregation; diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramAggregator.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramAggregator.java index 14e9e6242a8..15afebc439a 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramAggregator.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramAggregator.java @@ -35,7 +35,7 @@ public final class DoubleExponentialHistogramAggregator private final Supplier> reservoirSupplier; private final int maxBuckets; - private final int startingScale; + private final int maxScale; /** * Constructs an exponential histogram aggregator. @@ -45,15 +45,15 @@ public final class DoubleExponentialHistogramAggregator public DoubleExponentialHistogramAggregator( Supplier> reservoirSupplier, int maxBuckets, - int startingScale) { + int maxScale) { this.reservoirSupplier = reservoirSupplier; this.maxBuckets = maxBuckets; - this.startingScale = startingScale; + this.maxScale = maxScale; } @Override public AggregatorHandle createHandle() { - return new Handle(reservoirSupplier.get(), maxBuckets, startingScale); + return new Handle(reservoirSupplier.get(), maxBuckets, maxScale); } /** @@ -187,7 +187,7 @@ static final class Handle private long count; private int scale; - Handle(ExemplarReservoir reservoir, int maxBuckets, int startingScale) { + Handle(ExemplarReservoir reservoir, int maxBuckets, int maxScale) { super(reservoir); this.maxBuckets = maxBuckets; this.sum = 0; @@ -195,7 +195,7 @@ static final class Handle this.min = Double.MAX_VALUE; this.max = -1; this.count = 0; - this.scale = startingScale; + this.scale = maxScale; } @Override diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramBuckets.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramBuckets.java index 4cc750c0c10..09dc75903cc 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramBuckets.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramBuckets.java @@ -25,10 +25,10 @@ final class DoubleExponentialHistogramBuckets implements ExponentialHistogramBuc private ExponentialHistogramIndexer exponentialHistogramIndexer; private long totalCount; - DoubleExponentialHistogramBuckets(int startingScale, int maxBuckets) { + DoubleExponentialHistogramBuckets(int scale, int maxBuckets) { this.counts = new AdaptingCircularBufferCounter(maxBuckets); - this.scale = startingScale; - this.exponentialHistogramIndexer = ExponentialHistogramIndexer.get(scale); + this.scale = scale; + this.exponentialHistogramIndexer = ExponentialHistogramIndexer.get(this.scale); this.totalCount = 0; } diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/ExponentialHistogramAggregation.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/ExponentialHistogramAggregation.java index 4ab3a876435..d095e64f4cb 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/ExponentialHistogramAggregation.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/ExponentialHistogramAggregation.java @@ -11,6 +11,7 @@ import io.opentelemetry.sdk.internal.RandomSupplier; import io.opentelemetry.sdk.metrics.Aggregation; import io.opentelemetry.sdk.metrics.data.ExemplarData; +import io.opentelemetry.sdk.metrics.data.MetricDataType; import io.opentelemetry.sdk.metrics.internal.aggregator.Aggregator; import io.opentelemetry.sdk.metrics.internal.aggregator.AggregatorFactory; import io.opentelemetry.sdk.metrics.internal.aggregator.DoubleExponentialHistogramAggregator; @@ -27,24 +28,38 @@ public final class ExponentialHistogramAggregation implements Aggregation, AggregatorFactory { private static final int DEFAULT_MAX_BUCKETS = 160; - private static final int DEFAULT_STARTING_SCALE = 20; + private static final int DEFAULT_MAX_SCALE = 20; private static final Aggregation DEFAULT = - new ExponentialHistogramAggregation(DEFAULT_MAX_BUCKETS); + new ExponentialHistogramAggregation(DEFAULT_MAX_BUCKETS, DEFAULT_MAX_SCALE); private final int maxBuckets; + private final int maxScale; - private ExponentialHistogramAggregation(int maxBuckets) { + private ExponentialHistogramAggregation(int maxBuckets, int maxScale) { this.maxBuckets = maxBuckets; + this.maxScale = maxScale; } public static Aggregation getDefault() { return DEFAULT; } - public static Aggregation create(int maxBuckets) { + /** + * Aggregations measurements into an {@link MetricDataType#EXPONENTIAL_HISTOGRAM}. + * + * @param maxBuckets the max number of positive buckets and negative buckets (max total buckets is + * 2 * {@code maxBuckets} + 1 zero bucket). + * @param maxScale the maximum and initial scale. If measurements can't fit in a particular scale + * given the {@code maxBuckets}, the scale is reduced until the measurements can be + * accommodated. Setting maxScale may reduce the number of downscales. Additionally, the + * performance of computing bucket index is improved when scale is <= 0. + * @return the aggregation + */ + public static Aggregation create(int maxBuckets, int maxScale) { checkArgument(maxBuckets >= 1, "maxBuckets must be > 0"); - return new ExponentialHistogramAggregation(maxBuckets); + checkArgument(maxScale <= 20 && maxScale >= -10, "maxScale must be -10 <= x <= 20"); + return new ExponentialHistogramAggregation(maxBuckets, maxScale); } @Override @@ -61,7 +76,7 @@ public Aggregator createAggregator( Runtime.getRuntime().availableProcessors(), RandomSupplier.platformDefault())), maxBuckets, - DEFAULT_STARTING_SCALE); + maxScale); } @Override @@ -77,6 +92,10 @@ public boolean isCompatibleWithInstrument(InstrumentDescriptor instrumentDescrip @Override public String toString() { - return "ExponentialHistogramAggregation{maxBuckets=" + maxBuckets + "}"; + return "ExponentialHistogramAggregation{maxBuckets=" + + maxBuckets + + ",maxScale=" + + maxScale + + "}"; } } diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/AggregationTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/AggregationTest.java index 330f6c67394..3c929cf51ce 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/AggregationTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/AggregationTest.java @@ -31,10 +31,10 @@ void haveToString() { // TODO(jack-berg): Use Aggregation.exponentialHistogram() when available assertThat(ExponentialHistogramAggregation.getDefault()) .asString() - .isEqualTo("ExponentialHistogramAggregation{maxBuckets=160}"); - assertThat(ExponentialHistogramAggregation.create(1)) + .isEqualTo("ExponentialHistogramAggregation{maxBuckets=160,maxScale=20}"); + assertThat(ExponentialHistogramAggregation.create(1, 0)) .asString() - .isEqualTo("ExponentialHistogramAggregation{maxBuckets=1}"); + .isEqualTo("ExponentialHistogramAggregation{maxBuckets=1,maxScale=0}"); } @Test diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/SdkDoubleHistogramTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/SdkDoubleHistogramTest.java index 75728e868d7..6060d78ed57 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/SdkDoubleHistogramTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/SdkDoubleHistogramTest.java @@ -208,7 +208,9 @@ void collectMetrics_ExponentialHistogramAggregation() { .registerMetricReader(sdkMeterReader) .registerView( InstrumentSelector.builder().setType(InstrumentType.HISTOGRAM).build(), - View.builder().setAggregation(ExponentialHistogramAggregation.create(5)).build()) + View.builder() + .setAggregation(ExponentialHistogramAggregation.create(5, 20)) + .build()) .build(); DoubleHistogram doubleHistogram = sdkMeterProvider diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/SdkLongHistogramTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/SdkLongHistogramTest.java index 6ecb61d6d55..35bb68f53c9 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/SdkLongHistogramTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/SdkLongHistogramTest.java @@ -208,7 +208,9 @@ void collectMetrics_ExponentialHistogramAggregation() { .registerMetricReader(sdkMeterReader) .registerView( InstrumentSelector.builder().setType(InstrumentType.HISTOGRAM).build(), - View.builder().setAggregation(ExponentialHistogramAggregation.create(5)).build()) + View.builder() + .setAggregation(ExponentialHistogramAggregation.create(5, 20)) + .build()) .build(); LongHistogram longHistogram = sdkMeterProvider diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramAggregatorTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramAggregatorTest.java index 9c8da1989c0..713fda8a93a 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramAggregatorTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramAggregatorTest.java @@ -49,7 +49,7 @@ class DoubleExponentialHistogramAggregatorTest { @Mock ExemplarReservoir reservoir; - private static final int STARTING_SCALE = 20; + private static final int MAX_SCALE = 20; private static final DoubleExponentialHistogramAggregator aggregator = new DoubleExponentialHistogramAggregator(ExemplarReservoir::doubleNoSamples, 160, 20); private static final Resource RESOURCE = Resource.getDefault(); @@ -62,7 +62,7 @@ private static Stream provideAggregator() return Stream.of( aggregator, new DoubleExponentialHistogramAggregator( - ExemplarReservoir::doubleNoSamples, 160, STARTING_SCALE)); + ExemplarReservoir::doubleNoSamples, 160, MAX_SCALE)); } private static int valueToIndex(int scale, double value) { @@ -89,10 +89,10 @@ void createHandle() { .doAccumulateThenReset(Collections.emptyList()); assertThat(accumulation.getPositiveBuckets()) .isInstanceOf(DoubleExponentialHistogramAggregator.EmptyExponentialHistogramBuckets.class); - assertThat(accumulation.getPositiveBuckets().getScale()).isEqualTo(STARTING_SCALE); + assertThat(accumulation.getPositiveBuckets().getScale()).isEqualTo(MAX_SCALE); assertThat(accumulation.getNegativeBuckets()) .isInstanceOf(DoubleExponentialHistogramAggregator.EmptyExponentialHistogramBuckets.class); - assertThat(accumulation.getNegativeBuckets().getScale()).isEqualTo(STARTING_SCALE); + assertThat(accumulation.getNegativeBuckets().getScale()).isEqualTo(MAX_SCALE); } @Test @@ -199,7 +199,7 @@ void testRecordingsAtLimits(DoubleExponentialHistogramAggregator aggregator) { @Test void testExemplarsInAccumulation() { DoubleExponentialHistogramAggregator agg = - new DoubleExponentialHistogramAggregator(() -> reservoir, 160, STARTING_SCALE); + new DoubleExponentialHistogramAggregator(() -> reservoir, 160, MAX_SCALE); Attributes attributes = Attributes.builder().put("test", "value").build(); DoubleExemplarData exemplar = @@ -437,7 +437,7 @@ void testToMetricData() { Mockito.when(reservoirSupplier.get()).thenReturn(reservoir); DoubleExponentialHistogramAggregator cumulativeAggregator = - new DoubleExponentialHistogramAggregator(reservoirSupplier, 160, STARTING_SCALE); + new DoubleExponentialHistogramAggregator(reservoirSupplier, 160, MAX_SCALE); AggregatorHandle aggregatorHandle = cumulativeAggregator.createHandle(); diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/view/ExponentialHistogramAggregationTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/view/ExponentialHistogramAggregationTest.java index 0f50183ce8c..8f36486e27c 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/view/ExponentialHistogramAggregationTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/view/ExponentialHistogramAggregationTest.java @@ -15,13 +15,19 @@ class ExponentialHistogramAggregationTest { @Test void goodConfig() { assertThat(ExponentialHistogramAggregation.getDefault()).isNotNull(); - assertThat(ExponentialHistogramAggregation.create(10)).isNotNull(); + assertThat(ExponentialHistogramAggregation.create(10, 20)).isNotNull(); } @Test - void badBuckets_throwArgumentException() { - assertThatThrownBy(() -> ExponentialHistogramAggregation.create(0)) + void invalidConfig_Throws() { + assertThatThrownBy(() -> ExponentialHistogramAggregation.create(0, 20)) .isInstanceOf(IllegalArgumentException.class) .hasMessage("maxBuckets must be > 0"); + assertThatThrownBy(() -> ExponentialHistogramAggregation.create(1, 21)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("maxScale must be -10 <= x <= 20"); + assertThatThrownBy(() -> ExponentialHistogramAggregation.create(1, -11)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("maxScale must be -10 <= x <= 20"); } }