feat(sdk-metrics-base): implement min/max recording for Histograms (#3032)

pichlermarc · dyladan · Flarna · web-flow · commit d2de661de097 · 2022-06-24T08:52:42.000-04:00
* feat(sdk-metrics-base): prepare min/max collection.

* chore: updating submodule for otlp-grpc-exporter-base

* chore: updating submodule for otlp-proto-exporter-base

* feat(sdk-trace-base): implement min/max recording, add tests.

* feat(docs): update metrics exporter compatibility.

* fix(changelog): add changelog entry.

* fix(changelog): add more detail to changelog.

* fix(changelog): reword, appease linter.

* refactor(sdk-metrics-base): inline hasMinMax(), reformat.

* fix(otlp-transformer): simplify distinction with for data with min/max and without.

Co-authored-by: Daniel Dyla &lt;dyladan@users.noreply.github.com&gt;
Co-authored-by: Gerhard Stöbich &lt;deb2001-github@yahoo.de&gt;
diff --git a/examples/otlp-exporter-node/docker/docker-compose.yaml b/examples/otlp-exporter-node/docker/docker-compose.yaml
@@ -2,7 +2,7 @@ version: "3"
 services:
   # Collector
   collector:
-    image: otel/opentelemetry-collector-contrib:0.50.0
+    image: otel/opentelemetry-collector-contrib:0.53.0
 #    image: otel/opentelemetry-collector-contrib:latest
     command: ["--config=/conf/collector-config.yaml"]
     volumes:
diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md
@@ -7,6 +7,11 @@ All notable changes to experimental packages in this project will be documented
 ### :boom: Breaking Change
 
 * fix: remove aws and gcp detector from SDK #3024 @flarna
+* feat(sdk-metrics-base): implement min/max recording for Histograms #3032 @pichlermarc
+  * adds `min`/`max` recording to Histograms
+  * updates [opentelemetry-proto](https://github.com/open-telemetry/opentelemetry-proto) to `0.18` so that `min` and
+    `max` can be exported. This change breaks the OTLP/JSON Metric Exporter for all collector versions `<0.52` due to
+    [open-telemetry/opentelemetry-collector#5312](https://github.com/open-telemetry/opentelemetry-collector/issues/5312).
 
 ### :rocket: (Enhancement)
 
diff --git a/experimental/packages/opentelemetry-exporter-metrics-otlp-grpc/README.md b/experimental/packages/opentelemetry-exporter-metrics-otlp-grpc/README.md
@@ -4,7 +4,7 @@
 [![Apache License][license-image]][license-image]
 
 This module provides exporter for web and node to be used with [opentelemetry-collector][opentelemetry-collector-url].
-Compatible with [opentelemetry-collector][opentelemetry-collector-url] versions `>=0.16 <=0.50`.
+Compatible with [opentelemetry-collector][opentelemetry-collector-url] versions `>=0.16 <=0.53`.
 
 ## Installation
 
diff --git a/experimental/packages/opentelemetry-exporter-metrics-otlp-grpc/test/metricsHelper.ts b/experimental/packages/opentelemetry-exporter-metrics-otlp-grpc/test/metricsHelper.ts
@@ -196,8 +196,12 @@ export function ensureExportedHistogramIsCorrect(
           exemplars: [],
           flags: 0,
           _sum: 'sum',
+          _min: 'min',
+          _max: 'max',
           sum: 21,
           count: '2',
+          min: 7,
+          max: 14,
           startTimeUnixNano: String(startTime),
           timeUnixNano: String(time),
           bucketCounts,
diff --git a/experimental/packages/opentelemetry-exporter-metrics-otlp-http/README.md b/experimental/packages/opentelemetry-exporter-metrics-otlp-http/README.md
@@ -4,7 +4,7 @@
 [![Apache License][license-image]][license-image]
 
 This module provides exporter for web and node to be used with [opentelemetry-collector][opentelemetry-collector-url].
-Compatible with [opentelemetry-collector][opentelemetry-collector-url] versions `>=0.48 <=0.50`.
+Compatible with [opentelemetry-collector][opentelemetry-collector-url] versions `>=0.52 <=0.53`.
 
 ## Installation
 
diff --git a/experimental/packages/opentelemetry-exporter-metrics-otlp-http/test/metricsHelper.ts b/experimental/packages/opentelemetry-exporter-metrics-otlp-http/test/metricsHelper.ts
@@ -370,6 +370,8 @@ export function ensureHistogramIsCorrect(
           attributes: [],
           sum: 21,
           count: 2,
+          min: 7,
+          max: 14,
           startTimeUnixNano: startTime,
           timeUnixNano: time,
           bucketCounts,
diff --git a/experimental/packages/opentelemetry-exporter-metrics-otlp-proto/README.md b/experimental/packages/opentelemetry-exporter-metrics-otlp-proto/README.md
@@ -4,7 +4,7 @@
 [![Apache License][license-image]][license-image]
 
 This module provides exporter for node to be used with [opentelemetry-collector][opentelemetry-collector-url].
-Compatible with [opentelemetry-collector][opentelemetry-collector-url] versions `>=0.32 <=0.50`.
+Compatible with [opentelemetry-collector][opentelemetry-collector-url] versions `>=0.32 <=0.53`.
 
 ## Installation
 
diff --git a/experimental/packages/opentelemetry-exporter-metrics-otlp-proto/test/metricsHelper.ts b/experimental/packages/opentelemetry-exporter-metrics-otlp-proto/test/metricsHelper.ts
@@ -187,6 +187,8 @@ export function ensureExportedHistogramIsCorrect(
         {
           sum: 21,
           count: '2',
+          min: 7,
+          max: 14,
           startTimeUnixNano: String(startTime),
           timeUnixNano: String(time),
           bucketCounts,
diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/aggregator/Histogram.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/aggregator/Histogram.ts
@@ -37,19 +37,29 @@ function createNewEmptyCheckpoint(boundaries: number[]): Histogram {
     },
     sum: 0,
     count: 0,
+    hasMinMax: false,
+    min: Infinity,
+    max: -1
   };
 }
 
 export class HistogramAccumulation implements Accumulation {
   constructor(
     private readonly _boundaries: number[],
+    private _recordMinMax = true,
     private _current: Histogram = createNewEmptyCheckpoint(_boundaries)
   ) {}
 
   record(value: number): void {
     this._current.count += 1;
     this._current.sum += value;
 
+    if (this._recordMinMax) {
+      this._current.min = Math.min(value, this._current.min);
+      this._current.max = Math.max(value, this._current.max);
+      this._current.hasMinMax = true;
+    }
+
     for (let i = 0; i < this._boundaries.length; i++) {
       if (value < this._boundaries[i]) {
         this._current.buckets.counts[i] += 1;
@@ -74,11 +84,12 @@ export class HistogramAggregator implements Aggregator<HistogramAccumulation> {
 
   /**
    * @param _boundaries upper bounds of recorded values.
+   * @param _recordMinMax If set to true, min and max will be recorded. Otherwise, min and max will not be recorded.
    */
-  constructor(private readonly _boundaries: number[]) {}
+  constructor(private readonly _boundaries: number[], private readonly _recordMinMax: boolean) {}
 
   createAccumulation() {
-    return new HistogramAccumulation(this._boundaries);
+    return new HistogramAccumulation(this._boundaries, this._recordMinMax);
   }
 
   /**
@@ -98,13 +109,32 @@ export class HistogramAggregator implements Aggregator<HistogramAccumulation> {
       mergedCounts[idx] = previousCounts[idx] + deltaCounts[idx];
     }
 
-    return new HistogramAccumulation(previousValue.buckets.boundaries, {
+    let min = Infinity;
+    let max = -1;
+
+    if (this._recordMinMax) {
+      if (previousValue.hasMinMax && deltaValue.hasMinMax) {
+        min = Math.min(previousValue.min, deltaValue.min);
+        max = Math.max(previousValue.max, deltaValue.max);
+      } else if (previousValue.hasMinMax) {
+        min = previousValue.min;
+        max = previousValue.max;
+      } else if (deltaValue.hasMinMax) {
+        min = deltaValue.min;
+        max = deltaValue.max;
+      }
+    }
+
+    return new HistogramAccumulation(previousValue.buckets.boundaries, this._recordMinMax, {
       buckets: {
         boundaries: previousValue.buckets.boundaries,
         counts: mergedCounts,
       },
       count: previousValue.count + deltaValue.count,
       sum: previousValue.sum + deltaValue.sum,
+      hasMinMax: this._recordMinMax && (previousValue.hasMinMax || deltaValue.hasMinMax),
+      min: min,
+      max: max
     });
   }
 
@@ -123,13 +153,16 @@ export class HistogramAggregator implements Aggregator<HistogramAccumulation> {
       diffedCounts[idx] = currentCounts[idx] - previousCounts[idx];
     }
 
-    return new HistogramAccumulation(previousValue.buckets.boundaries, {
+    return new HistogramAccumulation(previousValue.buckets.boundaries, this._recordMinMax, {
       buckets: {
         boundaries: previousValue.buckets.boundaries,
         counts: diffedCounts,
       },
       count: currentValue.count - previousValue.count,
       sum: currentValue.sum - previousValue.sum,
+      hasMinMax: false,
+      min: Infinity,
+      max: -1
     });
   }
 
diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/aggregator/types.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/aggregator/types.ts
@@ -59,6 +59,9 @@ export interface Histogram {
   };
   sum: number;
   count: number;
+  hasMinMax: boolean;
+  min: number;
+  max: number;
 }
 
 /**
diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/view/Aggregation.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/view/Aggregation.ts
@@ -83,7 +83,7 @@ export class LastValueAggregation extends Aggregation {
  * The default histogram aggregation.
  */
 export class HistogramAggregation extends Aggregation {
-  private static DEFAULT_INSTANCE = new HistogramAggregator([0, 5, 10, 25, 50, 75, 100, 250, 500, 1000]);
+  private static DEFAULT_INSTANCE = new HistogramAggregator([0, 5, 10, 25, 50, 75, 100, 250, 500, 1000], true);
   createAggregator(_instrument: InstrumentDescriptor) {
     return HistogramAggregation.DEFAULT_INSTANCE;
   }
@@ -94,10 +94,12 @@ export class HistogramAggregation extends Aggregation {
  */
 export class ExplicitBucketHistogramAggregation extends Aggregation {
   private _boundaries: number[];
+
   /**
    * @param boundaries the bucket boundaries of the histogram aggregation
+   * @param _recordMinMax If set to true, min and max will be recorded. Otherwise, min and max will not be recorded.
    */
-  constructor(boundaries: number[]) {
+  constructor(boundaries: number[], private readonly _recordMinMax = true) {
     super();
     if (boundaries === undefined || boundaries.length === 0) {
       throw new Error('HistogramAggregator should be created with boundaries.');
@@ -117,7 +119,7 @@ export class ExplicitBucketHistogramAggregation extends Aggregation {
   }
 
   createAggregator(_instrument: InstrumentDescriptor) {
-    return new HistogramAggregator(this._boundaries);
+    return new HistogramAggregator(this._boundaries, this._recordMinMax);
   }
 }
 
diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/test/Instruments.test.ts b/experimental/packages/opentelemetry-sdk-metrics-base/test/Instruments.test.ts
@@ -295,6 +295,9 @@ describe('Instruments', () => {
               },
               count: 2,
               sum: 10,
+              hasMinMax: true,
+              max: 10,
+              min: 0
             },
           },
           {
@@ -306,12 +309,86 @@ describe('Instruments', () => {
               },
               count: 2,
               sum: 100,
+              hasMinMax: true,
+              max: 100,
+              min: 0
             },
           },
         ],
       });
     });
 
+    it('should collect min and max', async () => {
+      const { meter, deltaReader, cumulativeReader } = setup();
+      const histogram = meter.createHistogram('test', {
+        valueType: ValueType.INT,
+      });
+
+      histogram.record(10);
+      histogram.record(100);
+      await deltaReader.collect();
+      await cumulativeReader.collect();
+
+      histogram.record(20);
+      histogram.record(90);
+
+      // Delta should only have min/max of values recorded after the collection.
+      await validateExport(deltaReader, {
+        descriptor: {
+          name: 'test',
+          description: '',
+          unit: '',
+          type: InstrumentType.HISTOGRAM,
+          valueType: ValueType.INT,
+        },
+        dataPointType: DataPointType.HISTOGRAM,
+        dataPoints: [
+          {
+            attributes: {},
+            value: {
+              buckets: {
+                boundaries: [0, 5, 10, 25, 50, 75, 100, 250, 500, 1000],
+                counts: [0, 0, 0, 1, 0, 0, 1, 0, 0, 0, 0],
+              },
+              count: 2,
+              sum: 110,
+              hasMinMax: true,
+              min: 20,
+              max: 90
+            },
+          }
+        ],
+      });
+
+      // Cumulative should have min/max of all recorded values.
+      await validateExport(cumulativeReader, {
+        descriptor: {
+          name: 'test',
+          description: '',
+          unit: '',
+          type: InstrumentType.HISTOGRAM,
+          valueType: ValueType.INT,
+        },
+        dataPointType: DataPointType.HISTOGRAM,
+        dataPoints: [
+          {
+            attributes: {},
+            value: {
+              buckets: {
+                boundaries: [0, 5, 10, 25, 50, 75, 100, 250, 500, 1000],
+                counts: [0, 0, 0, 2, 0, 0, 1, 1, 0, 0, 0],
+              },
+              count: 4,
+              sum: 220,
+              hasMinMax: true,
+              min: 10,
+              max: 100
+            },
+          }
+        ],
+      });
+    });
+
     it('should not record negative INT values', async () => {
       const { meter, deltaReader } = setup();
       const histogram = meter.createHistogram('test', {
@@ -347,6 +424,9 @@ describe('Instruments', () => {
               },
               count: 2,
               sum: 10.1,
+              hasMinMax: true,
+              max: 10,
+              min: 0.1
             },
           },
           {
@@ -358,6 +438,9 @@ describe('Instruments', () => {
               },
               count: 2,
               sum: 100.1,
+              hasMinMax: true,
+              max: 100,
+              min: 0.1
             },
           },
         ],
diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/test/aggregator/Histogram.test.ts b/experimental/packages/opentelemetry-sdk-metrics-base/test/aggregator/Histogram.test.ts
@@ -24,15 +24,15 @@ import { commonValues, defaultInstrumentDescriptor } from '../util';
 describe('HistogramAggregator', () => {
   describe('createAccumulation', () => {
     it('no exceptions on createAccumulation', () => {
-      const aggregator = new HistogramAggregator([1, 10, 100]);
+      const aggregator = new HistogramAggregator([1, 10, 100], true);
       const accumulation = aggregator.createAccumulation();
       assert(accumulation instanceof HistogramAccumulation);
     });
   });
 
   describe('merge', () => {
     it('no exceptions', () => {
-      const aggregator = new HistogramAggregator([1, 10, 100]);
+      const aggregator = new HistogramAggregator([1, 10, 100], true);
       const prev = aggregator.createAccumulation();
       prev.record(0);
       prev.record(1);
@@ -55,7 +55,7 @@ describe('HistogramAggregator', () => {
 
   describe('diff', () => {
     it('no exceptions', () => {
-      const aggregator = new HistogramAggregator([1, 10, 100]);
+      const aggregator = new HistogramAggregator([1, 10, 100], true);
       const prev = aggregator.createAccumulation();
       prev.record(0);
       prev.record(1);
@@ -68,13 +68,16 @@ describe('HistogramAggregator', () => {
       curr.record(2);
       curr.record(11);
 
-      const expected = new HistogramAccumulation([1, 10, 100], {
+      const expected = new HistogramAccumulation([1, 10, 100], true, {
         buckets: {
           boundaries: [1, 10, 100],
           counts: [0, 1, 1, 0],
         },
         count: 2,
         sum: 13,
+        hasMinMax: false,
+        min: Infinity,
+        max: -1
       });
 
       assert.deepStrictEqual(aggregator.diff(prev, curr), expected);
@@ -83,7 +86,7 @@ describe('HistogramAggregator', () => {
 
   describe('toMetricData', () => {
     it('transform without exception', () => {
-      const aggregator = new HistogramAggregator([1, 10, 100]);
+      const aggregator = new HistogramAggregator([1, 10, 100], true);
 
       const accumulation = aggregator.createAccumulation();
       accumulation.record(0);
@@ -108,6 +111,9 @@ describe('HistogramAggregator', () => {
               },
               count: 2,
               sum: 1,
+              hasMinMax: true,
+              min: 0,
+              max: 1
             },
           },
         ],
diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/test/view/Aggregation.test.ts b/experimental/packages/opentelemetry-sdk-metrics-base/test/view/Aggregation.test.ts
diff --git a/experimental/packages/otlp-grpc-exporter-base/protos b/experimental/packages/otlp-grpc-exporter-base/protos
diff --git a/experimental/packages/otlp-proto-exporter-base/protos b/experimental/packages/otlp-proto-exporter-base/protos
diff --git a/experimental/packages/otlp-transformer/src/metrics/internal.ts b/experimental/packages/otlp-transformer/src/metrics/internal.ts
diff --git a/experimental/packages/otlp-transformer/src/metrics/types.ts b/experimental/packages/otlp-transformer/src/metrics/types.ts
diff --git a/experimental/packages/otlp-transformer/test/metrics.test.ts b/experimental/packages/otlp-transformer/test/metrics.test.ts

Original file line number	Diff line number	Diff line change
`@@ -187,6 +187,8 @@ export function ensureExportedHistogramIsCorrect(`
`187`	`187`	`{`
`188`	`188`	`sum: 21,`
`189`	`189`	`count: '2',`
	`190`	`+ min: 7,`
	`191`	`+ max: 14,`
`190`	`192`	`startTimeUnixNano: String(startTime),`
`191`	`193`	`timeUnixNano: String(time),`
`192`	`194`	`bucketCounts,`
Original file line number	Diff line number	Diff line change
`@@ -59,6 +59,9 @@ export interface Histogram {`
`59`	`59`	`};`
`60`	`60`	`sum: number;`
`61`	`61`	`count: number;`
	`62`	`+ hasMinMax: boolean;`
	`63`	`+ min: number;`
	`64`	`+ max: number;`
`62`	`65`	`}`
`63`	`66`
`64`	`67`	`/**`