diff --git a/CHANGELOG.md b/CHANGELOG.md index c5f1569a88..0266669bdf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/ * fix(opentelemetry-exporter-logs-otlp-http): Add otel-api as dev dep for tests as they are directly importing the api and which is breaking the web-sandbox tests which is using rollup * fix(core): stop rounding to nearest int in hrTimeTo*seconds() functions [#4014](https://github.com/open-telemetry/opentelemetry-js/pull/4014/) @aabmass +* fix(sdk-metrics): ignore invalid metric values [#3988](https://github.com/open-telemetry/opentelemetry-js/pull/3988) @legendecas ### :books: (Refine Doc) diff --git a/packages/sdk-metrics/src/Instruments.ts b/packages/sdk-metrics/src/Instruments.ts index d99c621c0b..f665952f05 100644 --- a/packages/sdk-metrics/src/Instruments.ts +++ b/packages/sdk-metrics/src/Instruments.ts @@ -48,6 +48,12 @@ export class SyncInstrument { attributes: MetricAttributes = {}, context: Context = contextApi.active() ) { + if (typeof value !== 'number') { + diag.warn( + `non-number value provided to metric ${this._descriptor.name}: ${value}` + ); + return; + } if ( this._descriptor.valueType === ValueType.INT && !Number.isInteger(value) @@ -56,6 +62,10 @@ export class SyncInstrument { `INT value type cannot accept a floating-point value for ${this._descriptor.name}, ignoring the fractional digits.` ); value = Math.trunc(value); + // ignore non-finite values. + if (!Number.isInteger(value)) { + return; + } } this._writableMetricStorage.record( value, diff --git a/packages/sdk-metrics/src/ObservableResult.ts b/packages/sdk-metrics/src/ObservableResult.ts index 31e194290c..9298a46d93 100644 --- a/packages/sdk-metrics/src/ObservableResult.ts +++ b/packages/sdk-metrics/src/ObservableResult.ts @@ -41,6 +41,12 @@ export class ObservableResultImpl implements ObservableResult { * Observe a measurement of the value associated with the given attributes. */ observe(value: number, attributes: MetricAttributes = {}): void { + if (typeof value !== 'number') { + diag.warn( + `non-number value provided to metric ${this._descriptor.name}: ${value}` + ); + return; + } if ( this._descriptor.valueType === ValueType.INT && !Number.isInteger(value) @@ -49,6 +55,10 @@ export class ObservableResultImpl implements ObservableResult { `INT value type cannot accept a floating-point value for ${this._descriptor.name}, ignoring the fractional digits.` ); value = Math.trunc(value); + // ignore non-finite values. + if (!Number.isInteger(value)) { + return; + } } this._buffer.set(attributes, value); } @@ -79,6 +89,12 @@ export class BatchObservableResultImpl implements BatchObservableResult { map = new AttributeHashMap(); this._buffer.set(metric, map); } + if (typeof value !== 'number') { + diag.warn( + `non-number value provided to metric ${metric._descriptor.name}: ${value}` + ); + return; + } if ( metric._descriptor.valueType === ValueType.INT && !Number.isInteger(value) @@ -87,6 +103,10 @@ export class BatchObservableResultImpl implements BatchObservableResult { `INT value type cannot accept a floating-point value for ${metric._descriptor.name}, ignoring the fractional digits.` ); value = Math.trunc(value); + // ignore non-finite values. + if (!Number.isInteger(value)) { + return; + } } map.set(attributes, value); } diff --git a/packages/sdk-metrics/test/Instruments.test.ts b/packages/sdk-metrics/test/Instruments.test.ts index 8651643fa3..80f834f30c 100644 --- a/packages/sdk-metrics/test/Instruments.test.ts +++ b/packages/sdk-metrics/test/Instruments.test.ts @@ -74,9 +74,14 @@ describe('Instruments', () => { }); counter.add(1); - // floating-point value should be trunc-ed. - counter.add(1.1); counter.add(1, { foo: 'bar' }); + // floating-point values should be trunc-ed. + counter.add(1.1); + // non-finite/non-number values should be ignored. + counter.add(Infinity); + counter.add(-Infinity); + counter.add(NaN); + counter.add('1' as any); await validateExport(cumulativeReader, { descriptor: { name: 'test', @@ -124,10 +129,13 @@ describe('Instruments', () => { }); counter.add(1); - // add floating-point value. - counter.add(1.1); counter.add(1, { foo: 'bar' }); + // add floating-point values. + counter.add(1.1); counter.add(1.2, { foo: 'bar' }); + // non-number values should be ignored. + counter.add('1' as any); + await validateExport(cumulativeReader, { dataPointType: DataPointType.SUM, isMonotonic: true, @@ -197,6 +205,13 @@ describe('Instruments', () => { upDownCounter.add(-1.1); upDownCounter.add(4, { foo: 'bar' }); upDownCounter.add(1.1, { foo: 'bar' }); + + // non-finite/non-number values should be ignored. + upDownCounter.add(Infinity); + upDownCounter.add(-Infinity); + upDownCounter.add(NaN); + upDownCounter.add('1' as any); + await validateExport(deltaReader, { descriptor: { name: 'test', @@ -230,6 +245,8 @@ describe('Instruments', () => { upDownCounter.add(-1.1); upDownCounter.add(4, { foo: 'bar' }); upDownCounter.add(1.1, { foo: 'bar' }); + // non-number values should be ignored. + upDownCounter.add('1' as any); await validateExport(deltaReader, { dataPointType: DataPointType.SUM, isMonotonic: false, @@ -283,6 +300,12 @@ describe('Instruments', () => { histogram.record(0.1); histogram.record(100, { foo: 'bar' }); histogram.record(0.1, { foo: 'bar' }); + // non-finite/non-number values should be ignored. + histogram.record(Infinity); + histogram.record(-Infinity); + histogram.record(NaN); + histogram.record('1' as any); + await validateExport(deltaReader, { descriptor: { name: 'test', @@ -427,6 +450,9 @@ describe('Instruments', () => { histogram.record(0.1); histogram.record(100, { foo: 'bar' }); histogram.record(0.1, { foo: 'bar' }); + // non-number values should be ignored. + histogram.record('1' as any); + await validateExport(deltaReader, { dataPointType: DataPointType.HISTOGRAM, dataPoints: [ diff --git a/packages/sdk-metrics/test/ObservableResult.test.ts b/packages/sdk-metrics/test/ObservableResult.test.ts index f07b3f9f21..9aacc46066 100644 --- a/packages/sdk-metrics/test/ObservableResult.test.ts +++ b/packages/sdk-metrics/test/ObservableResult.test.ts @@ -63,8 +63,27 @@ describe('ObservableResultImpl', () => { valueType: ValueType.INT, }); observableResult.observe(1.1, {}); + // should ignore non-finite/non-number values. + observableResult.observe(Infinity, {}); + observableResult.observe(-Infinity, {}); + observableResult.observe(NaN, {}); + assert.strictEqual(observableResult._buffer.get({}), 1); }); + + it('should ignore non-number values', () => { + const observableResult = new ObservableResultImpl({ + name: 'test', + description: '', + type: InstrumentType.COUNTER, + unit: '', + valueType: ValueType.INT, + }); + + observableResult.observe('1' as any, {}); + + assert.strictEqual(observableResult._buffer.get({}), undefined); + }); }); }); @@ -126,7 +145,34 @@ describe('BatchObservableResultImpl', () => { ); observableResult.observe(observable, 1.1, {}); + // should ignore non-finite/non-number values. + observableResult.observe(observable, Infinity, {}); + observableResult.observe(observable, -Infinity, {}); + observableResult.observe(observable, NaN, {}); assert.strictEqual(observableResult._buffer.get(observable)?.get({}), 1); }); + + it('should ignore invalid values', () => { + const observableResult = new BatchObservableResultImpl(); + const observable = new ObservableInstrument( + { + name: 'test', + description: '', + type: InstrumentType.COUNTER, + unit: '', + valueType: ValueType.INT, + }, + [], + new ObservableRegistry() + ); + + observableResult.observe(observable, '1' as any, {}); + observableResult.observe(/** invalid observable */ {} as any, 1, {}); + assert.strictEqual( + observableResult._buffer.get(observable)!.get({}), + undefined + ); + assert.strictEqual(observableResult._buffer.size, 1); + }); }); });