optimise bucket comparison to fix slow test (#4204)

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

@@ -29,13 +29,15 @@ final class DoubleExponentialHistogramBuckets implements ExponentialHistogramBuc
   private ExponentialCounter counts;
   private int scale;
   private double scaleFactor;
+  private long totalCount;
 
   DoubleExponentialHistogramBuckets(
       int scale, int maxBuckets, ExponentialCounterFactory counterFactory) {
     this.counterFactory = counterFactory;
     this.counts = counterFactory.newCounter(maxBuckets);
     this.scale = scale;
     this.scaleFactor = computeScaleFactor(scale);
+    this.totalCount = 0;
   }
 
   // For copying
@@ -44,6 +46,7 @@ final class DoubleExponentialHistogramBuckets implements ExponentialHistogramBuc
     this.counts = counterFactory.copy(buckets.counts);
     this.scale = buckets.scale;
     this.scaleFactor = buckets.scaleFactor;
+    this.totalCount = buckets.totalCount;
   }
 
   /** Returns a copy of this bucket. */
@@ -53,6 +56,7 @@ DoubleExponentialHistogramBuckets copy() {
 
   /** Resets all counters in this bucket set to zero, but preserves scale. */
   public void clear() {
+    this.totalCount = 0;
     this.counts.clear();
   }
 
@@ -62,7 +66,11 @@ boolean record(double value) {
       throw new IllegalStateException("Illegal attempted recording of zero at bucket level.");
     }
     int index = valueToIndex(value);
-    return this.counts.increment(index, 1);
+    boolean recordingSuccessful = this.counts.increment(index, 1);
+    if (recordingSuccessful) {
+      totalCount++;
+    }
+    return recordingSuccessful;
   }
 
   @Override
@@ -92,13 +100,6 @@ public List<Long> getBucketCounts() {
 
   @Override
   public long getTotalCount() {
-    if (counts.isEmpty()) {
-      return 0;
-    }
-    long totalCount = 0;
-    for (int i = counts.getIndexStart(); i <= counts.getIndexEnd(); i++) {
-      totalCount += counts.get(i);
-    }
     return totalCount;
   }
 
@@ -219,6 +220,7 @@ private void mergeWith(DoubleExponentialHistogramBuckets other, boolean additive
         throw new IllegalStateException("Failed to merge exponential histogram buckets.");
       }
     }
+    this.totalCount += sign * other.totalCount;
   }
 
   int getScale() {
@@ -306,7 +308,20 @@ public boolean equals(@Nullable Object obj) {
    * </ul>
    */
   private boolean sameBucketCounts(DoubleExponentialHistogramBuckets other) {
+    if (this.totalCount != other.totalCount) {
+      return false;
+    }
     int min = Math.min(this.counts.getIndexStart(), other.counts.getIndexStart());
+
+    // This check is so we avoid iterating from Integer.MIN_VALUE.
+    // In this case, we can assume that those buckets are empty.
+    // We start iterating from the other bucket index instead.
+    // They still may be equal as it is possible for another set of buckets
+    // to be empty but have a higher start index.
+    if (min == Integer.MIN_VALUE) {
+      min = Math.max(this.counts.getIndexStart(), other.counts.getIndexStart());
+    }
+
     int max = Math.max(this.counts.getIndexEnd(), other.counts.getIndexEnd());
     for (int idx = min; idx <= max; idx++) {
       if (this.counts.get(idx) != other.counts.get(idx)) {

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

@@ -154,6 +154,7 @@ void testRecordingsAtLimits(DoubleExponentialHistogramAggregator aggregator) {
     assertThat(bucketCounts.get(bucketCounts.size() - 1)).isEqualTo(1);
     assertThat(bucketCounts.stream().filter(i -> i == 0).count())
         .isEqualTo(bucketCounts.size() - 2);
+    assertThat(acc.getPositiveBuckets().getTotalCount()).isEqualTo(2);
 
     // With 320 buckets allowed, minimum scale is -3
     assertThat(acc.getScale()).isEqualTo(-3);
@@ -263,8 +264,8 @@ void diffAccumulation() {
     // Note: This test relies on implementation details of ExponentialCounter, specifically it
     // assumes that an Array of all zeros is the same as an empty counter array for negative
     // buckets.
-    assertThat(aggregator.diff(previousAccumulation, nextAccumulation))
-        .isEqualTo(getTestAccumulation(exemplars, 0, 1));
+    ExponentialHistogramAccumulation diff = aggregator.diff(previousAccumulation, nextAccumulation);
+    assertThat(diff).isEqualTo(getTestAccumulation(exemplars, 0, 1));
   }
 
   @Test
@@ -403,6 +404,7 @@ void testInsert1M() {
     ExponentialHistogramAccumulation acc = handle.accumulateThenReset(Attributes.empty());
     assertThat(Objects.requireNonNull(acc).getScale()).isEqualTo(4);
     assertThat(acc.getPositiveBuckets().getBucketCounts().size()).isEqualTo(320);
+    assertThat(acc.getPositiveBuckets().getTotalCount()).isEqualTo(n);
   }
 
   @Test
@@ -424,6 +426,7 @@ void testDownScale() {
     assertThat(acc.getSum()).isEqualTo(23.5);
     assertThat(buckets.getOffset()).isEqualTo(-1);
     assertThat(buckets.getBucketCounts()).isEqualTo(Arrays.asList(1L, 1L, 1L, 1L, 0L, 1L));
+    assertThat(buckets.getTotalCount()).isEqualTo(5);
   }
 
   @Test