From 738d9988ae8c7e5f9b78f664e90d3fc01357cb77 Mon Sep 17 00:00:00 2001 From: jack-berg <34418638+jack-berg@users.noreply.github.com> Date: Wed, 7 Dec 2022 09:19:15 -0600 Subject: [PATCH] Stop extra copy of exponential histogram buckets (#5020) --- .../DoubleExponentialHistogramAggregator.java | 34 ++++++++++++------- .../DoubleExponentialHistogramBuckets.java | 27 ++------------- 2 files changed, 25 insertions(+), 36 deletions(-) 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 59ec1adab32..9552e3f2c1a 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 @@ -58,13 +58,10 @@ public AggregatorHandle 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( @@ -72,11 +69,9 @@ public ExponentialHistogramAccumulation merge( // 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/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 8bedfd20e97..9193bbbed3a 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 @@ -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. * *

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