fix(sdk-metrics-base): coerce histogram boundaries to be implicit Infinity (#2859)

Co-authored-by: Valentin Marchaud <contact@vmarchaud.fr>

experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusSerializer.test.ts

@@ -22,7 +22,7 @@ import {
   MetricReader,
   DataPoint,
   DataPointType,
-  HistogramAggregation,
+  ExplicitBucketHistogramAggregation,
   SumAggregation,
   Histogram,
 } from '@opentelemetry/sdk-metrics-base-wip';
@@ -109,7 +109,7 @@ describe('PrometheusSerializer', () => {
         const reader = new TestMetricReader();
         const meterProvider = new MeterProvider();
         meterProvider.addMetricReader(reader);
-        meterProvider.addView({ aggregation: new HistogramAggregation([1, 10, 100]) });
+        meterProvider.addView({ aggregation: new ExplicitBucketHistogramAggregation([1, 10, 100]) });
         const meter = meterProvider.getMeter('test');
 
         const histogram = meter.createHistogram('test');
@@ -208,12 +208,12 @@ describe('PrometheusSerializer', () => {
       });
     });
 
-    describe('with HistogramAggregator', () => {
+    describe('with ExplicitBucketHistogramAggregation', () => {
       async function testSerializer(serializer: PrometheusSerializer) {
         const reader = new TestMetricReader();
         const meterProvider = new MeterProvider();
         meterProvider.addMetricReader(reader);
-        meterProvider.addView({ aggregation: new HistogramAggregation([1, 10, 100]) });
+        meterProvider.addView({ aggregation: new ExplicitBucketHistogramAggregation([1, 10, 100]) });
         const meter = meterProvider.getMeter('test');
 
         const histogram = meter.createHistogram('test', {
@@ -235,7 +235,7 @@ describe('PrometheusSerializer', () => {
         return result;
       }
 
-      it('serialize metric record with HistogramAggregator aggregator, cumulative', async () => {
+      it('serialize metric record with ExplicitHistogramAggregation aggregator, cumulative', async () => {
         const serializer = new PrometheusSerializer();
         const result = await testSerializer(serializer);
         assert.strictEqual(

experimental/packages/opentelemetry-sdk-metrics-base/src/aggregator/Histogram.ts

@@ -27,10 +27,12 @@ import { InstrumentDescriptor } from '../InstrumentDescriptor';
 import { Maybe } from '../utils';
 
 function createNewEmptyCheckpoint(boundaries: number[]): Histogram {
+  const counts = boundaries.map(() => 0);
+  counts.push(0);
   return {
     buckets: {
       boundaries,
-      counts: boundaries.map(() => 0).concat([0]),
+      counts,
     },
     sum: 0,
     count: 0,
@@ -68,16 +70,11 @@ export class HistogramAccumulation implements Accumulation {
  */
 export class HistogramAggregator implements Aggregator<HistogramAccumulation> {
   public kind: AggregatorKind.HISTOGRAM = AggregatorKind.HISTOGRAM;
-  private readonly _boundaries: number[];
 
-  constructor(boundaries: number[]) {
-    if (boundaries === undefined || boundaries.length === 0) {
-      throw new Error('HistogramAggregator should be created with boundaries.');
-    }
-    // we need to an ordered set to be able to correctly compute count for each
-    // boundary since we'll iterate on each in order.
-    this._boundaries = boundaries.sort((a, b) => a - b);
-  }
+  /**
+   * @param _boundaries upper bounds of recorded values.
+   */
+  constructor(private readonly _boundaries: number[]) {}
 
   createAccumulation() {
     return new HistogramAccumulation(this._boundaries);

experimental/packages/opentelemetry-sdk-metrics-base/src/view/Aggregation.ts

@@ -83,14 +83,9 @@ export class LastValueAggregation extends Aggregation {
  * The default histogram aggregation.
  */
 export class HistogramAggregation extends Aggregation {
-  private DEFAULT_INSTANCE: HistogramAggregator;
-  constructor(boundaries: number[] = [0, 5, 10, 25, 50, 75, 100, 250, 500, 1000]) {
-    super();
-    this.DEFAULT_INSTANCE = new HistogramAggregator(boundaries);
-  }
-
+  private static DEFAULT_INSTANCE = new HistogramAggregator([0, 5, 10, 25, 50, 75, 100, 250, 500, 1000]);
   createAggregator(_instrument: InstrumentDescriptor) {
-    return this.DEFAULT_INSTANCE;
+    return HistogramAggregation.DEFAULT_INSTANCE;
   }
 }
 
@@ -98,11 +93,27 @@ export class HistogramAggregation extends Aggregation {
  * The explicit bucket histogram aggregation.
  */
 export class ExplicitBucketHistogramAggregation extends Aggregation {
+  private _boundaries: number[];
   /**
-   * @param _boundaries the bucket boundaries of the histogram aggregation
+   * @param boundaries the bucket boundaries of the histogram aggregation
    */
-  constructor(private _boundaries: number[]) {
+  constructor(boundaries: number[]) {
     super();
+    if (boundaries === undefined || boundaries.length === 0) {
+      throw new Error('HistogramAggregator should be created with boundaries.');
+    }
+    // Copy the boundaries array for modification.
+    boundaries = boundaries.concat();
+    // We need to an ordered set to be able to correctly compute count for each
+    // boundary since we'll iterate on each in order.
+    boundaries = boundaries.sort((a, b) => a - b);
+    // Remove all Infinity from the boundaries.
+    const minusInfinityIndex = boundaries.lastIndexOf(-Infinity);
+    let infinityIndex: number | undefined = boundaries.indexOf(Infinity);
+    if (infinityIndex === -1) {
+      infinityIndex = undefined;
+    }
+    this._boundaries = boundaries.slice(minusInfinityIndex + 1, infinityIndex);
   }
 
   createAggregator(_instrument: InstrumentDescriptor) {

experimental/packages/opentelemetry-sdk-metrics-base/test/view/Aggregation.test.ts

@@ -83,19 +83,47 @@ describe('DefaultAggregation', () => {
   });
 });
 
+describe('HistogramAggregator', () => {
+  describe('createAggregator', () => {
+    it('should create histogram aggregators with boundaries', () => {
+      const aggregator = new HistogramAggregation().createAggregator(defaultInstrumentDescriptor);
+      assert(aggregator instanceof HistogramAggregator);
+      assert.deepStrictEqual(aggregator['_boundaries'], [0, 5, 10, 25, 50, 75, 100, 250, 500, 1000]);
+    });
+  });
+});
+
 describe('ExplicitBucketHistogramAggregation', () => {
   it('construct without exceptions', () => {
-    const aggregation = new ExplicitBucketHistogramAggregation([1, 10, 100]);
+    const cases = [
+      [1, 10, 100],
+      [1, 10, 100, Infinity],
+      [-Infinity, 1, 10, 100],
+    ];
+    for (const boundaries of cases) {
+      const aggregation = new ExplicitBucketHistogramAggregation(boundaries);
+      assert(aggregation instanceof ExplicitBucketHistogramAggregation);
+      assert.deepStrictEqual(aggregation['_boundaries'], [1, 10, 100]);
+    }
+  });
+
+  it('constructor should not modify inputs', () => {
+    const boundaries = [100, 10, 1];
+    const aggregation = new ExplicitBucketHistogramAggregation(boundaries);
     assert(aggregation instanceof ExplicitBucketHistogramAggregation);
+    assert.deepStrictEqual(aggregation['_boundaries'], [1, 10, 100]);
+    assert.deepStrictEqual(boundaries, [100, 10, 1]);
   });
 
   describe('createAggregator', () => {
     it('should create histogram aggregators with boundaries', () => {
-      const aggregator1 = new ExplicitBucketHistogramAggregation([1, 10, 100]).createAggregator(defaultInstrumentDescriptor);
+      const aggregator1 = new ExplicitBucketHistogramAggregation([100, 10, 1]).createAggregator(defaultInstrumentDescriptor);
       assert(aggregator1 instanceof HistogramAggregator);
       assert.deepStrictEqual(aggregator1['_boundaries'], [1, 10, 100]);
 
-      const aggregator2 = new ExplicitBucketHistogramAggregation([10, 100, 1000]).createAggregator(defaultInstrumentDescriptor);
+      const aggregator2 = new ExplicitBucketHistogramAggregation(
+        [-Infinity, -Infinity, 10, 100, 1000, Infinity, Infinity]
+      ).createAggregator(defaultInstrumentDescriptor);
       assert(aggregator2 instanceof HistogramAggregator);
       assert.deepStrictEqual(aggregator2['_boundaries'], [10, 100, 1000]);
     });