Stop extra copy of exponential histogram buckets (#5020)

open-telemetry · Dec 7, 2022 · 738d998 · 738d998
1 parent c6d1ec1
commit 738d998
Show file tree

Hide file tree

Showing 2 changed files with 25 additions and 36 deletions.
diff --git a/...o/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramAggregator.java b/...o/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramAggregator.java
@@ -58,25 +58,20 @@ public AggregatorHandle<ExponentialHistogramAccumulation, DoubleExemplarData> cr
   }
 
   /**
-   * This function is an immutable merge. It firstly combines the sum and zero count. Then it
-   * performs a merge using the buckets from both accumulations, without modifying those
-   * accumulations.
-   *
-   * @param previous the previously captured accumulation
-   * @param current the newly captured (delta) accumulation
-   * @return the result of the merge of the given accumulations.
+   * Merge the exponential histogram accumulations. Mutates the {@link
+   * ExponentialHistogramAccumulation#getPositiveBuckets()} and {@link
+   * ExponentialHistogramAccumulation#getNegativeBuckets()} of {@code previous}. Mutating buckets is
+   * acceptable because copies are already made in {@link Handle#doAccumulateThenReset(List)}.
    */
   @Override
   public ExponentialHistogramAccumulation merge(
       ExponentialHistogramAccumulation previous, ExponentialHistogramAccumulation current) {
 
     // Create merged buckets
     DoubleExponentialHistogramBuckets posBuckets =
-        DoubleExponentialHistogramBuckets.merge(
-            previous.getPositiveBuckets(), current.getPositiveBuckets());
+        merge(previous.getPositiveBuckets(), current.getPositiveBuckets());
     DoubleExponentialHistogramBuckets negBuckets =
-        DoubleExponentialHistogramBuckets.merge(
-            previous.getNegativeBuckets(), current.getNegativeBuckets());
+        merge(previous.getNegativeBuckets(), current.getNegativeBuckets());
 
     // resolve possible scale difference due to merge
     int commonScale = Math.min(posBuckets.getScale(), negBuckets.getScale());
@@ -95,7 +90,7 @@ public ExponentialHistogramAccumulation merge(
       max = current.getMax();
     }
     return ExponentialHistogramAccumulation.create(
-        posBuckets.getScale(),
+        commonScale,
         previous.getSum() + current.getSum(),
         previous.hasMinMax() || current.hasMinMax(),
         min,
@@ -106,6 +101,21 @@ public ExponentialHistogramAccumulation merge(
         current.getExemplars());
   }
 
+  /**
+   * Merge the exponential histogram buckets. If {@code a} is empty, return {@code b}. If {@code b}
+   * is empty, return {@code a}. Else merge {@code b} into {@code a}.
+   */
+  private static DoubleExponentialHistogramBuckets merge(
+      DoubleExponentialHistogramBuckets a, DoubleExponentialHistogramBuckets b) {
+    if (b.getTotalCount() == 0) {
+      return a;
+    } else if (a.getTotalCount() == 0) {
+      return b;
+    }
+    a.mergeInto(b);
+    return a;
+  }
+
   @Override
   public MetricData toMetricData(
       Resource resource,

diff --git a/...a/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramBuckets.java b/...a/io/opentelemetry/sdk/metrics/internal/aggregator/DoubleExponentialHistogramBuckets.java
@@ -132,30 +132,9 @@ void downscale(int by) {
     this.exponentialHistogramIndexer = ExponentialHistogramIndexer.get(this.scale);
   }
 
-  /**
-   * Immutable method for merging. This method copies the first set of buckets, performs the merge
-   * on the copy, and returns the copy.
-   *
-   * @param a first buckets
-   * @param b second buckets
-   * @return A new set of buckets, the result
-   */
-  static DoubleExponentialHistogramBuckets merge(
-      DoubleExponentialHistogramBuckets a, DoubleExponentialHistogramBuckets b) {
-    if (b.counts.isEmpty()) {
-      return a;
-    } else if (a.counts.isEmpty()) {
-      return b;
-    }
-    DoubleExponentialHistogramBuckets copy = a.copy();
-    copy.mergeWith(b/* additive= */ );
-    return copy;
-  }
-
   /**
    * This method merges this instance with another set of buckets. It alters the underlying bucket
-   * counts and scale of this instance only, so it is to be used with caution. For immutability, use
-   * the static merge() method.
+   * counts and scale of this instance only, so it is to be used with caution.
    *
    * <p>The bucket counts of this instance will be added to or subtracted from depending on the
    * additive parameter.
@@ -164,7 +143,7 @@ static DoubleExponentialHistogramBuckets merge(
    *
    * @param other the histogram that will be merged into this one
    */
-  private void mergeWith(DoubleExponentialHistogramBuckets other) {
+  void mergeInto(DoubleExponentialHistogramBuckets other) {
     if (other.counts.isEmpty()) {
       return;
     }
@@ -195,7 +174,7 @@ private void mergeWith(DoubleExponentialHistogramBuckets other) {
     // since we changed scale of this, we need to know the new difference between the two scales
     deltaOther = other.scale - this.scale;
 
-    // do actual merging of other into this. Will decrement or increment depending on sign.
+    // do actual merging of other into this.
     for (int i = other.getOffset(); i <= other.counts.getIndexEnd(); i++) {
       if (!this.counts.increment(i >> deltaOther, other.counts.get(i))) {
         // This should never occur if scales and windows are calculated without bugs