chore: more cleanup

open-telemetry · Dec 22, 2022 · e293cdd · e293cdd
1 parent e73f159
commit e293cdd
Show file tree

Hide file tree

Showing 4 changed files with 19 additions and 14 deletions.
diff --git a/packages/sdk-metrics/src/aggregator/ExponentialHistogram.ts b/packages/sdk-metrics/src/aggregator/ExponentialHistogram.ts
@@ -207,7 +207,6 @@ export class ExponentialHistogramAccumulation implements Accumulation {
       this._min = value;
     }
 
-    // Note: Not checking for overflow here. TODO.
     this._count += increment;
 
     if (value === 0) {
@@ -411,6 +410,8 @@ export class ExponentialHistogramAccumulation implements Accumulation {
       return;
     }
     if (change < 0) {
+      // Note: this should be impossible. If we get here it's because
+      // there is a bug in the implementation.
       throw new Error(`impossible change of scale: ${this.scale}`);
     }
     const newScale = this._mapping.scale() - change;

diff --git a/packages/sdk-metrics/src/aggregator/exponential-histogram/Buckets.ts b/packages/sdk-metrics/src/aggregator/exponential-histogram/Buckets.ts
@@ -54,11 +54,7 @@ export class Buckets {
    * @returns {number} The logical counts based on the backing array
    */
   counts(): number[] {
-    const counts = new Array<number>(this.length());
-    for (let i = 0; i < this.length(); i++) {
-      counts[i] = this.at(i);
-    }
-    return counts;
+    return Array.from({ length: this.length() }, (_, i) => this.at(i));
   }
 
   /**
@@ -163,7 +159,7 @@ export class Buckets {
       }
       outpos++;
     }
-    // note `by` will always be > 0 and < 32-bits, so `>>` is safe to use
+
     this.indexStart >>= by;
     this.indexEnd >>= by;
     this.indexBase = this.indexStart;
@@ -288,7 +284,8 @@ class BucketsBacking {
     if (this._counts[bucketIndex] >= decrement) {
       this._counts[bucketIndex] -= decrement;
     } else {
-      // should not happen, todo: log
+      // this should not happen, but we're being defensive against
+      // negative counts.
       this._counts[bucketIndex] = 0;
     }
   }

diff --git a/packages/sdk-metrics/src/aggregator/exponential-histogram/util.ts b/packages/sdk-metrics/src/aggregator/exponential-histogram/util.ts
@@ -16,10 +16,8 @@
 
 /**
  * Note: other languages provide this as a built in function. This is
- * a naive, but functionally correct implementation. Ultimately, this is
- * used to compute the lower bucket boundaries for the Exponent and
- * Logarithm mappings. This would not run in normal application code
- * using the exponential histogram.
+ * a naive, but functionally correct implementation. This is used sparingly,
+ * when creating a new mapping in a running application.
  *
  * ldexp returns frac × 2**exp. With the following special cases:
  *   ldexp(±0, exp) = ±0

diff --git a/packages/sdk-metrics/test/aggregator/exponential-histogram/ExponentMapping.test.ts b/packages/sdk-metrics/test/aggregator/exponential-histogram/ExponentMapping.test.ts
@@ -145,7 +145,13 @@ describe('ExponentMapping', () => {
       [0x100000000000, 2],
       [0x1000000000000, 2], // 2**48
       [0x10000000000000, 3],
-      [0x1fffffffffffff, 3], // Number.MAX_SAFE_INTEGER
+      [0x1000000000000000, 3],
+      [0x10000000000000000, 3], // 2**64
+      [0x100000000000000000, 4],
+      [0x1000000000000000000, 4],
+      [0x10000000000000000000, 4],
+      [0x100000000000000000000, 4], // 2**80
+      [0x1000000000000000000000, 5],
 
       [1 / 0x1, -1],
       [1 / 0x10, -1],
@@ -161,7 +167,10 @@ describe('ExponentMapping', () => {
       [1 / 0x100000000000, -3],
       [1 / 0x1000000000000, -4], // 2**-48
       [1 / 0x10000000000000, -4],
-      [1 / 0x1fffffffffffff, -4], // Number.MAX_SAFE_INTEGER
+      [1 / 0x100000000000000, -4],
+      [1 / 0x1000000000000000, -4],
+      [1 / 0x10000000000000000, -5], // 2**-64
+      [1 / 0x100000000000000000, -5],
 
       // Max values
       // below is equivalent to [0x1.FFFFFFFFFFFFFp1023, 63],