From b4b2b0c22cb1267706401c130ee22c9fb56c464d Mon Sep 17 00:00:00 2001 From: legendecas Date: Thu, 10 Feb 2022 11:42:20 +0800 Subject: [PATCH 1/2] feat(sdk-metrics-base): add ValueType support in sync instruments --- .../src/Instruments.ts | 20 +- .../test/Instruments.test.ts | 241 +++++++++++++++--- .../test/util.ts | 2 +- 3 files changed, 227 insertions(+), 36 deletions(-) diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/Instruments.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/Instruments.ts index 60dbc9e364..8e9560810a 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/src/Instruments.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/Instruments.ts @@ -16,17 +16,27 @@ import * as api from '@opentelemetry/api'; import * as metrics from '@opentelemetry/api-metrics-wip'; +import { ValueType } from '@opentelemetry/api-metrics-wip'; import { InstrumentDescriptor } from './InstrumentDescriptor'; import { WritableMetricStorage } from './state/WritableMetricStorage'; export class SyncInstrument { - constructor(private _writableMetricStorage: WritableMetricStorage, private _descriptor: InstrumentDescriptor) { } + private _valueType: ValueType; + constructor(private _writableMetricStorage: WritableMetricStorage, private _descriptor: InstrumentDescriptor) { + this._valueType = _descriptor.valueType; + } getName(): string { return this._descriptor.name; } - aggregate(value: number, attributes: metrics.Attributes = {}, context: api.Context = api.context.active()) { + protected _record(value: number, attributes: metrics.Attributes = {}, context: api.Context = api.context.active()) { + if (this._valueType === ValueType.INT && !Number.isInteger(value)) { + api.diag.warn( + `INT value type cannot accept a floating-point value for ${this._descriptor.name}, ignoring the fractional digits.` + ); + value = Math.trunc(value); + } this._writableMetricStorage.record(value, attributes, context); } } @@ -39,7 +49,7 @@ export class UpDownCounter extends SyncInstrument implements metrics.UpDownCount * Increment value of counter by the input. Inputs may be negative. */ add(value: number, attributes?: metrics.Attributes, ctx?: api.Context): void { - this.aggregate(value, attributes, ctx); + this._record(value, attributes, ctx); } } @@ -56,7 +66,7 @@ export class Counter extends SyncInstrument implements metrics.Counter { return; } - this.aggregate(value, attributes, ctx); + this._record(value, attributes, ctx); } } @@ -68,6 +78,6 @@ export class Histogram extends SyncInstrument implements metrics.Histogram { * Records a measurement. Value of the measurement must not be negative. */ record(value: number, attributes?: metrics.Attributes, ctx?: api.Context): void { - this.aggregate(value, attributes, ctx); + this._record(value, attributes, ctx); } } diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/test/Instruments.test.ts b/experimental/packages/opentelemetry-sdk-metrics-base/test/Instruments.test.ts index 67c89891f3..97151c5e42 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/test/Instruments.test.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/test/Instruments.test.ts @@ -18,33 +18,56 @@ import * as assert from 'assert'; import * as sinon from 'sinon'; import { InstrumentationLibrary } from '@opentelemetry/core'; import { Resource } from '@opentelemetry/resources'; -import { AggregationTemporality, InstrumentDescriptor, MeterProvider, MetricReader, PointData, PointDataType } from '../src'; +import { AggregationTemporality, InstrumentDescriptor, InstrumentType, MeterProvider, MetricReader, PointData, PointDataType } from '../src'; import { TestMetricReader } from './export/TestMetricReader'; import { assertMetricData, assertPointData, commonValues, commonAttributes, defaultResource, defaultInstrumentationLibrary } from './util'; import { Histogram } from '../src/aggregator/types'; -import { ObservableResult } from '@opentelemetry/api-metrics-wip'; +import { ObservableResult, ValueType } from '@opentelemetry/api-metrics-wip'; describe('Instruments', () => { describe('Counter', () => { - it('should add common values and attributes without exceptions', () => { - const { meter } = setup(); - const counter = meter.createCounter('test'); + it('should add common values and attributes without exceptions', async () => { + const { meter, cumulativeReader } = setup(); + const counter = meter.createCounter('test', { + description: 'foobar', + unit: 'kB', + }); for (const values of commonValues) { for (const attributes of commonAttributes) { counter.add(values, attributes); } } + + await validateExport(cumulativeReader, { + instrumentDescriptor: { + name: 'test', + description: 'foobar', + unit: 'kB', + type: InstrumentType.COUNTER, + valueType: ValueType.DOUBLE, + }, + }); }); - it('should add valid values', async () => { + it('should add valid INT values', async () => { const { meter, cumulativeReader } = setup(); - const counter = meter.createCounter('test'); + const counter = meter.createCounter('test', { + valueType: ValueType.INT, + }); counter.add(1); - counter.add(1); + // floating-point value should be trunc-ed. + counter.add(1.1); counter.add(1, { foo: 'bar' }); await validateExport(cumulativeReader, { + instrumentDescriptor: { + name: 'test', + description: '', + unit: '1', + type: InstrumentType.COUNTER, + valueType: ValueType.INT, + }, pointDataType: PointDataType.SINGULAR, pointData: [ { @@ -59,7 +82,7 @@ describe('Instruments', () => { }); // add negative values should not be observable. - counter.add(-1); + counter.add(-1.1); await validateExport(cumulativeReader, { pointDataType: PointDataType.SINGULAR, pointData: [ @@ -74,28 +97,93 @@ describe('Instruments', () => { ], }); }); + + it('should add valid DOUBLE values', async () => { + const { meter, cumulativeReader } = setup(); + const counter = meter.createCounter('test', { + valueType: ValueType.DOUBLE, + }); + + counter.add(1); + // add floating-point value. + counter.add(1.1); + counter.add(1, { foo: 'bar' }); + counter.add(1.2, { foo: 'bar' }); + await validateExport(cumulativeReader, { + pointDataType: PointDataType.SINGULAR, + pointData: [ + { + attributes: {}, + point: 2.1, + }, + { + attributes: { foo: 'bar' }, + point: 2.2, + }, + ], + }); + + // add negative values should not be observable. + counter.add(-1.1); + await validateExport(cumulativeReader, { + pointDataType: PointDataType.SINGULAR, + pointData: [ + { + attributes: {}, + point: 2.1, + }, + { + attributes: { foo: 'bar' }, + point: 2.2, + }, + ], + }); + }); }); describe('UpDownCounter', () => { - it('should add common values and attributes without exceptions', () => { - const { meter } = setup(); - const upDownCounter = meter.createUpDownCounter('test'); + it('should add common values and attributes without exceptions', async () => { + const { meter, cumulativeReader } = setup(); + const upDownCounter = meter.createUpDownCounter('test', { + description: 'foobar', + unit: 'kB', + }); for (const values of commonValues) { for (const attributes of commonAttributes) { upDownCounter.add(values, attributes); } } + + await validateExport(cumulativeReader, { + instrumentDescriptor: { + name: 'test', + description: 'foobar', + unit: 'kB', + type: InstrumentType.UP_DOWN_COUNTER, + valueType: ValueType.DOUBLE, + }, + }); }); - it('should add values', async () => { + it('should add INT values', async () => { const { meter, deltaReader } = setup(); - const upDownCounter = meter.createUpDownCounter('test'); + const upDownCounter = meter.createUpDownCounter('test', { + valueType: ValueType.INT, + }); upDownCounter.add(3); - upDownCounter.add(-1); + upDownCounter.add(-1.1); upDownCounter.add(4, { foo: 'bar' }); + upDownCounter.add(1.1, { foo: 'bar' }); await validateExport(deltaReader, { + instrumentDescriptor: { + name: 'test', + description: '', + unit: '1', + type: InstrumentType.UP_DOWN_COUNTER, + valueType: ValueType.INT, + }, pointDataType: PointDataType.SINGULAR, pointData: [ { @@ -104,7 +192,32 @@ describe('Instruments', () => { }, { attributes: { foo: 'bar' }, - point: 4, + point: 5, + } + ], + }); + }); + + it('should add DOUBLE values', async () => { + const { meter, deltaReader } = setup(); + const upDownCounter = meter.createUpDownCounter('test', { + valueType: ValueType.DOUBLE, + }); + + upDownCounter.add(3); + upDownCounter.add(-1.1); + upDownCounter.add(4, { foo: 'bar' }); + upDownCounter.add(1.1, { foo: 'bar' }); + await validateExport(deltaReader, { + pointDataType: PointDataType.SINGULAR, + pointData: [ + { + attributes: {}, + point: 1.9, + }, + { + attributes: { foo: 'bar' }, + point: 5.1, } ], }); @@ -112,9 +225,12 @@ describe('Instruments', () => { }); describe('Histogram', () => { - it('should record common values and attributes without exceptions', () => { - const { meter } = setup(); - const histogram = meter.createHistogram('test'); + it('should record common values and attributes without exceptions', async () => { + const { meter, cumulativeReader } = setup(); + const histogram = meter.createHistogram('test', { + description: 'foobar', + unit: 'kB', + }); for (const values of commonValues) { for (const attributes of commonAttributes) { @@ -122,16 +238,37 @@ describe('Instruments', () => { histogram.record(values, attributes); } } + + await validateExport(cumulativeReader, { + instrumentDescriptor: { + name: 'test', + description: 'foobar', + unit: 'kB', + type: InstrumentType.HISTOGRAM, + valueType: ValueType.DOUBLE, + }, + }); }); - it('should record values', async () => { + it('should record INT values', async () => { const { meter, deltaReader } = setup(); - const histogram = meter.createHistogram('test'); + const histogram = meter.createHistogram('test', { + valueType: ValueType.INT, + }); histogram.record(10); - histogram.record(-1); + // -0.1 should be trunc-ed to -0 + histogram.record(-0.1); histogram.record(100, { foo: 'bar' }); + histogram.record(-0.1, { foo: 'bar' }); await validateExport(deltaReader, { + instrumentDescriptor: { + name: 'test', + description: '', + unit: '1', + type: InstrumentType.HISTOGRAM, + valueType: ValueType.INT, + }, pointDataType: PointDataType.HISTOGRAM, pointData: [ { @@ -139,10 +276,10 @@ describe('Instruments', () => { point: { buckets: { boundaries: [0, 5, 10, 25, 50, 75, 100, 250, 500, 1000], - counts: [1, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0], + counts: [0, 1, 0, 1, 0, 0, 0, 0, 0, 0, 0], }, count: 2, - sum: 9, + sum: 10, }, }, { @@ -150,15 +287,55 @@ describe('Instruments', () => { point: { buckets: { boundaries: [0, 5, 10, 25, 50, 75, 100, 250, 500, 1000], - counts: [0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0], + counts: [0, 1, 0, 0, 0, 0, 0, 1, 0, 0, 0], }, - count: 1, + count: 2, sum: 100, }, }, ], }); }); + + + it('should record DOUBLE values', async () => { + const { meter, deltaReader } = setup(); + const histogram = meter.createHistogram('test', { + valueType: ValueType.DOUBLE, + }); + + histogram.record(10); + histogram.record(-0.1); + histogram.record(100, { foo: 'bar' }); + histogram.record(-0.1, { foo: 'bar' }); + await validateExport(deltaReader, { + pointDataType: PointDataType.HISTOGRAM, + pointData: [ + { + attributes: {}, + point: { + buckets: { + boundaries: [0, 5, 10, 25, 50, 75, 100, 250, 500, 1000], + counts: [1, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0], + }, + count: 2, + sum: 9.9, + }, + }, + { + attributes: { foo: 'bar' }, + point: { + buckets: { + boundaries: [0, 5, 10, 25, 50, 75, 100, 250, 500, 1000], + counts: [1, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0], + }, + count: 2, + sum: 99.9, + }, + }, + ], + }); + }); }); describe('ObservableCounter', () => { @@ -361,12 +538,12 @@ interface ValidateMetricData { resource?: Resource; instrumentationLibrary?: InstrumentationLibrary; instrumentDescriptor?: InstrumentDescriptor; - pointDataType: PointDataType, - pointData: Partial>>[]; + pointDataType?: PointDataType, + pointData?: Partial>>[]; } -async function validateExport(deltaReader: MetricReader, expected: ValidateMetricData) { - const metricData = await deltaReader.collect(); +async function validateExport(reader: MetricReader, expected: ValidateMetricData) { + const metricData = await reader.collect(); const metric = metricData[0]; assertMetricData( @@ -376,6 +553,10 @@ async function validateExport(deltaReader: MetricReader, expected: ValidateMetri expected.instrumentationLibrary, expected.resource ); + + if (expected.pointData == null) { + return; + } assert.strictEqual(metric.pointData.length, expected.pointData.length); for (let idx = 0; idx < metric.pointData.length; idx++) { diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/test/util.ts b/experimental/packages/opentelemetry-sdk-metrics-base/test/util.ts index d3d9e9e9b6..b50f6ac928 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/test/util.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/test/util.ts @@ -120,6 +120,6 @@ export function assertPartialDeepStrictEqual(actual: unknown, expected: T, me } const ownNames = Object.getOwnPropertyNames(expected); for (const ownName of ownNames) { - assert.deepStrictEqual((actual as any)[ownName], (expected as any)[ownName], message); + assert.deepStrictEqual((actual as any)[ownName], (expected as any)[ownName], `${ownName} not equals: ${message ?? ''}`); } } From 1722c38c9e9c04a546e348ececfdb3ef54a04532 Mon Sep 17 00:00:00 2001 From: legendecas Date: Fri, 11 Feb 2022 11:51:04 +0800 Subject: [PATCH 2/2] fixup! --- .../opentelemetry-sdk-metrics-base/src/Instruments.ts | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/Instruments.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/Instruments.ts index 8e9560810a..0d899b29b1 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/src/Instruments.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/Instruments.ts @@ -16,22 +16,18 @@ import * as api from '@opentelemetry/api'; import * as metrics from '@opentelemetry/api-metrics-wip'; -import { ValueType } from '@opentelemetry/api-metrics-wip'; import { InstrumentDescriptor } from './InstrumentDescriptor'; import { WritableMetricStorage } from './state/WritableMetricStorage'; export class SyncInstrument { - private _valueType: ValueType; - constructor(private _writableMetricStorage: WritableMetricStorage, private _descriptor: InstrumentDescriptor) { - this._valueType = _descriptor.valueType; - } + constructor(private _writableMetricStorage: WritableMetricStorage, private _descriptor: InstrumentDescriptor) {} getName(): string { return this._descriptor.name; } protected _record(value: number, attributes: metrics.Attributes = {}, context: api.Context = api.context.active()) { - if (this._valueType === ValueType.INT && !Number.isInteger(value)) { + if (this._descriptor.valueType === metrics.ValueType.INT && !Number.isInteger(value)) { api.diag.warn( `INT value type cannot accept a floating-point value for ${this._descriptor.name}, ignoring the fractional digits.` );