From 03009ce00611bb6ea3443e2e18485ea13d5515e4 Mon Sep 17 00:00:00 2001 From: jack-berg <34418638+jack-berg@users.noreply.github.com> Date: Thu, 6 Oct 2022 15:02:08 -0500 Subject: [PATCH] Lower exclusive exponential histogram bounds (#4700) * Switch exponential histograms to use lower exclusive boundaries * Cache ExponentialHistogramIndexers * Update javadoc to reflect lower exlusive boundaries * Spotless * Fix typo * Add tests and stop rounding subnormal values * spotless * Add explanatory comments --- .../DoubleExponentialHistogramBuckets.java | 44 +--- .../ExponentialHistogramIndexer.java | 109 +++++++++ .../ExponentialHistogramBuckets.java | 4 +- .../ExponentialHistogramPointData.java | 5 +- ...bleExponentialHistogramAggregatorTest.java | 6 +- ...DoubleExponentialHistogramBucketsTest.java | 4 +- .../ExponentialHistogramIndexerTest.java | 223 ++++++++++++++++++ 7 files changed, 348 insertions(+), 47 deletions(-) create mode 100644 sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/ExponentialHistogramIndexer.java create mode 100644 sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/aggregator/ExponentialHistogramIndexerTest.java 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 e34286a27e4..8bedfd20e97 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 @@ -23,12 +23,10 @@ */ final class DoubleExponentialHistogramBuckets implements ExponentialHistogramBuckets { - private static final double LOG_BASE2_E = 1D / Math.log(2); - private final ExponentialCounterFactory counterFactory; private ExponentialCounter counts; private int scale; - private double scaleFactor; + private ExponentialHistogramIndexer exponentialHistogramIndexer; private long totalCount; DoubleExponentialHistogramBuckets( @@ -36,7 +34,7 @@ final class DoubleExponentialHistogramBuckets implements ExponentialHistogramBuc this.counterFactory = counterFactory; this.counts = counterFactory.newCounter(maxBuckets); this.scale = startingScale; - this.scaleFactor = computeScaleFactor(startingScale); + this.exponentialHistogramIndexer = ExponentialHistogramIndexer.get(scale); this.totalCount = 0; } @@ -45,7 +43,7 @@ final class DoubleExponentialHistogramBuckets implements ExponentialHistogramBuc this.counterFactory = buckets.counterFactory; this.counts = counterFactory.copy(buckets.counts); this.scale = buckets.scale; - this.scaleFactor = buckets.scaleFactor; + this.exponentialHistogramIndexer = buckets.exponentialHistogramIndexer; this.totalCount = buckets.totalCount; } @@ -65,7 +63,7 @@ boolean record(double value) { // Guarded by caller. If passed 0 it would be a bug in the SDK. throw new IllegalStateException("Illegal attempted recording of zero at bucket level."); } - int index = valueToIndex(value); + int index = exponentialHistogramIndexer.computeIndex(value); boolean recordingSuccessful = this.counts.increment(index, 1); if (recordingSuccessful) { totalCount++; @@ -131,7 +129,7 @@ void downscale(int by) { } this.scale = this.scale - by; - this.scaleFactor = computeScaleFactor(this.scale); + this.exponentialHistogramIndexer = ExponentialHistogramIndexer.get(this.scale); } /** @@ -220,7 +218,7 @@ int getScale() { * @return The required scale reduction in order to fit the value in these buckets. */ int getScaleReduction(double value) { - long index = valueToIndex(value); + long index = exponentialHistogramIndexer.computeIndex(value); long newStart = Math.min(index, counts.getIndexStart()); long newEnd = Math.max(index, counts.getIndexEnd()); return getScaleReduction(newStart, newEnd); @@ -237,36 +235,6 @@ int getScaleReduction(long newStart, long newEnd) { return scaleReduction; } - private int getIndexByLogarithm(double value) { - return (int) Math.floor(Math.log(value) * scaleFactor); - } - - private int getIndexByExponent(double value) { - return Math.getExponent(value) >> -scale; - } - - private static double computeScaleFactor(int scale) { - return Math.scalb(LOG_BASE2_E, scale); - } - - /** - * Maps a recorded double value to a bucket index. - * - *

The strategy to retrieve the index is specified in the OpenTelemetry - * specification. - * - * @param value Measured value (must be non-zero). - * @return the index of the bucket which the value maps to. - */ - private int valueToIndex(double value) { - double absValue = Math.abs(value); - if (scale > 0) { - return getIndexByLogarithm(absValue); - } - return getIndexByExponent(absValue); - } - @Override public boolean equals(@Nullable Object obj) { if (!(obj instanceof DoubleExponentialHistogramBuckets)) { diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/ExponentialHistogramIndexer.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/ExponentialHistogramIndexer.java new file mode 100644 index 00000000000..8bd95f3275b --- /dev/null +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/ExponentialHistogramIndexer.java @@ -0,0 +1,109 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.sdk.metrics.internal.aggregator; + +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; + +class ExponentialHistogramIndexer { + + private static final Map cache = new ConcurrentHashMap<>(); + + /** Bit mask used to isolate exponent of IEEE 754 double precision number. */ + private static final long EXPONENT_BIT_MASK = 0x7FF0000000000000L; + + /** Bit mask used to isolate the significand of IEEE 754 double precision number. */ + private static final long SIGNIFICAND_BIT_MASK = 0xFFFFFFFFFFFFFL; + + /** Bias used in representing the exponent of IEEE 754 double precision number. */ + private static final int EXPONENT_BIAS = 1023; + + /** + * The number of bits used to represent the significand of IEEE 754 double precision number, + * excluding the implicit bit. + */ + private static final int SIGNIFICAND_WIDTH = 52; + + /** The number of bits used to represent the exponent of IEEE 754 double precision number. */ + private static final int EXPONENT_WIDTH = 11; + + private static final double LOG_BASE2_E = 1D / Math.log(2); + + private final int scale; + private final double scaleFactor; + + private ExponentialHistogramIndexer(int scale) { + this.scale = scale; + this.scaleFactor = computeScaleFactor(scale); + } + + /** Get an indexer for the given scale. Indexers are cached and reused for */ + public static ExponentialHistogramIndexer get(int scale) { + return cache.computeIfAbsent(scale, unused -> new ExponentialHistogramIndexer(scale)); + } + + /** + * Compute the index for the given value. + * + *

The algorithm to retrieve the index is specified in the OpenTelemetry + * specification. + * + * @param value Measured value (must be non-zero). + * @return the index of the bucket which the value maps to. + */ + int computeIndex(double value) { + double absValue = Math.abs(value); + // For positive scales, compute the index by logarithm, which is simpler but may be + // inaccurate near bucket boundaries + if (scale > 0) { + return getIndexByLogarithm(absValue); + } + // For scale zero, compute the exact index by extracting the exponent + if (scale == 0) { + return mapToIndexScaleZero(absValue); + } + // For negative scales, compute the exact index by extracting the exponent and shifting it to + // the right by -scale + return mapToIndexScaleZero(absValue) >> -scale; + } + + /** + * Compute the bucket index using a logarithm based approach. + * + * @see All + * Scales: Use the Logarithm Function + */ + private int getIndexByLogarithm(double value) { + return (int) Math.ceil(Math.log(value) * scaleFactor) - 1; + } + + /** + * Compute the exact bucket index for scale zero by extracting the exponent. + * + * @see Scale + * Zero: Extract the Exponent + */ + private static int mapToIndexScaleZero(double value) { + long rawBits = Double.doubleToLongBits(value); + long rawExponent = (rawBits & EXPONENT_BIT_MASK) >> SIGNIFICAND_WIDTH; + long rawSignificand = rawBits & SIGNIFICAND_BIT_MASK; + if (rawExponent == 0) { + rawExponent -= Long.numberOfLeadingZeros(rawSignificand - 1) - EXPONENT_WIDTH - 1; + } + int ieeeExponent = (int) (rawExponent - EXPONENT_BIAS); + if (rawSignificand == 0) { + return ieeeExponent - 1; + } + return ieeeExponent; + } + + private static double computeScaleFactor(int scale) { + return Math.scalb(LOG_BASE2_E, scale); + } +} diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/data/exponentialhistogram/ExponentialHistogramBuckets.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/data/exponentialhistogram/ExponentialHistogramBuckets.java index be87d2ad82c..edff712e37d 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/data/exponentialhistogram/ExponentialHistogramBuckets.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/data/exponentialhistogram/ExponentialHistogramBuckets.java @@ -12,7 +12,7 @@ * ExponentialHistogramBuckets represents either the positive or negative measurements taken for a * {@link ExponentialHistogramPointData}. * - *

The bucket boundaries are lower-bound inclusive, and are calculated using the {@link + *

The bucket boundaries are lower-bound exclusive, and are calculated using the {@link * ExponentialHistogramPointData#getScale()} and the {@link #getOffset()}. * *

For example, assume {@link ExponentialHistogramPointData#getScale()} is 0, the base is 2.0. @@ -35,7 +35,7 @@ public interface ExponentialHistogramBuckets { int getOffset(); /** - * The bucket counts is a of counts representing number of measurements that fall into each + * The bucket counts is a list of counts representing number of measurements that fall into each * bucket. * * @return the bucket counts. diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/data/exponentialhistogram/ExponentialHistogramPointData.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/data/exponentialhistogram/ExponentialHistogramPointData.java index 6d9bdc1b178..808ca97d70c 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/data/exponentialhistogram/ExponentialHistogramPointData.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/data/exponentialhistogram/ExponentialHistogramPointData.java @@ -20,8 +20,9 @@ *

The bucket boundaries are calculated using both the scale {@link #getScale()}, and the offset * {@link ExponentialHistogramBuckets#getOffset()}. * - *

See: - * https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/datamodel.md#exponentialhistogram + *

See data + * model. * *

This class is internal and is hence not for public use. Its APIs are unstable and can change * at any time. 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 e4becdaa99c..ec36cf9e9af 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 @@ -68,7 +68,7 @@ private static Stream provideAggregator() private static int valueToIndex(int scale, double value) { double scaleFactor = Math.scalb(1D / Math.log(2), scale); - return (int) Math.floor(Math.log(value) * scaleFactor); + return (int) Math.ceil(Math.log(value) * scaleFactor) - 1; } private static ExponentialHistogramAccumulation getTestAccumulation( @@ -362,8 +362,8 @@ void testInsert1M() { AggregatorHandle handle = aggregator.createHandle(); - double min = 1.0 / (1 << 16); int n = 1024 * 1024 - 1; + double min = 16.0 / n; double d = min; for (int i = 0; i < n; i++) { handle.recordDouble(d); @@ -393,7 +393,7 @@ void testDownScale() { assertThat(Objects.requireNonNull(acc).getScale()).isEqualTo(0); ExponentialHistogramBuckets buckets = acc.getPositiveBuckets(); assertThat(acc.getSum()).isEqualTo(23.5); - assertThat(buckets.getOffset()).isEqualTo(-1); + assertThat(buckets.getOffset()).isEqualTo(-2); assertThat(buckets.getBucketCounts()).isEqualTo(Arrays.asList(1L, 1L, 1L, 1L, 0L, 1L)); assertThat(buckets.getTotalCount()).isEqualTo(5); } 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 b32e534382b..0fa829a8d9d 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 @@ -60,7 +60,7 @@ void testDownscale(ExponentialBucketStrategy buckets) { MetricAssertions.assertThat(b) .hasTotalCount(3) .hasCounts(Arrays.asList(1L, 1L, 1L)) - .hasOffset(0); + .hasOffset(-1); } @ParameterizedTest @@ -110,6 +110,6 @@ void testToString(ExponentialBucketStrategy buckets) { DoubleExponentialHistogramBuckets b = buckets.newBuckets(); b.record(1); assertThat(b.toString()) - .isEqualTo("DoubleExponentialHistogramBuckets{scale: 20, offset: 0, counts: {0=1} }"); + .isEqualTo("DoubleExponentialHistogramBuckets{scale: 20, offset: -1, counts: {-1=1} }"); } } diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/aggregator/ExponentialHistogramIndexerTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/aggregator/ExponentialHistogramIndexerTest.java new file mode 100644 index 00000000000..7bd34d9a21b --- /dev/null +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/aggregator/ExponentialHistogramIndexerTest.java @@ -0,0 +1,223 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.sdk.metrics.internal.aggregator; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.Arrays; +import java.util.function.Consumer; +import org.assertj.core.api.AssertionsForClassTypes; +import org.junit.jupiter.api.Test; + +class ExponentialHistogramIndexerTest { + + @Test + void get_Caches() { + assertThat(ExponentialHistogramIndexer.get(1)).isSameAs(ExponentialHistogramIndexer.get(1)); + assertThat(ExponentialHistogramIndexer.get(2)).isSameAs(ExponentialHistogramIndexer.get(2)); + assertThat(ExponentialHistogramIndexer.get(1)).isNotSameAs(ExponentialHistogramIndexer.get(2)); + } + + @Test + void computeIndex_ScaleOne() { + ExponentialHistogramIndexer indexer = ExponentialHistogramIndexer.get(1); + + Arrays.asList( + // Near +Inf + of(Double.MAX_VALUE, 2047), + of(0x1p1023, 2045), + of(0x1.1p1023, 2046), + of(0x1p1022, 2043), + of(0x1.1p1022, 2044), + // Near 0 + of(0x1p-1022, -2045), + of(0x1.1p-1022, -2044), + of(0x1p-1021, -2043), + of(0x1.1p-1021, -2042), + of(Double.MIN_NORMAL, -2045), + of(Double.MIN_VALUE, -2149), + // Near 1 + of(15d, 7), + of(9d, 6), + of(7d, 5), + of(5d, 4), + of(3d, 3), + of(2.5d, 2), + of(1.5d, 1), + of(1.2d, 0), + of(1d, -1), + of(0.75d, -1), + of(0.55d, -2), + of(0.45d, -3)) + .forEach(test(indexer)); + } + + @Test + void computeIndex_ScaleZero() { + ExponentialHistogramIndexer indexer = ExponentialHistogramIndexer.get(0); + + Arrays.asList( + // Near +Inf + of(Double.MAX_VALUE, Double.MAX_EXPONENT), + of(0x1p1023, 1022), + of(0x1.1p1023, 1023), + of(0x1p1022, 1021), + of(0x1.1p1022, 1022), + // Near 0 + of(0x1p-1022, -1023), + of(0x1.1p-1022, -1022), + of(0x1p-1021, -1022), + of(0x1.1p-1021, -1021), + of(Double.MIN_NORMAL, -1023), + of(Double.MIN_VALUE, -1075), + // Near 1 + of(4, 1), + of(3, 1), + of(2, 0), + of(1.5, 0), + of(1, -1), + of(0.75, -1), + of(0.51, -1), + of(0.5, -2), + of(0.26, -2), + of(0.25, -3), + of(0.126, -3), + of(0.125, -4)) + .forEach(test(indexer)); + } + + @Test + void computeIndex_ScaleNegOne() { + ExponentialHistogramIndexer indexer = ExponentialHistogramIndexer.get(-1); + + Arrays.asList( + of(17d, 2), + of(16d, 1), + of(15d, 1), + of(9d, 1), + of(8d, 1), + of(5d, 1), + of(4d, 0), + of(3d, 0), + of(2d, 0), + of(1.5d, 0), + of(1d, -1), + of(0.75d, -1), + of(0.5d, -1), + of(0.25d, -2), + of(0.20d, -2), + of(0.13d, -2), + of(0.125d, -2), + of(0.10d, -2), + of(0.0625d, -3), + of(0.06d, -3)) + .forEach(test(indexer)); + } + + @Test + void valueToIndex_ScaleNegFour() { + ExponentialHistogramIndexer indexer = ExponentialHistogramIndexer.get(-4); + + Arrays.asList( + of(0x1p0, -1), + of(0x10p0, 0), + of(0x100p0, 0), + of(0x1000p0, 0), + of(0x10000p0, 0), // Base == 2**16 + of(0x100000p0, 1), + of(0x1000000p0, 1), + of(0x10000000p0, 1), + of(0x100000000p0, 1), // == 2**32 + of(0x1000000000p0, 2), + of(0x10000000000p0, 2), + of(0x100000000000p0, 2), + of(0x1000000000000p0, 2), // 2**48 + of(0x10000000000000p0, 3), + of(0x100000000000000p0, 3), + of(0x1000000000000000p0, 3), + of(0x10000000000000000p0, 3), // 2**64 + of(0x100000000000000000p0, 4), + of(0x1000000000000000000p0, 4), + of(0x10000000000000000000p0, 4), + of(0x100000000000000000000p0, 4), + of(0x1000000000000000000000p0, 5), + of(1 / 0x1p0, -1), + of(1 / 0x10p0, -1), + of(1 / 0x100p0, -1), + of(1 / 0x1000p0, -1), + of(1 / 0x10000p0, -2), // 2**-16 + of(1 / 0x100000p0, -2), + of(1 / 0x1000000p0, -2), + of(1 / 0x10000000p0, -2), + of(1 / 0x100000000p0, -3), // 2**-32 + of(1 / 0x1000000000p0, -3), + of(1 / 0x10000000000p0, -3), + of(1 / 0x100000000000p0, -3), + of(1 / 0x1000000000000p0, -4), // 2**-48 + of(1 / 0x10000000000000p0, -4), + of(1 / 0x100000000000000p0, -4), + of(1 / 0x1000000000000000p0, -4), + of(1 / 0x10000000000000000p0, -5), // 2**-64 + of(1 / 0x100000000000000000p0, -5), + + // Max values + of(0x1.FFFFFFFFFFFFFp1023, 63), + of(0x1p1023, 63), + of(0x1p1019, 63), + of(0x1p1009, 63), + of(0x1p1008, 62), + of(0x1p1007, 62), + of(0x1p1000, 62), + of(0x1p0993, 62), + of(0x1p0992, 61), + of(0x1p0991, 61), + + // Min and subnormal values + of(0x1p-1074, -68), + of(0x1p-1073, -68), + of(0x1p-1072, -68), + of(0x1p-1057, -67), + of(0x1p-1056, -67), + of(0x1p-1041, -66), + of(0x1p-1040, -66), + of(0x1p-1025, -65), + of(0x1p-1024, -65), + of(0x1p-1023, -64), + of(0x1p-1022, -64), + of(0x1p-1009, -64), + of(0x1p-1008, -64), + of(0x1p-1007, -63), + of(0x1p-0993, -63), + of(0x1p-0992, -63), + of(0x1p-0991, -62), + of(0x1p-0977, -62), + of(0x1p-0976, -62), + of(0x1p-0975, -61)) + .forEach(test(indexer)); + } + + private static Consumer test(ExponentialHistogramIndexer indexer) { + return testCase -> + AssertionsForClassTypes.assertThat(indexer.computeIndex(testCase.value)) + .describedAs( + "value " + Double.toHexString(testCase.value) + " (" + testCase.value + ")") + .isEqualTo(testCase.expectedBucket); + } + + private static TestCase of(double value, int expectedBucket) { + return new TestCase(value, expectedBucket); + } + + private static class TestCase { + private final double value; + private final int expectedBucket; + + private TestCase(double value, int expectedBucket) { + this.value = value; + this.expectedBucket = expectedBucket; + } + } +}