feat(histogram): align collection of optional Histogram properties with spec (#3102)

Co-authored-by: Daniel Dyla <[email protected]>

Author: pichlermarc

experimental/CHANGELOG.md

@@ -10,6 +10,10 @@ All notable changes to experimental packages in this project will be documented
 * feat(sdk-metrics-base): split up Singular into Sum and Gauge in MetricData [#3079](https://github.com/open-telemetry/opentelemetry-js/pull/3079) @pichlermarc
   * removes `DataPointType.SINGULAR`, and replaces it with `DataPointType.SUM` and `DataPointType.GAUGE`
   * removes `SingularMetricData` and replaces it with `SumMetricData` (including an additional `isMonotonic` flag) and `GaugeMetricData`
+* feat(histogram): align collection of optional Histogram properties with spec [#3102](https://github.com/open-telemetry/opentelemetry-js/pull/3079) @pichlermarc
+  * changes type of `sum` property on `Histogram` to `number | undefined`
+  * changes type of `min` and `max` properties on `Histogram` to `number | undefined`
+  * removes `hasMinMax` flag on the exported `Histogram` - this is now indicated by `min` and `max` being `undefined`
 
 ### :rocket: (Enhancement)
 

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

@@ -24,7 +24,10 @@ import {
   DataPoint,
   Histogram,
 } from '@opentelemetry/sdk-metrics-base';
-import type { MetricAttributes, MetricAttributeValue } from '@opentelemetry/api-metrics';
+import type {
+  MetricAttributes,
+  MetricAttributeValue
+} from '@opentelemetry/api-metrics';
 import { hrTimeToMilliseconds } from '@opentelemetry/core';
 
 type PrometheusDataTypeLiteral =
@@ -52,6 +55,7 @@ function escapeAttributeValue(str: MetricAttributeValue = '') {
 }
 
 const invalidCharacterRegex = /[^a-z0-9_]/gi;
+
 /**
  * Ensures metric names are valid Prometheus metric names by removing
  * characters allowed by OpenTelemetry but disallowed by Prometheus.
@@ -254,25 +258,28 @@ export class PrometheusSerializer {
     let results = '';
 
     name = enforcePrometheusNamingConvention(name, type);
-    const { value, attributes } = dataPoint;
+    const attributes = dataPoint.attributes;
+    const histogram = dataPoint.value;
     const timestamp = hrTimeToMilliseconds(dataPoint.endTime);
     /** Histogram["bucket"] is not typed with `number` */
     for (const key of ['count', 'sum'] as ('count' | 'sum')[]) {
-      results += stringify(
-        name + '_' + key,
-        attributes,
-        value[key],
-        this._appendTimestamp ? timestamp : undefined,
-        undefined
-      );
+      const value = histogram[key];
+      if (value != null)
+        results += stringify(
+          name + '_' + key,
+          attributes,
+          value,
+          this._appendTimestamp ? timestamp : undefined,
+          undefined
+        );
     }
 
     let cumulativeSum = 0;
-    const countEntries = value.buckets.counts.entries();
+    const countEntries = histogram.buckets.counts.entries();
     let infiniteBoundaryDefined = false;
     for (const [idx, val] of countEntries) {
       cumulativeSum += val;
-      const upperBound = value.buckets.boundaries[idx];
+      const upperBound = histogram.buckets.boundaries[idx];
       /** HistogramAggregator is producing different boundary output -
        * in one case not including infinity values, in other -
        * full, e.g. [0, 100] and [0, 100, Infinity]

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

@@ -355,7 +355,7 @@ describe('PrometheusSerializer', () => {
         return result;
       }
 
-      it('serialize metric record with ExplicitHistogramAggregation aggregator, cumulative', async () => {
+      it('serialize cumulative metric record', async () => {
         const serializer = new PrometheusSerializer();
         const result = await testSerializer(serializer);
         assert.strictEqual(
@@ -376,6 +376,53 @@ describe('PrometheusSerializer', () => {
             `test_bucket{val="2",le="+Inf"} 1 ${mockedHrTimeMs}\n`
         );
       });
+
+      it('serialize cumulative metric record on instrument that allows negative values', async () => {
+        const serializer = new PrometheusSerializer();
+        const reader = new TestMetricReader();
+        const meterProvider = new MeterProvider({
+          views: [
+            new View({
+              aggregation: new ExplicitBucketHistogramAggregation([1, 10, 100]),
+              instrumentName: '*'
+            })
+          ]
+        });
+        meterProvider.addMetricReader(reader);
+        const meter = meterProvider.getMeter('test');
+
+        const upDownCounter = meter.createUpDownCounter('test', {
+          description: 'foobar',
+        });
+        upDownCounter.add(5, { val: '1' });
+        upDownCounter.add(50, { val: '1' });
+        upDownCounter.add(120, { val: '1' });
+
+        upDownCounter.add(5, { val: '2' });
+
+        const { resourceMetrics, errors } = await reader.collect();
+        assert.strictEqual(errors.length, 0);
+        assert.strictEqual(resourceMetrics.scopeMetrics.length, 1);
+        assert.strictEqual(resourceMetrics.scopeMetrics[0].metrics.length, 1);
+        const scopeMetrics = resourceMetrics.scopeMetrics[0];
+
+        const result = serializer['_serializeScopeMetrics'](scopeMetrics);
+        assert.strictEqual(
+          result,
+          '# HELP test foobar\n' +
+          '# TYPE test histogram\n' +
+          `test_count{val="1"} 3 ${mockedHrTimeMs}\n` +
+          `test_bucket{val="1",le="1"} 0 ${mockedHrTimeMs}\n` +
+          `test_bucket{val="1",le="10"} 1 ${mockedHrTimeMs}\n` +
+          `test_bucket{val="1",le="100"} 2 ${mockedHrTimeMs}\n` +
+          `test_bucket{val="1",le="+Inf"} 3 ${mockedHrTimeMs}\n` +
+          `test_count{val="2"} 1 ${mockedHrTimeMs}\n` +
+          `test_bucket{val="2",le="1"} 0 ${mockedHrTimeMs}\n` +
+          `test_bucket{val="2",le="10"} 1 ${mockedHrTimeMs}\n` +
+          `test_bucket{val="2",le="100"} 1 ${mockedHrTimeMs}\n` +
+          `test_bucket{val="2",le="+Inf"} 1 ${mockedHrTimeMs}\n`
+        );
+      });
     });
   });
 

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

@@ -18,16 +18,38 @@ import {
   Accumulation,
   AccumulationRecord,
   Aggregator,
-  AggregatorKind,
-  Histogram,
+  AggregatorKind
 } from './types';
-import { HistogramMetricData, DataPointType } from '../export/MetricData';
+import {
+  DataPointType,
+  HistogramMetricData
+} from '../export/MetricData';
 import { HrTime } from '@opentelemetry/api';
-import { InstrumentDescriptor } from '../InstrumentDescriptor';
+import {
+  InstrumentDescriptor,
+  InstrumentType
+} from '../InstrumentDescriptor';
 import { Maybe } from '../utils';
 import { AggregationTemporality } from '../export/AggregationTemporality';
 
-function createNewEmptyCheckpoint(boundaries: number[]): Histogram {
+/**
+ * Internal value type for HistogramAggregation.
+ * Differs from the exported type as undefined sum/min/max complicate arithmetic
+ * performed by this aggregation, but are required to be undefined in the exported types.
+ */
+interface InternalHistogram {
+  buckets: {
+    boundaries: number[];
+    counts: number[];
+  };
+  sum: number;
+  count: number;
+  hasMinMax: boolean;
+  min: number;
+  max: number;
+}
+
+function createNewEmptyCheckpoint(boundaries: number[]): InternalHistogram {
   const counts = boundaries.map(() => 0);
   counts.push(0);
   return {
@@ -48,7 +70,7 @@ export class HistogramAccumulation implements Accumulation {
     public startTime: HrTime,
     private readonly _boundaries: number[],
     private _recordMinMax = true,
-    private _current: Histogram = createNewEmptyCheckpoint(_boundaries)
+    private _current: InternalHistogram = createNewEmptyCheckpoint(_boundaries)
   ) {}
 
   record(value: number): void {
@@ -75,7 +97,7 @@ export class HistogramAccumulation implements Accumulation {
     this.startTime = startTime;
   }
 
-  toPointValue(): Histogram {
+  toPointValue(): InternalHistogram {
     return this._current;
   }
 }
@@ -181,11 +203,25 @@ export class HistogramAggregator implements Aggregator<HistogramAccumulation> {
       aggregationTemporality,
       dataPointType: DataPointType.HISTOGRAM,
       dataPoints: accumulationByAttributes.map(([attributes, accumulation]) => {
+        const pointValue = accumulation.toPointValue();
+
+        // determine if instrument allows negative values.
+        const allowsNegativeValues =
+          (descriptor.type === InstrumentType.UP_DOWN_COUNTER) ||
+          (descriptor.type === InstrumentType.OBSERVABLE_GAUGE) ||
+          (descriptor.type === InstrumentType.OBSERVABLE_UP_DOWN_COUNTER);
+
         return {
           attributes,
           startTime: accumulation.startTime,
           endTime,
-          value: accumulation.toPointValue(),
+          value: {
+            min: pointValue.hasMinMax ? pointValue.min : undefined,
+            max: pointValue.hasMinMax ? pointValue.max : undefined,
+            sum: !allowsNegativeValues ? pointValue.sum : undefined,
+            buckets: pointValue.buckets,
+            count: pointValue.count
+          },
         };
       })
     };

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

@@ -57,11 +57,10 @@ export interface Histogram {
     boundaries: number[];
     counts: number[];
   };
-  sum: number;
+  sum?: number;
   count: number;
-  hasMinMax: boolean;
-  min: number;
-  max: number;
+  min?: number;
+  max?: number;
 }
 
 /**

experimental/packages/opentelemetry-sdk-metrics-base/src/export/MetricData.ts

@@ -19,8 +19,8 @@ import { MetricAttributes } from '@opentelemetry/api-metrics';
 import { InstrumentationScope } from '@opentelemetry/core';
 import { Resource } from '@opentelemetry/resources';
 import { InstrumentDescriptor } from '../InstrumentDescriptor';
-import { Histogram } from '../aggregator/types';
 import { AggregationTemporality } from './AggregationTemporality';
+import { Histogram } from '../aggregator/types';
 
 /**
  * Basic metric data fields.

experimental/packages/opentelemetry-sdk-metrics-base/test/Instruments.test.ts

@@ -25,7 +25,8 @@ import {
   MeterProvider,
   MetricReader,
   DataPoint,
-  DataPointType
+  DataPointType,
+  Histogram
 } from '../src';
 import { TestMetricReader } from './export/TestMetricReader';
 import {
@@ -36,7 +37,6 @@ import {
   defaultResource,
   defaultInstrumentationScope
 } from './util';
-import { Histogram } from '../src/aggregator/types';
 import { ObservableResult, ValueType } from '@opentelemetry/api-metrics';
 
 describe('Instruments', () => {
@@ -301,7 +301,6 @@ describe('Instruments', () => {
               },
               count: 2,
               sum: 10,
-              hasMinMax: true,
               max: 10,
               min: 0
             },
@@ -315,7 +314,6 @@ describe('Instruments', () => {
               },
               count: 2,
               sum: 100,
-              hasMinMax: true,
               max: 100,
               min: 0
             },
@@ -358,7 +356,6 @@ describe('Instruments', () => {
               },
               count: 2,
               sum: 110,
-              hasMinMax: true,
               min: 20,
               max: 90
             },
@@ -386,7 +383,6 @@ describe('Instruments', () => {
               },
               count: 4,
               sum: 220,
-              hasMinMax: true,
               min: 10,
               max: 100
             },
@@ -430,7 +426,6 @@ describe('Instruments', () => {
               },
               count: 2,
               sum: 10.1,
-              hasMinMax: true,
               max: 10,
               min: 0.1
             },
@@ -444,7 +439,6 @@ describe('Instruments', () => {
               },
               count: 2,
               sum: 100.1,
-              hasMinMax: true,
               max: 100,
               min: 0.1
             },