fix(sdk-metrics-base): only record non-negative histogram values. (#3002

)
open-telemetry · May 30, 2022 · fcef5e2 · fcef5e2
1 parent fd95d42
commit fcef5e2
Show file tree

Hide file tree

Showing 3 changed files with 57 additions and 11 deletions.
diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md
@@ -10,6 +10,8 @@ All notable changes to experimental packages in this project will be documented
 
 ### :bug: (Bug Fix)
 
+* fix(sdk-metrics-base): only record non-negative histogram values #3002 @pichlermarc
+
 ### :books: (Refine Doc)
 
 ### :house: (Internal)

diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/Instruments.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/Instruments.ts
@@ -72,6 +72,10 @@ export class HistogramInstrument extends SyncInstrument implements metrics.Histo
    * Records a measurement. Value of the measurement must not be negative.
    */
   record(value: number, attributes?: metrics.MetricAttributes, ctx?: api.Context): void {
+    if (value < 0) {
+      api.diag.warn(`negative value provided to histogram ${this._descriptor.name}: ${value}`);
+      return;
+    }
     this._record(value, attributes, ctx);
   }
 }

diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/test/Instruments.test.ts b/experimental/packages/opentelemetry-sdk-metrics-base/test/Instruments.test.ts
@@ -18,9 +18,24 @@ import * as assert from 'assert';
 import * as sinon from 'sinon';
 import { InstrumentationScope } from '@opentelemetry/core';
 import { Resource } from '@opentelemetry/resources';
-import { AggregationTemporality, InstrumentDescriptor, InstrumentType, MeterProvider, MetricReader, DataPoint, DataPointType } from '../src';
+import {
+  AggregationTemporality,
+  InstrumentDescriptor,
+  InstrumentType,
+  MeterProvider,
+  MetricReader,
+  DataPoint,
+  DataPointType
+} from '../src';
 import { TestMetricReader } from './export/TestMetricReader';
-import { assertMetricData, assertDataPoint, commonValues, commonAttributes, defaultResource, defaultInstrumentationScope } from './util';
+import {
+  assertMetricData,
+  assertDataPoint,
+  commonValues,
+  commonAttributes,
+  defaultResource,
+  defaultInstrumentationScope
+} from './util';
 import { Histogram } from '../src/aggregator/types';
 import { ObservableResult, ValueType } from '@opentelemetry/api-metrics';
 
@@ -257,10 +272,10 @@ describe('Instruments', () => {
       });
 
       histogram.record(10);
-      // -0.1 should be trunc-ed to -0
-      histogram.record(-0.1);
+      // 0.1 should be trunc-ed to 0
+      histogram.record(0.1);
       histogram.record(100, { foo: 'bar' });
-      histogram.record(-0.1, { foo: 'bar' });
+      histogram.record(0.1, { foo: 'bar' });
       await validateExport(deltaReader, {
         descriptor: {
           name: 'test',
@@ -297,6 +312,18 @@ describe('Instruments', () => {
       });
     });
 
+    it('should not record negative INT values', async () => {
+      const { meter, deltaReader } = setup();
+      const histogram = meter.createHistogram('test', {
+        valueType: ValueType.DOUBLE,
+      });
+
+      histogram.record(-1, { foo: 'bar' });
+      await validateExport(deltaReader, {
+        dataPointType: DataPointType.HISTOGRAM,
+        dataPoints: [],
+      });
+    });
 
     it('should record DOUBLE values', async () => {
       const { meter, deltaReader } = setup();
@@ -305,9 +332,9 @@ describe('Instruments', () => {
       });
 
       histogram.record(10);
-      histogram.record(-0.1);
+      histogram.record(0.1);
       histogram.record(100, { foo: 'bar' });
-      histogram.record(-0.1, { foo: 'bar' });
+      histogram.record(0.1, { foo: 'bar' });
       await validateExport(deltaReader, {
         dataPointType: DataPointType.HISTOGRAM,
         dataPoints: [
@@ -316,26 +343,39 @@ describe('Instruments', () => {
             value: {
               buckets: {
                 boundaries: [0, 5, 10, 25, 50, 75, 100, 250, 500, 1000],
-                counts: [1, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0],
+                counts: [0, 1, 0, 1, 0, 0, 0, 0, 0, 0, 0],
               },
               count: 2,
-              sum: 9.9,
+              sum: 10.1,
             },
           },
           {
             attributes: { foo: 'bar' },
             value: {
               buckets: {
                 boundaries: [0, 5, 10, 25, 50, 75, 100, 250, 500, 1000],
-                counts: [1, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0],
+                counts: [0, 1, 0, 0, 0, 0, 0, 1, 0, 0, 0],
               },
               count: 2,
-              sum: 99.9,
+              sum: 100.1,
             },
           },
         ],
       });
     });
+
+    it('should not record negative DOUBLE values', async () => {
+      const { meter, deltaReader } = setup();
+      const histogram = meter.createHistogram('test', {
+        valueType: ValueType.DOUBLE,
+      });
+
+      histogram.record(-0.5, { foo: 'bar' });
+      await validateExport(deltaReader, {
+        dataPointType: DataPointType.HISTOGRAM,
+        dataPoints: [],
+      });
+    });
   });
 
   describe('ObservableCounter', () => {