Skip to content

Commit

Permalink
fix(sdk-metrics): ignore in accumulation instead
Browse files Browse the repository at this point in the history
  • Loading branch information
pichlermarc committed Feb 1, 2024
1 parent 185515b commit 7898d31
Show file tree
Hide file tree
Showing 6 changed files with 39 additions and 8 deletions.
6 changes: 0 additions & 6 deletions packages/sdk-metrics/src/Instruments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,6 @@ export class SyncInstrument {
);
return;
}
if (Number.isNaN(value)) {
diag.warn(
`NaN provided to metric ${this._descriptor.name}, ignoring to preserve the integrity of the metrics stream`
);
return;
}
if (
this._descriptor.valueType === ValueType.INT &&
!Number.isInteger(value)
Expand Down
6 changes: 6 additions & 0 deletions packages/sdk-metrics/src/aggregator/ExponentialHistogram.ts
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,12 @@ export class ExponentialHistogramAccumulation implements Accumulation {
* @param increment
*/
updateByIncrement(value: number, increment: number) {
// NaN does not fall into any bucket, is not zero and should not be counted,
// NaN is never greater than max nor less than min, therefore return as there's nothing for us to do.
if (Number.isNaN(value)) {
return;
}

if (value > this._max) {
this._max = value;
}
Expand Down
6 changes: 6 additions & 0 deletions packages/sdk-metrics/src/aggregator/Histogram.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,12 @@ export class HistogramAccumulation implements Accumulation {
) {}

record(value: number): void {
// NaN does not fall into any bucket, is not zero and should not be counted,
// NaN is never greater than max nor less than min, therefore return as there's nothing for us to do.
if (Number.isNaN(value)) {
return;
}

this._current.count += 1;
this._current.sum += value;

Expand Down
2 changes: 0 additions & 2 deletions packages/sdk-metrics/test/Instruments.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,6 @@ describe('Instruments', () => {
counter.add(1.2, { foo: 'bar' });
// non-number values should be ignored.
counter.add('1' as any);
counter.add(NaN);

await validateExport(cumulativeReader, {
dataPointType: DataPointType.SUM,
Expand Down Expand Up @@ -248,7 +247,6 @@ describe('Instruments', () => {
upDownCounter.add(1.1, { foo: 'bar' });
// non-number values should be ignored.
upDownCounter.add('1' as any);
upDownCounter.add(NaN);

await validateExport(deltaReader, {
dataPointType: DataPointType.SUM,
Expand Down
14 changes: 14 additions & 0 deletions packages/sdk-metrics/test/aggregator/ExponentialHistogram.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,20 @@ describe('ExponentialHistogramAccumulation', () => {
}
});
});

it('ignores NaN', () => {
const accumulation = new ExponentialHistogramAccumulation([0, 0], 1);

accumulation.record(NaN);

assert.strictEqual(accumulation.scale, 0);
assert.strictEqual(accumulation.max, -Infinity);
assert.strictEqual(accumulation.min, Infinity);
assert.strictEqual(accumulation.sum, 0);
assert.strictEqual(accumulation.count, 0);
assert.deepStrictEqual(getCounts(accumulation.positive), []);
assert.deepStrictEqual(getCounts(accumulation.negative), []);
});
});
describe('merge', () => {
it('handles simple (even) case', () => {
Expand Down
13 changes: 13 additions & 0 deletions packages/sdk-metrics/test/aggregator/Histogram.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,19 @@ describe('HistogramAccumulation', () => {
accumulation.record(value);
}
});

it('ignores NaN', () => {
const accumulation = new HistogramAccumulation([0, 0], [1, 10, 100]);

accumulation.record(NaN);

const pointValue = accumulation.toPointValue();
assert.strictEqual(pointValue.max, -Infinity);
assert.strictEqual(pointValue.min, Infinity);
assert.strictEqual(pointValue.sum, 0);
assert.strictEqual(pointValue.count, 0);
assert.deepStrictEqual(pointValue.buckets.counts, [0, 0, 0, 0]);
});
});

describe('setStartTime', () => {
Expand Down

0 comments on commit 7898d31

Please sign in to comment.