Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ignore non-number value on BaseBoundInstrument.update #1366

Merged
merged 12 commits into from
Aug 20, 2020
Merged
8 changes: 8 additions & 0 deletions packages/opentelemetry-metrics/src/BoundInstrument.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,14 @@ export class BaseBoundInstrument {

update(value: number): void {
if (this._disabled) return;
if (typeof value !== 'number') {
this._logger.error(
`Metric cannot accept a non-number value for ${Object.values(
this._labels
)}.`
);
return;
}

if (this._valueType === api.ValueType.INT && !Number.isInteger(value)) {
this._logger.warn(
Expand Down
106 changes: 106 additions & 0 deletions packages/opentelemetry-metrics/test/Meter.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,35 @@ import { Resource } from '@opentelemetry/resources';
import { UpDownSumObserverMetric } from '../src/UpDownSumObserverMetric';
import { hashLabels } from '../src/Utils';
import { Batcher } from '../src/export/Batcher';
import { ValueType } from '@opentelemetry/api';

const nonNumberValues = [
// type undefined
undefined,
// type null
null,
// type function
function () {},
// type boolean
true,
false,
// type string
'1',
// type object
{},
// type symbol
// symbols cannot be cast to number, early errors will be thrown.
];

if (Number(process.versions.node.match(/^\d+/)) >= 10) {
nonNumberValues.push(
// type bigint
// Preferring BigInt builtin object instead of bigint literal to keep Node.js v8.x working.
// TODO: should metric instruments support bigint?
// @ts-ignore
BigInt(1) // eslint-disable-line node/no-unsupported-features/es-builtins
);
}

describe('Meter', () => {
let meter: Meter;
Expand Down Expand Up @@ -374,6 +403,56 @@ describe('Meter', () => {
assert.strictEqual(record1.aggregator.toPoint().value, 20);
assert.strictEqual(boundCounter, boundCounter1);
});

it('should truncate non-integer values for INT valueType', async () => {
const upDownCounter = meter.createUpDownCounter('name', {
valueType: ValueType.INT,
});
const boundCounter = upDownCounter.bind(labels);

[-1.1, 2.2].forEach(val => {
boundCounter.add(val);
});
await meter.collect();
const [record1] = meter.getBatcher().checkPointSet();
assert.strictEqual(record1.aggregator.toPoint().value, 1);
});

it('should ignore non-number values for INT valueType', async () => {
const upDownCounter = meter.createUpDownCounter('name', {
valueType: ValueType.DOUBLE,
});
const boundCounter = upDownCounter.bind(labels);

await Promise.all(
nonNumberValues.map(async val => {
// @ts-expect-error
boundCounter.add(val);
await meter.collect();
const [record1] = meter.getBatcher().checkPointSet();

assert.strictEqual(record1.aggregator.toPoint().value, 0);
})
);
});

it('should ignore non-number values for DOUBLE valueType', async () => {
const upDownCounter = meter.createUpDownCounter('name', {
valueType: ValueType.DOUBLE,
});
const boundCounter = upDownCounter.bind(labels);

await Promise.all(
nonNumberValues.map(async val => {
// @ts-expect-error
boundCounter.add(val);
await meter.collect();
const [record1] = meter.getBatcher().checkPointSet();

assert.strictEqual(record1.aggregator.toPoint().value, 0);
})
);
});
});

describe('.unbind()', () => {
Expand Down Expand Up @@ -651,6 +730,33 @@ describe('Meter', () => {
);
assert.strictEqual(boundValueRecorder1, boundValueRecorder2);
});

it('should ignore non-number values', async () => {
const valueRecorder = meter.createValueRecorder(
'name'
) as ValueRecorderMetric;
const boundValueRecorder = valueRecorder.bind(labels);

await Promise.all(
nonNumberValues.map(async val => {
// @ts-expect-error
boundValueRecorder.record(val);
await meter.collect();
const [record1] = meter.getBatcher().checkPointSet();

assert.deepStrictEqual(
record1.aggregator.toPoint().value as Distribution,
{
count: 0,
last: 0,
max: -Infinity,
min: Infinity,
sum: 0,
}
);
})
);
});
});

describe('.unbind()', () => {
Expand Down