From 5687fb12860b7fad5d533f93627198be1d13e002 Mon Sep 17 00:00:00 2001 From: "marc.pichler" Date: Mon, 30 May 2022 14:37:25 +0200 Subject: [PATCH 1/4] fix(sdk-metrics-base): only record non-negative histogram values. --- .../opentelemetry-sdk-metrics-base/src/Instruments.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/Instruments.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/Instruments.ts index 44467cc7d82..3cd1f0f15ba 100644 --- 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); } } From b67b462b6f85a770f7e2a32c6b38ff6a09549c27 Mon Sep 17 00:00:00 2001 From: "marc.pichler" Date: Mon, 30 May 2022 14:40:08 +0200 Subject: [PATCH 2/4] fix(sdk-metrics-base): adapt tests to use non-negative values. --- .../test/Instruments.test.ts | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/test/Instruments.test.ts b/experimental/packages/opentelemetry-sdk-metrics-base/test/Instruments.test.ts index c0d7d77dfba..8719b69ed59 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/test/Instruments.test.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/test/Instruments.test.ts @@ -257,10 +257,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', @@ -305,9 +305,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,10 +316,10 @@ 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, }, }, { @@ -327,10 +327,10 @@ describe('Instruments', () => { 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, }, }, ], From ed386108ad7505c8b616c1eb02b154d72223b39f Mon Sep 17 00:00:00 2001 From: "marc.pichler" Date: Mon, 30 May 2022 14:49:13 +0200 Subject: [PATCH 3/4] fix(sdk-metrics-base): add test that negative histogram values are dropped. --- .../test/Instruments.test.ts | 44 ++++++++++++++++++- 1 file changed, 42 insertions(+), 2 deletions(-) diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/test/Instruments.test.ts b/experimental/packages/opentelemetry-sdk-metrics-base/test/Instruments.test.ts index 8719b69ed59..b6301f5c64d 100644 --- 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'; @@ -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(); @@ -336,6 +363,19 @@ describe('Instruments', () => { ], }); }); + + 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', () => { From f04f8cd8aa2939dc848977d70359fe25fcdab0aa Mon Sep 17 00:00:00 2001 From: "marc.pichler" Date: Mon, 30 May 2022 15:06:04 +0200 Subject: [PATCH 4/4] fix(changelog): add changelog entry. --- experimental/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md index f24fbe0ea0f..20581c11835 100644 --- 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)