From 19196a02519cf83ee0867f58c7e9532d3ed29c55 Mon Sep 17 00:00:00 2001 From: jack-berg <34418638+jack-berg@users.noreply.github.com> Date: Mon, 6 Nov 2023 10:58:34 -0600 Subject: [PATCH] Stabilize explicit bucket boundaries advice API (#5897) --- .../api/metrics/DoubleHistogramBuilder.java | 16 +++ .../api/metrics/LongHistogramBuilder.java | 16 +++ .../current_vs_latest/opentelemetry-api.txt | 7 +- .../ExtendedDoubleHistogramBuilder.java | 9 -- .../metrics/ExtendedLongHistogramBuilder.java | 9 -- .../sdk/metrics/SdkDoubleHistogram.java | 9 ++ .../sdk/metrics/SdkLongHistogram.java | 15 ++- .../ExplicitBucketHistogramUtils.java | 21 ++- .../ExplicitBucketBoundariesAdviceTest.java | 121 ++++++++++++++++-- 9 files changed, 176 insertions(+), 47 deletions(-) diff --git a/api/all/src/main/java/io/opentelemetry/api/metrics/DoubleHistogramBuilder.java b/api/all/src/main/java/io/opentelemetry/api/metrics/DoubleHistogramBuilder.java index 68d89a3302b..bc93b3c9e3f 100644 --- a/api/all/src/main/java/io/opentelemetry/api/metrics/DoubleHistogramBuilder.java +++ b/api/all/src/main/java/io/opentelemetry/api/metrics/DoubleHistogramBuilder.java @@ -5,6 +5,8 @@ package io.opentelemetry.api.metrics; +import java.util.List; + /** * Builder class for {@link DoubleHistogram}. * @@ -32,6 +34,20 @@ public interface DoubleHistogramBuilder { */ DoubleHistogramBuilder setUnit(String unit); + /** + * Set the explicit bucket buckets boundaries advice, which suggests the recommended set of + * explicit bucket boundaries for this histogram. + * + * @param bucketBoundaries The explicit bucket boundaries advice. + * @see Explicit + * bucket boundaries advisory parameter + * @since 1.32.0 + */ + default DoubleHistogramBuilder setExplicitBucketBoundariesAdvice(List bucketBoundaries) { + return this; + } + /** Sets the Counter for recording {@code long} values. */ LongHistogramBuilder ofLongs(); diff --git a/api/all/src/main/java/io/opentelemetry/api/metrics/LongHistogramBuilder.java b/api/all/src/main/java/io/opentelemetry/api/metrics/LongHistogramBuilder.java index 80f59753457..baad3928346 100644 --- a/api/all/src/main/java/io/opentelemetry/api/metrics/LongHistogramBuilder.java +++ b/api/all/src/main/java/io/opentelemetry/api/metrics/LongHistogramBuilder.java @@ -5,6 +5,8 @@ package io.opentelemetry.api.metrics; +import java.util.List; + /** * Builder class for {@link LongHistogram}. * @@ -31,6 +33,20 @@ public interface LongHistogramBuilder { */ LongHistogramBuilder setUnit(String unit); + /** + * Set the explicit bucket buckets boundaries advice, which suggests the recommended set of + * explicit bucket boundaries for this histogram. + * + * @param bucketBoundaries The explicit bucket boundaries advice. + * @see Explicit + * bucket boundaries advisory parameter + * @since 1.32.0 + */ + default LongHistogramBuilder setExplicitBucketBoundariesAdvice(List bucketBoundaries) { + return this; + } + /** * Builds and returns a Histogram instrument with the configuration. * diff --git a/docs/apidiffs/current_vs_latest/opentelemetry-api.txt b/docs/apidiffs/current_vs_latest/opentelemetry-api.txt index df26146497b..faa8d3cf893 100644 --- a/docs/apidiffs/current_vs_latest/opentelemetry-api.txt +++ b/docs/apidiffs/current_vs_latest/opentelemetry-api.txt @@ -1,2 +1,7 @@ Comparing source compatibility of against -No changes. \ No newline at end of file +*** MODIFIED INTERFACE: PUBLIC ABSTRACT io.opentelemetry.api.metrics.DoubleHistogramBuilder (not serializable) + === CLASS FILE FORMAT VERSION: 52.0 <- 52.0 + +++ NEW METHOD: PUBLIC(+) io.opentelemetry.api.metrics.DoubleHistogramBuilder setExplicitBucketBoundariesAdvice(java.util.List) +*** MODIFIED INTERFACE: PUBLIC ABSTRACT io.opentelemetry.api.metrics.LongHistogramBuilder (not serializable) + === CLASS FILE FORMAT VERSION: 52.0 <- 52.0 + +++ NEW METHOD: PUBLIC(+) io.opentelemetry.api.metrics.LongHistogramBuilder setExplicitBucketBoundariesAdvice(java.util.List) diff --git a/extensions/incubator/src/main/java/io/opentelemetry/extension/incubator/metrics/ExtendedDoubleHistogramBuilder.java b/extensions/incubator/src/main/java/io/opentelemetry/extension/incubator/metrics/ExtendedDoubleHistogramBuilder.java index 881727538c2..dc515448f52 100644 --- a/extensions/incubator/src/main/java/io/opentelemetry/extension/incubator/metrics/ExtendedDoubleHistogramBuilder.java +++ b/extensions/incubator/src/main/java/io/opentelemetry/extension/incubator/metrics/ExtendedDoubleHistogramBuilder.java @@ -12,15 +12,6 @@ /** Extended {@link DoubleHistogramBuilder} with experimental APIs. */ public interface ExtendedDoubleHistogramBuilder extends DoubleHistogramBuilder { - /** - * Specify the explicit bucket buckets boundaries advice, which suggests the recommended set of - * explicit bucket boundaries for this histogram. - */ - default ExtendedDoubleHistogramBuilder setExplicitBucketBoundariesAdvice( - List bucketBoundaries) { - return this; - } - /** * Specify the attribute advice, which suggests the recommended set of attribute keys to be used * for this histogram. diff --git a/extensions/incubator/src/main/java/io/opentelemetry/extension/incubator/metrics/ExtendedLongHistogramBuilder.java b/extensions/incubator/src/main/java/io/opentelemetry/extension/incubator/metrics/ExtendedLongHistogramBuilder.java index 665e4e3555e..3afb35b7e4c 100644 --- a/extensions/incubator/src/main/java/io/opentelemetry/extension/incubator/metrics/ExtendedLongHistogramBuilder.java +++ b/extensions/incubator/src/main/java/io/opentelemetry/extension/incubator/metrics/ExtendedLongHistogramBuilder.java @@ -12,15 +12,6 @@ /** Extended {@link LongHistogramBuilder} with experimental APIs. */ public interface ExtendedLongHistogramBuilder extends LongHistogramBuilder { - /** - * Specify the explicit bucket buckets boundaries advice, which suggests the recommended set of - * explicit bucket boundaries for this histogram. - */ - default ExtendedLongHistogramBuilder setExplicitBucketBoundariesAdvice( - List bucketBoundaries) { - return this; - } - /** * Specify the attribute advice, which suggests the recommended set of attribute keys to be used * for this histogram. diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkDoubleHistogram.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkDoubleHistogram.java index 64a0daac68e..9d9c1535f0b 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkDoubleHistogram.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkDoubleHistogram.java @@ -13,11 +13,13 @@ import io.opentelemetry.context.Context; import io.opentelemetry.extension.incubator.metrics.ExtendedDoubleHistogramBuilder; import io.opentelemetry.sdk.internal.ThrottlingLogger; +import io.opentelemetry.sdk.metrics.internal.aggregator.ExplicitBucketHistogramUtils; import io.opentelemetry.sdk.metrics.internal.descriptor.InstrumentDescriptor; import io.opentelemetry.sdk.metrics.internal.state.MeterProviderSharedState; import io.opentelemetry.sdk.metrics.internal.state.MeterSharedState; import io.opentelemetry.sdk.metrics.internal.state.WriteableMetricStorage; import java.util.List; +import java.util.Objects; import java.util.logging.Level; import java.util.logging.Logger; @@ -97,6 +99,13 @@ public LongHistogramBuilder ofLongs() { @Override public ExtendedDoubleHistogramBuilder setExplicitBucketBoundariesAdvice( List bucketBoundaries) { + try { + Objects.requireNonNull(bucketBoundaries, "bucketBoundaries must not be null"); + ExplicitBucketHistogramUtils.validateBucketBoundaries(bucketBoundaries); + } catch (IllegalArgumentException | NullPointerException e) { + logger.warning("Error setting explicit bucket boundaries advice: " + e.getMessage()); + return this; + } builder.setExplicitBucketBoundaries(bucketBoundaries); return this; } diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkLongHistogram.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkLongHistogram.java index 4e2e223fa90..38a7e52293e 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkLongHistogram.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkLongHistogram.java @@ -12,12 +12,14 @@ import io.opentelemetry.context.Context; import io.opentelemetry.extension.incubator.metrics.ExtendedLongHistogramBuilder; import io.opentelemetry.sdk.internal.ThrottlingLogger; +import io.opentelemetry.sdk.metrics.internal.aggregator.ExplicitBucketHistogramUtils; import io.opentelemetry.sdk.metrics.internal.descriptor.Advice; import io.opentelemetry.sdk.metrics.internal.descriptor.InstrumentDescriptor; import io.opentelemetry.sdk.metrics.internal.state.MeterProviderSharedState; import io.opentelemetry.sdk.metrics.internal.state.MeterSharedState; import io.opentelemetry.sdk.metrics.internal.state.WriteableMetricStorage; import java.util.List; +import java.util.Objects; import java.util.logging.Level; import java.util.logging.Logger; import java.util.stream.Collectors; @@ -99,9 +101,16 @@ public SdkLongHistogram build() { @Override public ExtendedLongHistogramBuilder setExplicitBucketBoundariesAdvice( List bucketBoundaries) { - List doubleBoundaries = - bucketBoundaries.stream().map(Long::doubleValue).collect(Collectors.toList()); - builder.setExplicitBucketBoundaries(doubleBoundaries); + List boundaries; + try { + Objects.requireNonNull(bucketBoundaries, "bucketBoundaries must not be null"); + boundaries = bucketBoundaries.stream().map(Long::doubleValue).collect(Collectors.toList()); + ExplicitBucketHistogramUtils.validateBucketBoundaries(boundaries); + } catch (IllegalArgumentException | NullPointerException e) { + logger.warning("Error setting explicit bucket boundaries advice: " + e.getMessage()); + return this; + } + builder.setExplicitBucketBoundaries(boundaries); return this; } diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/ExplicitBucketHistogramUtils.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/ExplicitBucketHistogramUtils.java index fd11247c57c..4d25b873f5a 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/ExplicitBucketHistogramUtils.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/ExplicitBucketHistogramUtils.java @@ -26,7 +26,8 @@ private ExplicitBucketHistogramUtils() {} /** Converts bucket boundary "convenient" configuration into the "more efficient" array. */ public static double[] createBoundaryArray(List boundaries) { - return validateBucketBoundaries(boundaries.stream().mapToDouble(i -> i).toArray()); + validateBucketBoundaries(boundaries); + return boundaries.stream().mapToDouble(i -> i).toArray(); } /** @@ -51,32 +52,30 @@ public static int findBucketIndex(double[] boundaries, double value) { * Validates errors in boundary configuration. * * @param boundaries The array of bucket boundaries. - * @return The original boundaries. * @throws IllegalArgumentException if boundaries are not specified correctly. */ - public static double[] validateBucketBoundaries(double[] boundaries) { + public static void validateBucketBoundaries(List boundaries) { for (double v : boundaries) { if (Double.isNaN(v)) { throw new IllegalArgumentException("invalid bucket boundary: NaN"); } } - for (int i = 1; i < boundaries.length; ++i) { - if (boundaries[i - 1] >= boundaries[i]) { + for (int i = 1; i < boundaries.size(); ++i) { + if (boundaries.get(i - 1) >= boundaries.get(i)) { throw new IllegalArgumentException( "Bucket boundaries must be in increasing order: " - + boundaries[i - 1] + + boundaries.get(i - 1) + " >= " - + boundaries[i]); + + boundaries.get(i)); } } - if (boundaries.length > 0) { - if (boundaries[0] == Double.NEGATIVE_INFINITY) { + if (!boundaries.isEmpty()) { + if (boundaries.get(0) == Double.NEGATIVE_INFINITY) { throw new IllegalArgumentException("invalid bucket boundary: -Inf"); } - if (boundaries[boundaries.length - 1] == Double.POSITIVE_INFINITY) { + if (boundaries.get(boundaries.size() - 1) == Double.POSITIVE_INFINITY) { throw new IllegalArgumentException("invalid bucket boundary: +Inf"); } } - return boundaries; } } diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/ExplicitBucketBoundariesAdviceTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/ExplicitBucketBoundariesAdviceTest.java index 555f852df1d..16d4d05cac0 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/ExplicitBucketBoundariesAdviceTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/ExplicitBucketBoundariesAdviceTest.java @@ -8,10 +8,10 @@ import static io.opentelemetry.sdk.metrics.Aggregation.explicitBucketHistogram; import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat; +import io.github.netmikey.logunit.api.LogCapturer; import io.opentelemetry.api.metrics.DoubleHistogram; import io.opentelemetry.api.metrics.LongHistogram; -import io.opentelemetry.extension.incubator.metrics.ExtendedDoubleHistogramBuilder; -import io.opentelemetry.extension.incubator.metrics.ExtendedLongHistogramBuilder; +import io.opentelemetry.internal.testing.slf4j.SuppressLogger; import io.opentelemetry.sdk.metrics.export.AggregationTemporalitySelector; import io.opentelemetry.sdk.metrics.export.DefaultAggregationSelector; import io.opentelemetry.sdk.testing.exporter.InMemoryMetricReader; @@ -21,6 +21,7 @@ import java.util.function.Function; import java.util.stream.Stream; import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.extension.RegisterExtension; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; @@ -29,6 +30,12 @@ class ExplicitBucketBoundariesAdviceTest { private SdkMeterProvider meterProvider = SdkMeterProvider.builder().build(); + @RegisterExtension + LogCapturer logCapturer = + LogCapturer.create() + .captureForLogger(SdkLongHistogram.class.getName()) + .captureForLogger(SdkDoubleHistogram.class.getName()); + @AfterEach void cleanup() { meterProvider.close(); @@ -61,6 +68,8 @@ void histogramWithoutAdvice(Function> histogram .hasBucketBoundaries( 0d, 5d, 10d, 25d, 50d, 75d, 100d, 250d, 500d, 750d, 1_000d, 2_500d, 5_000d, 7_500d, 10_000d)))); + + assertThat(logCapturer.getEvents()).isEmpty(); } @ParameterizedTest @@ -88,6 +97,8 @@ void histogramWithAdvice(Function> histogramBui point .hasBucketCounts(1, 1, 1, 1) .hasBucketBoundaries(10.0, 20.0, 30.0)))); + + assertThat(logCapturer.getEvents()).isEmpty(); } @ParameterizedTest @@ -120,6 +131,8 @@ void histogramWithAdviceAndViews(Function> hist histogram -> histogram.hasPointsSatisfying( point -> point.hasBucketCounts(4, 0).hasBucketBoundaries(50.0)))); + + assertThat(logCapturer.getEvents()).isEmpty(); } @ParameterizedTest @@ -151,6 +164,42 @@ void histogramWithAdviceAndReaderAggregationPreference( histogram -> histogram.hasPointsSatisfying( point -> point.hasBucketCounts(4, 0).hasBucketBoundaries(50.0)))); + + assertThat(logCapturer.getEvents()).isEmpty(); + } + + @ParameterizedTest + @MethodSource("histogramsWithInvalidAdvice") + @SuppressLogger(SdkDoubleHistogram.class) + @SuppressLogger(SdkLongHistogram.class) + void histogramWithInvalidAdvice( + Function> histogramBuilder, String expectedErrorMessage) { + InMemoryMetricReader reader = InMemoryMetricReader.create(); + meterProvider = SdkMeterProvider.builder().registerMetricReader(reader).build(); + + Consumer histogramRecorder = histogramBuilder.apply(meterProvider); + histogramRecorder.accept(5L); + histogramRecorder.accept(15L); + histogramRecorder.accept(25L); + histogramRecorder.accept(35L); + + // Should use default bucket bounds + assertThat(reader.collectAllMetrics()) + .satisfiesExactly( + metric -> + assertThat(metric) + .hasHistogramSatisfying( + histogram -> + histogram.hasPointsSatisfying( + point -> + point + .hasBucketCounts( + 0, 1, 0, 2, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0) + .hasBucketBoundaries( + 0d, 5d, 10d, 25d, 50d, 75d, 100d, 250d, 500d, 750d, + 1_000d, 2_500d, 5_000d, 7_500d, 10_000d)))); + + logCapturer.assertContains(expectedErrorMessage); } private static Stream histogramsWithoutAdvice() { @@ -158,16 +207,16 @@ private static Stream histogramsWithoutAdvice() { Arguments.of( (Function>) meterProvider -> { - DoubleHistogram build = + DoubleHistogram histogram = meterProvider.get("meter").histogramBuilder("histogram").build(); - return build::record; + return histogram::record; }), Arguments.of( (Function>) meterProvider -> { - LongHistogram build = + LongHistogram histogram = meterProvider.get("meter").histogramBuilder("histogram").ofLongs().build(); - return build::record; + return histogram::record; })); } @@ -176,22 +225,66 @@ private static Stream histogramsWithAdvice() { Arguments.of( (Function>) meterProvider -> { - DoubleHistogram build = - ((ExtendedDoubleHistogramBuilder) - meterProvider.get("meter").histogramBuilder("histogram")) + DoubleHistogram histogram = + meterProvider + .get("meter") + .histogramBuilder("histogram") .setExplicitBucketBoundariesAdvice(Arrays.asList(10.0, 20.0, 30.0)) .build(); - return build::record; + return histogram::record; }), Arguments.of( (Function>) meterProvider -> { - LongHistogram build = - ((ExtendedLongHistogramBuilder) - meterProvider.get("meter").histogramBuilder("histogram").ofLongs()) + LongHistogram histogram = + meterProvider + .get("meter") + .histogramBuilder("histogram") + .ofLongs() .setExplicitBucketBoundariesAdvice(Arrays.asList(10L, 20L, 30L)) .build(); - return build::record; + return histogram::record; })); } + + private static Stream histogramsWithInvalidAdvice() { + return Stream.of( + Arguments.of( + (Function>) + meterProvider -> { + DoubleHistogram histogram = + meterProvider + .get("meter") + .histogramBuilder("histogram") + .setExplicitBucketBoundariesAdvice(Arrays.asList(10.0, 9.0, 8.0)) + .build(); + return histogram::record; + }, + "Error setting explicit bucket boundaries advice: Bucket boundaries must be in increasing order: 10.0 >= 9.0"), + Arguments.of( + (Function>) + meterProvider -> { + LongHistogram histogram = + meterProvider + .get("meter") + .histogramBuilder("histogram") + .ofLongs() + .setExplicitBucketBoundariesAdvice(Arrays.asList(10L, 9L, 8L)) + .build(); + return histogram::record; + }, + "Error setting explicit bucket boundaries advice: Bucket boundaries must be in increasing order: 10.0 >= 9.0"), + Arguments.of( + (Function>) + meterProvider -> { + DoubleHistogram histogram = + meterProvider + .get("meter") + .histogramBuilder("histogram") + .setExplicitBucketBoundariesAdvice(null) + .build(); + return histogram::record; + }, + "Error setting explicit bucket boundaries advice: bucketBoundaries must not be null")); + } }