diff --git a/packages/sdk-metrics/src/aggregator/ExponentialHistogram.ts b/packages/sdk-metrics/src/aggregator/ExponentialHistogram.ts index 230d471699f..3edab1118c1 100644 --- 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 index 2bd8dd6a0c8..9c108a86b18 100644 --- 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(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 index 3d5beff3b44..d0f7edb6cae 100644 --- 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 index 876a92af014..47b160a28d2 100644 --- 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],