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

Author: jack-berg

Diff for: sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramBuckets.java

@@ -23,20 +23,18 @@
  */
 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(
       int startingScale, int maxBuckets, ExponentialCounterFactory counterFactory) {
     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.
-   *
-   * <p>The strategy to retrieve the index is specified in the <a
-   * href="https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/datamodel.md#exponential-buckets">OpenTelemetry
-   * specification</a>.
-   *
-   * @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 for: 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<Integer, ExponentialHistogramIndexer> 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.
+   *
+   * <p>The algorithm to retrieve the index is specified in the <a
+   * href="https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/data-model.md#exponential-buckets">OpenTelemetry
+   * specification</a>.
+   *
+   * @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 <a
+   *     href="https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/data-model.md#all-scales-use-the-logarithm-function">All
+   *     Scales: Use the Logarithm Function</a>
+   */
+  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 <a
+   *     href="https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/data-model.md#scale-zero-extract-the-exponent">Scale
+   *     Zero: Extract the Exponent</a>
+   */
+  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 for: 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}.
  *
- * <p>The bucket boundaries are lower-bound inclusive, and are calculated using the {@link
+ * <p>The bucket boundaries are lower-bound exclusive, and are calculated using the {@link
  * ExponentialHistogramPointData#getScale()} and the {@link #getOffset()}.
  *
  * <p>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 for: sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/data/exponentialhistogram/ExponentialHistogramPointData.java

@@ -20,8 +20,9 @@
  * <p>The bucket boundaries are calculated using both the scale {@link #getScale()}, and the offset
  * {@link ExponentialHistogramBuckets#getOffset()}.
  *
- * <p>See:
- * https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/datamodel.md#exponentialhistogram
+ * <p>See <a
+ * href="https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/datamodel.md#exponentialhistogram">data
+ * model</a>.
  *
  * <p>This class is internal and is hence not for public use. Its APIs are unstable and can change
  * at any time.

Diff for: sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramAggregatorTest.java

@@ -68,7 +68,7 @@ private static Stream<DoubleExponentialHistogramAggregator> 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<ExponentialHistogramAccumulation, DoubleExemplarData> 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 for: 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} }");
   }
 }