Skip to content

Commit

Permalink
fix(sdk-metrics): ignore NaN value recordings for histograms (open-te…
Browse files Browse the repository at this point in the history
…lemetry#4455)

* fix(sdk-metrics): ignore NaN value recordings

* fix(changelog): add changelog entry

* test(exporter-prometheus): adapt tests

* fix(sdk-metrics): ignore in accumulation instead

* fix(changelog): update changelog
  • Loading branch information
pichlermarc authored and Zirak committed Sep 14, 2024
1 parent 171620f commit 9bf303b
Show file tree
Hide file tree
Showing 8 changed files with 143 additions and 3 deletions.
5 changes: 3 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,9 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/

### :bug: (Bug Fix)

* fix(sdk-metrics): handle zero bucket counts in exponential histogram merge
[#4459](https://github.com/open-telemetry/opentelemetry-js/pull/4459) @mwear
* fix(sdk-metrics): handle zero bucket counts in exponential histogram merge [#4459](https://github.com/open-telemetry/opentelemetry-js/pull/4459) @mwear
* fix(sdk-metrics): ignore `NaN` value recordings in Histograms [#4455](https://github.com/open-telemetry/opentelemetry-js/pull/4455) @pichlermarc
* fixes a bug where recording `NaN` on a histogram would result in the sum of bucket count values not matching the overall count

### :books: (Refine Doc)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -622,7 +622,6 @@ describe('PrometheusSerializer', () => {
it('should serialize non-finite values', async () => {
const serializer = new PrometheusSerializer();
const cases = [
[NaN, 'NaN'],
[-Infinity, '-Inf'],
[+Infinity, '+Inf'],
] as [number, string][];
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: 2 additions & 0 deletions packages/sdk-metrics/test/Instruments.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,7 @@ describe('Instruments', () => {
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 @@ -506,6 +507,7 @@ describe('Instruments', () => {
histogram.record(0.1, { foo: 'bar' });
// non-number values should be ignored.
histogram.record('1' as any);
histogram.record(NaN);

await validateExport(deltaReader, {
dataPointType: DataPointType.HISTOGRAM,
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/*
* Copyright The OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import * as assert from 'assert';
import {
Aggregation,
AggregationTemporality,
MeterProvider,
MetricReader,
DataPoint,
ExponentialHistogram,
Histogram,
} from '../../src';
import { TestMetricReader } from '../export/TestMetricReader';

describe('histogram-recording-nan', () => {
it('exponential histogram should not count NaN', async () => {
const reader = new TestMetricReader({
aggregationTemporalitySelector() {
return AggregationTemporality.CUMULATIVE;
},
aggregationSelector(type) {
return Aggregation.ExponentialHistogram();
},
});
const meterProvider = new MeterProvider({
readers: [reader],
});

const meter = meterProvider.getMeter('my-meter');
const hist = meter.createHistogram('testhist');

hist.record(1);
hist.record(2);
hist.record(4);
hist.record(NaN);

const resourceMetrics1 = await collectNoErrors(reader);
const dataPoint1 = resourceMetrics1.scopeMetrics[0].metrics[0]
.dataPoints[0] as DataPoint<ExponentialHistogram>;

assert.deepStrictEqual(
dataPoint1.value.count,
3,
'Sum of bucket count values should match overall count'
);
});

it('explicit bucket histogram should not count NaN', async () => {
const reader = new TestMetricReader({
aggregationTemporalitySelector() {
return AggregationTemporality.CUMULATIVE;
},
aggregationSelector(type) {
return Aggregation.Histogram();
},
});
const meterProvider = new MeterProvider({
readers: [reader],
});

const meter = meterProvider.getMeter('my-meter');
const hist = meter.createHistogram('testhist');

hist.record(1);
hist.record(2);
hist.record(4);
hist.record(NaN);

const resourceMetrics1 = await collectNoErrors(reader);
const dataPoint1 = resourceMetrics1.scopeMetrics[0].metrics[0]
.dataPoints[0] as DataPoint<Histogram>;

assert.deepStrictEqual(
dataPoint1.value.count,
3,
'Sum of bucket count values should match overall count'
);
});

const collectNoErrors = async (reader: MetricReader) => {
const { resourceMetrics, errors } = await reader.collect();
assert.strictEqual(errors.length, 0);
return resourceMetrics;
};
});

0 comments on commit 9bf303b

Please sign in to comment.