From 93d923bdaeda73a0e96bb0273d41511d90fcd0fe Mon Sep 17 00:00:00 2001 From: legendecas Date: Wed, 29 Jul 2020 19:22:51 +0800 Subject: [PATCH 1/7] fix: skip non-number value on BaseBoundInstrument.update --- .../src/BoundInstrument.ts | 8 ++ .../opentelemetry-metrics/test/Meter.test.ts | 97 +++++++++++++++++++ 2 files changed, 105 insertions(+) diff --git a/packages/opentelemetry-metrics/src/BoundInstrument.ts b/packages/opentelemetry-metrics/src/BoundInstrument.ts index dad6ce01eca..b6f86e0703d 100644 --- a/packages/opentelemetry-metrics/src/BoundInstrument.ts +++ b/packages/opentelemetry-metrics/src/BoundInstrument.ts @@ -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( diff --git a/packages/opentelemetry-metrics/test/Meter.test.ts b/packages/opentelemetry-metrics/test/Meter.test.ts index eb6f3d3d0a2..17ba4717931 100644 --- a/packages/opentelemetry-metrics/test/Meter.test.ts +++ b/packages/opentelemetry-metrics/test/Meter.test.ts @@ -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; @@ -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 () => { + const upDownCounter = meter.createUpDownCounter('name', { + valueType: ValueType.INT, + }); + const boundCounter = upDownCounter.bind(labels); + { + 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()', () => { @@ -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()', () => { From 660538e6720c333dcbb2e0e3ede4e2797b4cc5a3 Mon Sep 17 00:00:00 2001 From: legendecas Date: Mon, 3 Aug 2020 10:42:51 +0800 Subject: [PATCH 2/7] test: iterates a set of non-number values for test --- .../opentelemetry-metrics/test/Meter.test.ts | 111 ++++++++---------- 1 file changed, 47 insertions(+), 64 deletions(-) diff --git a/packages/opentelemetry-metrics/test/Meter.test.ts b/packages/opentelemetry-metrics/test/Meter.test.ts index 17ba4717931..9b32d10e26b 100644 --- a/packages/opentelemetry-metrics/test/Meter.test.ts +++ b/packages/opentelemetry-metrics/test/Meter.test.ts @@ -43,6 +43,8 @@ import { hashLabels } from '../src/Utils'; import { Batcher } from '../src/export/Batcher'; import { ValueType } from '@opentelemetry/api'; +const nonNumberValues = [undefined, Symbol('123'), {}]; + describe('Meter', () => { let meter: Meter; const keya = 'keya'; @@ -376,7 +378,7 @@ describe('Meter', () => { assert.strictEqual(boundCounter, boundCounter1); }); - it('should trunk non-integer values for INT valueType', async () => { + it('should truncate non-integer values for INT valueType', async () => { const upDownCounter = meter.createUpDownCounter('name', { valueType: ValueType.INT, }); @@ -388,24 +390,24 @@ describe('Meter', () => { 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); - } + it('should ignore non-number values for INT valueType', async () => { + const upDownCounter = meter.createUpDownCounter('name', { + valueType: ValueType.DOUBLE, + }); + const boundCounter = upDownCounter.bind(labels); - { - // disable type checking... - (boundCounter.add as any)({}); - await meter.collect(); - const [record1] = meter.getBatcher().checkPointSet(); + await Promise.all( + nonNumberValues.map(async val => { + // disable type checking... + (boundCounter.add as any)(val); + await meter.collect(); + const [record1] = meter.getBatcher().checkPointSet(); - assert.strictEqual(record1.aggregator.toPoint().value, -1); - } + assert.strictEqual(record1.aggregator.toPoint().value, 0); + }) + ); }); it('should ignore non-number values for DOUBLE valueType', async () => { @@ -414,23 +416,16 @@ describe('Meter', () => { }); 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); - } + await Promise.all( + nonNumberValues.map(async val => { + // disable type checking... + (boundCounter.add as any)(val); + await meter.collect(); + const [record1] = meter.getBatcher().checkPointSet(); - { - // disable type checking... - (boundCounter.add as any)({}); - await meter.collect(); - const [record1] = meter.getBatcher().checkPointSet(); - - assert.strictEqual(record1.aggregator.toPoint().value, 0); - } + assert.strictEqual(record1.aggregator.toPoint().value, 0); + }) + ); }); }); @@ -715,38 +710,26 @@ describe('Meter', () => { '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, - } - ); - } + + await Promise.all( + nonNumberValues.map(async val => { + // 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, + } + ); + }) + ); }); }); From 5a67737a711695b4fb57f4c58f66e1f12dfcaaf2 Mon Sep 17 00:00:00 2001 From: legendecas Date: Mon, 3 Aug 2020 10:50:58 +0800 Subject: [PATCH 3/7] test: iterates float point values on truncating of non-integer values --- packages/opentelemetry-metrics/test/Meter.test.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/opentelemetry-metrics/test/Meter.test.ts b/packages/opentelemetry-metrics/test/Meter.test.ts index 9b32d10e26b..c6498fe1cca 100644 --- a/packages/opentelemetry-metrics/test/Meter.test.ts +++ b/packages/opentelemetry-metrics/test/Meter.test.ts @@ -383,13 +383,13 @@ describe('Meter', () => { valueType: ValueType.INT, }); const boundCounter = upDownCounter.bind(labels); - { - boundCounter.add(-1.1); - await meter.collect(); - const [record1] = meter.getBatcher().checkPointSet(); - assert.strictEqual(record1.aggregator.toPoint().value, -1); - } + [-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 () => { From 56521b717b589e4f71f8ecfbcb710fbd53b3327d Mon Sep 17 00:00:00 2001 From: legendecas Date: Mon, 3 Aug 2020 16:33:12 +0800 Subject: [PATCH 4/7] feat: use @ts-expect-error directive on testing arbitrary types --- packages/opentelemetry-metrics/test/Meter.test.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/opentelemetry-metrics/test/Meter.test.ts b/packages/opentelemetry-metrics/test/Meter.test.ts index c6498fe1cca..ce69c3b83a6 100644 --- a/packages/opentelemetry-metrics/test/Meter.test.ts +++ b/packages/opentelemetry-metrics/test/Meter.test.ts @@ -400,8 +400,8 @@ describe('Meter', () => { await Promise.all( nonNumberValues.map(async val => { - // disable type checking... - (boundCounter.add as any)(val); + // @ts-expect-error + boundCounter.add(val); await meter.collect(); const [record1] = meter.getBatcher().checkPointSet(); @@ -418,8 +418,8 @@ describe('Meter', () => { await Promise.all( nonNumberValues.map(async val => { - // disable type checking... - (boundCounter.add as any)(val); + // @ts-expect-error + boundCounter.add(val); await meter.collect(); const [record1] = meter.getBatcher().checkPointSet(); @@ -713,8 +713,8 @@ describe('Meter', () => { await Promise.all( nonNumberValues.map(async val => { - // disable type checking... - (boundValueRecorder.record as any)(undefined); + // @ts-expect-error + boundValueRecorder.record(undefined); await meter.collect(); const [record1] = meter.getBatcher().checkPointSet(); From ae07012bccf947c9e0ee34998a8805aa3d5891d9 Mon Sep 17 00:00:00 2001 From: legendecas Date: Mon, 3 Aug 2020 16:38:48 +0800 Subject: [PATCH 5/7] test: unused parameter val in iteration --- packages/opentelemetry-metrics/test/Meter.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opentelemetry-metrics/test/Meter.test.ts b/packages/opentelemetry-metrics/test/Meter.test.ts index ce69c3b83a6..79a606fd89e 100644 --- a/packages/opentelemetry-metrics/test/Meter.test.ts +++ b/packages/opentelemetry-metrics/test/Meter.test.ts @@ -714,7 +714,7 @@ describe('Meter', () => { await Promise.all( nonNumberValues.map(async val => { // @ts-expect-error - boundValueRecorder.record(undefined); + boundValueRecorder.record(val); await meter.collect(); const [record1] = meter.getBatcher().checkPointSet(); From 9ff4219c78c2d89c2ccfffb36d6aaf418c736051 Mon Sep 17 00:00:00 2001 From: legendecas Date: Mon, 3 Aug 2020 20:30:11 +0800 Subject: [PATCH 6/7] test: add more type tests --- .../opentelemetry-metrics/test/Meter.test.ts | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/packages/opentelemetry-metrics/test/Meter.test.ts b/packages/opentelemetry-metrics/test/Meter.test.ts index 79a606fd89e..60ba14c6b25 100644 --- a/packages/opentelemetry-metrics/test/Meter.test.ts +++ b/packages/opentelemetry-metrics/test/Meter.test.ts @@ -43,7 +43,27 @@ import { hashLabels } from '../src/Utils'; import { Batcher } from '../src/export/Batcher'; import { ValueType } from '@opentelemetry/api'; -const nonNumberValues = [undefined, Symbol('123'), {}]; +const nonNumberValues = [ + // type undefined + undefined, + // type null + null, + // type function + function () {}, + // type boolean + true, + false, + // type string + '1', + // type bigint + // TODO: should metric instruments support bigint? + // @ts-ignore + 1n, + // type object + {}, + // type symbol + // symbols cannot be cast to number, early errors will be thrown. +]; describe('Meter', () => { let meter: Meter; From e793a68ebad85471240896379ebbde03ebb913d7 Mon Sep 17 00:00:00 2001 From: legendecas Date: Tue, 4 Aug 2020 12:04:22 +0800 Subject: [PATCH 7/7] fix: bigint literal is not supported with Node.js v8.x --- packages/opentelemetry-metrics/test/Meter.test.ts | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/packages/opentelemetry-metrics/test/Meter.test.ts b/packages/opentelemetry-metrics/test/Meter.test.ts index 60ba14c6b25..19960fcb9f4 100644 --- a/packages/opentelemetry-metrics/test/Meter.test.ts +++ b/packages/opentelemetry-metrics/test/Meter.test.ts @@ -55,16 +55,22 @@ const nonNumberValues = [ false, // type string '1', - // type bigint - // TODO: should metric instruments support bigint? - // @ts-ignore - 1n, // 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; const keya = 'keya';