Skip to content

Commit

Permalink
fix(sdk-metrics): ignore invalid metric values (open-telemetry#3988)
Browse files Browse the repository at this point in the history 
Co-authored-by: Haddas Bronfman <85441461+haddasbronfman@users.noreply.github.com>
  • Loading branch information
@legendecas @haddasbronfman
legendecas and haddasbronfman authored Aug 1, 2023
1 parent 87fff2e commit 4cffe5d
Show file tree
Hide file tree
Showing 5 changed files with 107 additions and 4 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
10 changes: 10 additions & 0 deletions packages/sdk-metrics/src/Instruments.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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,
Expand Down
20 changes: 20 additions & 0 deletions packages/sdk-metrics/src/ObservableResult.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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);
}
Expand Down Expand Up @@ -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)
Expand All @@ -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);
}
Expand Down
34 changes: 30 additions & 4 deletions packages/sdk-metrics/test/Instruments.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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: [
Expand Down
46 changes: 46 additions & 0 deletions packages/sdk-metrics/test/ObservableResult.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
});

Expand Down Expand Up @@ -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);
});
});
});

0 comments on commit 4cffe5d

Please sign in to comment.