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
97 changes: 97 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,7 @@ 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';

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

it('should trunk non-integer values for INT valueType', async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you mean by "trunk" here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most likely trunc or truncate. @legendecas Please avoid abbreviations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ignore then ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's a typo and it should be fixed, and the other cases should be made into their own tests.

Suggested change
it('should trunk non-integer values for INT valueType', async () => {
it('should truncate floating number values for INT valueType', async () => {

const upDownCounter = meter.createUpDownCounter('name', {
valueType: ValueType.INT,
});
const boundCounter = upDownCounter.bind(labels);
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why extra brackets are needed here and in few other places ? If you want to separate cases perhaps it's a moment where it should go into separate unit test or simply make an array of values to test and then use a loop to test each value

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The brackets give lexical scope to the const [record1] = meter.getBatcher().checkPointSet(); line. It is repeated several times in the test so that different values may be tested.

These should be separate tests as they are not all testing truncation

boundCounter.add(-1.1);
await meter.collect();
const [record1] = meter.getBatcher().checkPointSet();

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

{
// disable type checking...
(boundCounter.add as any)(undefined);
await meter.collect();
const [record1] = meter.getBatcher().checkPointSet();

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

{
// disable type checking...
(boundCounter.add as any)({});
await meter.collect();
const [record1] = meter.getBatcher().checkPointSet();

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

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

{
// disable type checking...
(boundCounter.add as any)(undefined);
await meter.collect();
const [record1] = meter.getBatcher().checkPointSet();

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

{
// disable type checking...
(boundCounter.add as any)({});
await meter.collect();
const [record1] = meter.getBatcher().checkPointSet();

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

describe('.unbind()', () => {
Expand Down Expand Up @@ -651,6 +709,45 @@ 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);
{
// disable type checking...
(boundValueRecorder.record as any)(undefined);
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,
}
);
}
{
// disable type checking...
(boundValueRecorder.record as any)({});
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