From f32aa1ca25fed2bbe57200595d8d56aafade69e8 Mon Sep 17 00:00:00 2001 From: Jack Berg Date: Wed, 14 Dec 2022 09:55:20 -0600 Subject: [PATCH] Add MaxScale config parameter to ExponentialHistogram --- .../metric/viewconfig/ViewConfig.java | 2 +- .../aggregator/HistogramCollectBenchmark.java | 3 +- .../DoubleExponentialHistogramAggregator.java | 8 +++-- .../aggregator/ExponentialBucketStrategy.java | 24 +++++--------- .../view/ExponentialHistogramAggregation.java | 33 +++++++++++++++---- .../sdk/metrics/AggregationTest.java | 6 ++-- .../sdk/metrics/SdkDoubleHistogramTest.java | 4 ++- .../sdk/metrics/SdkLongHistogramTest.java | 4 ++- ...bleExponentialHistogramAggregatorTest.java | 12 +++---- ...DoubleExponentialHistogramBucketsTest.java | 4 +-- .../ExponentialHistogramAggregationTest.java | 12 +++++-- 11 files changed, 69 insertions(+), 43 deletions(-) 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 530e156bb19..f6754a63b8c 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 8ac8a212c36..bfcd286a57d 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 @@ -43,11 +43,13 @@ public final class DoubleExponentialHistogramAggregator * @param reservoirSupplier Supplier of exemplar reservoirs per-stream. */ public DoubleExponentialHistogramAggregator( - Supplier> reservoirSupplier, int maxBuckets) { + Supplier> reservoirSupplier, + int maxBuckets, + int maxScale) { this( reservoirSupplier, ExponentialBucketStrategy.newStrategy( - maxBuckets, ExponentialCounterFactory.circularBufferCounter())); + maxBuckets, ExponentialCounterFactory.circularBufferCounter(), maxScale)); } DoubleExponentialHistogramAggregator( @@ -202,7 +204,7 @@ static final class Handle this.min = Double.MAX_VALUE; this.max = -1; this.count = 0; - this.scale = bucketStrategy.getStartingScale(); + this.scale = bucketStrategy.getMaxScale(); } @Override diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/ExponentialBucketStrategy.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/ExponentialBucketStrategy.java index 36981ff5d13..a04972ea375 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/ExponentialBucketStrategy.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/ExponentialBucketStrategy.java @@ -10,39 +10,31 @@ /** The configuration for how to create exponential histogram buckets. */ final class ExponentialBucketStrategy { - private static final int DEFAULT_STARTING_SCALE = 20; - - private final int startingScale; + private final int maxScale; /** The maximum number of buckets that will be used for positive or negative recordings. */ private final int maxBuckets; /** The mechanism of constructing and copying buckets. */ private final ExponentialCounterFactory counterFactory; private ExponentialBucketStrategy( - int startingScale, int maxBuckets, ExponentialCounterFactory counterFactory) { - this.startingScale = startingScale; + int maxScale, int maxBuckets, ExponentialCounterFactory counterFactory) { + this.maxScale = maxScale; this.maxBuckets = maxBuckets; this.counterFactory = counterFactory; } /** Constructs fresh new buckets with default settings. */ DoubleExponentialHistogramBuckets newBuckets() { - return new DoubleExponentialHistogramBuckets(startingScale, maxBuckets, counterFactory); - } - - int getStartingScale() { - return startingScale; + return new DoubleExponentialHistogramBuckets(maxScale, maxBuckets, counterFactory); } - /** Create a new strategy for generating Exponential Buckets. */ - static ExponentialBucketStrategy newStrategy( - int maxBuckets, ExponentialCounterFactory counterFactory) { - return new ExponentialBucketStrategy(DEFAULT_STARTING_SCALE, maxBuckets, counterFactory); + int getMaxScale() { + return maxScale; } /** Create a new strategy for generating Exponential Buckets. */ static ExponentialBucketStrategy newStrategy( - int maxBuckets, ExponentialCounterFactory counterFactory, int startingScale) { - return new ExponentialBucketStrategy(startingScale, maxBuckets, counterFactory); + int maxBuckets, ExponentialCounterFactory counterFactory, int maxScale) { + return new ExponentialBucketStrategy(maxScale, maxBuckets, counterFactory); } } 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 9ea0958cee9..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,23 +28,38 @@ public final class ExponentialHistogramAggregation implements Aggregation, AggregatorFactory { private static final int DEFAULT_MAX_BUCKETS = 160; + 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 @@ -59,7 +75,8 @@ public Aggregator createAggregator( Clock.getDefault(), Runtime.getRuntime().availableProcessors(), RandomSupplier.platformDefault())), - maxBuckets); + maxBuckets, + maxScale); } @Override @@ -75,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 f2baa6b0a20..87054cac8c5 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 @@ -51,14 +51,14 @@ class DoubleExponentialHistogramAggregatorTest { @Mock ExemplarReservoir reservoir; private static final DoubleExponentialHistogramAggregator aggregator = - new DoubleExponentialHistogramAggregator(ExemplarReservoir::doubleNoSamples, 160); + new DoubleExponentialHistogramAggregator(ExemplarReservoir::doubleNoSamples, 160, 20); private static final Resource RESOURCE = Resource.getDefault(); private static final InstrumentationScopeInfo INSTRUMENTATION_SCOPE_INFO = InstrumentationScopeInfo.empty(); private static final MetricDescriptor METRIC_DESCRIPTOR = MetricDescriptor.create("name", "description", "unit"); private static final ExponentialBucketStrategy EXPONENTIAL_BUCKET_STRATEGY = - ExponentialBucketStrategy.newStrategy(160, ExponentialCounterFactory.mapCounter()); + ExponentialBucketStrategy.newStrategy(160, ExponentialCounterFactory.mapCounter(), 20); private static Stream provideAggregator() { return Stream.of( @@ -92,11 +92,11 @@ void createHandle() { assertThat(accumulation.getPositiveBuckets()) .isInstanceOf(DoubleExponentialHistogramAggregator.EmptyExponentialHistogramBuckets.class); assertThat(accumulation.getPositiveBuckets().getScale()) - .isEqualTo(EXPONENTIAL_BUCKET_STRATEGY.getStartingScale()); + .isEqualTo(EXPONENTIAL_BUCKET_STRATEGY.getMaxScale()); assertThat(accumulation.getNegativeBuckets()) .isInstanceOf(DoubleExponentialHistogramAggregator.EmptyExponentialHistogramBuckets.class); assertThat(accumulation.getNegativeBuckets().getScale()) - .isEqualTo(EXPONENTIAL_BUCKET_STRATEGY.getStartingScale()); + .isEqualTo(EXPONENTIAL_BUCKET_STRATEGY.getMaxScale()); } @Test @@ -203,7 +203,7 @@ void testRecordingsAtLimits(DoubleExponentialHistogramAggregator aggregator) { @Test void testExemplarsInAccumulation() { DoubleExponentialHistogramAggregator agg = - new DoubleExponentialHistogramAggregator(() -> reservoir, 160); + new DoubleExponentialHistogramAggregator(() -> reservoir, 160, 20); Attributes attributes = Attributes.builder().put("test", "value").build(); DoubleExemplarData exemplar = @@ -443,7 +443,7 @@ void testToMetricData() { Mockito.when(reservoirSupplier.get()).thenReturn(reservoir); DoubleExponentialHistogramAggregator cumulativeAggregator = - new DoubleExponentialHistogramAggregator(reservoirSupplier, 160); + new DoubleExponentialHistogramAggregator(reservoirSupplier, 160, 20); AggregatorHandle aggregatorHandle = cumulativeAggregator.createHandle(); diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramBucketsTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramBucketsTest.java index 0fa829a8d9d..0200fa5eccc 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramBucketsTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramBucketsTest.java @@ -24,9 +24,9 @@ class DoubleExponentialHistogramBucketsTest { static Stream bucketStrategies() { return Stream.of( - ExponentialBucketStrategy.newStrategy(160, ExponentialCounterFactory.mapCounter()), + ExponentialBucketStrategy.newStrategy(160, ExponentialCounterFactory.mapCounter(), 20), ExponentialBucketStrategy.newStrategy( - 160, ExponentialCounterFactory.circularBufferCounter())); + 160, ExponentialCounterFactory.circularBufferCounter(), 20)); } @ParameterizedTest 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"); } }