From 245cab211de7682f549447a78968905afa25ab69 Mon Sep 17 00:00:00 2001 From: Julian Labatut Date: Wed, 1 Mar 2023 18:17:57 +0100 Subject: [PATCH 1/4] feat: add unit to view instrument selection criteria (#3647) Signed-off-by: Julian Labatut --- packages/sdk-metrics/src/view/InstrumentSelector.ts | 9 ++++++++- .../sdk-metrics/src/view/RegistrationConflicts.ts | 2 ++ packages/sdk-metrics/src/view/View.ts | 13 +++++++++++++ packages/sdk-metrics/src/view/ViewRegistry.ts | 3 ++- 4 files changed, 25 insertions(+), 2 deletions(-) diff --git a/packages/sdk-metrics/src/view/InstrumentSelector.ts b/packages/sdk-metrics/src/view/InstrumentSelector.ts index e7bec3a44a..9b8006267f 100644 --- a/packages/sdk-metrics/src/view/InstrumentSelector.ts +++ b/packages/sdk-metrics/src/view/InstrumentSelector.ts @@ -15,20 +15,23 @@ */ import { InstrumentType } from '../InstrumentDescriptor'; -import { PatternPredicate, Predicate } from './Predicate'; +import { ExactPredicate, PatternPredicate, Predicate } from './Predicate'; export interface InstrumentSelectorCriteria { name?: string; type?: InstrumentType; + unit?: string; } export class InstrumentSelector { private _nameFilter: Predicate; private _type?: InstrumentType; + private _unitFilter: Predicate; constructor(criteria?: InstrumentSelectorCriteria) { this._nameFilter = new PatternPredicate(criteria?.name ?? '*'); this._type = criteria?.type; + this._unitFilter = new ExactPredicate(criteria?.unit); } getType() { @@ -38,4 +41,8 @@ export class InstrumentSelector { getNameFilter() { return this._nameFilter; } + + getUnitFilter() { + return this._unitFilter; + } } diff --git a/packages/sdk-metrics/src/view/RegistrationConflicts.ts b/packages/sdk-metrics/src/view/RegistrationConflicts.ts index e74add7334..dbb8073c8b 100644 --- a/packages/sdk-metrics/src/view/RegistrationConflicts.ts +++ b/packages/sdk-metrics/src/view/RegistrationConflicts.ts @@ -59,6 +59,7 @@ export function getTypeConflictResolutionRecipe( const selector: InstrumentSelectorCriteria = { name: otherDescriptor.name, type: otherDescriptor.type, + unit: otherDescriptor.unit, }; const selectorString = JSON.stringify(selector); @@ -73,6 +74,7 @@ export function getDescriptionResolutionRecipe( const selector: InstrumentSelectorCriteria = { name: otherDescriptor.name, type: otherDescriptor.type, + unit: otherDescriptor.unit, }; const selectorString = JSON.stringify(selector); diff --git a/packages/sdk-metrics/src/view/View.ts b/packages/sdk-metrics/src/view/View.ts index 398f936536..1e8d4fb0e0 100644 --- a/packages/sdk-metrics/src/view/View.ts +++ b/packages/sdk-metrics/src/view/View.ts @@ -83,6 +83,14 @@ export type ViewOptions = { * instrumentName: 'my.instruments.requests' */ instrumentName?: string; + /** + * Instrument selection criteria: + * The unit of the Instrument(s). + * + * @example select all instruments with unit 'ms' + * instrumentUnit: 'ms' + */ + instrumentUnit?: string; /** * Instrument selection criteria: * The name of the Meter. No wildcard support, name must match the meter exactly. @@ -113,6 +121,7 @@ function isSelectorNotProvided(options: ViewOptions): boolean { return ( options.instrumentName == null && options.instrumentType == null && + options.instrumentUnit == null && options.meterName == null && options.meterVersion == null && options.meterSchemaUrl == null @@ -161,6 +170,9 @@ export class View { * @param viewOptions.instrumentType * Instrument selection criteria: * The original type of the Instrument(s). + * @param viewOptions.instrumentUnit + * Instrument selection criteria: + * The unit of the Instrument(s). * @param viewOptions.meterName * Instrument selection criteria: * The name of the Meter. No wildcard support, name must match the meter exactly. @@ -213,6 +225,7 @@ export class View { this.instrumentSelector = new InstrumentSelector({ name: viewOptions.instrumentName, type: viewOptions.instrumentType, + unit: viewOptions.instrumentUnit, }); this.meterSelector = new MeterSelector({ name: viewOptions.meterName, diff --git a/packages/sdk-metrics/src/view/ViewRegistry.ts b/packages/sdk-metrics/src/view/ViewRegistry.ts index 265f699bf9..5f4f367e92 100644 --- a/packages/sdk-metrics/src/view/ViewRegistry.ts +++ b/packages/sdk-metrics/src/view/ViewRegistry.ts @@ -48,7 +48,8 @@ export class ViewRegistry { return ( (selector.getType() === undefined || instrument.type === selector.getType()) && - selector.getNameFilter().match(instrument.name) + selector.getNameFilter().match(instrument.name) && + selector.getUnitFilter().match(instrument.unit) ); } From 614cec3f6aeec031ad1d68557b1c7a3155d682fe Mon Sep 17 00:00:00 2001 From: Julian Labatut Date: Thu, 2 Mar 2023 23:01:48 +0100 Subject: [PATCH 2/4] fix: update CHANGELOG.md (#3647) Signed-off-by: Julian Labatut --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f2bae09b0..096ca1969f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/ * perf(propagator-jaeger): improve deserializeSpanContext performance [#3541](https://github.com/open-telemetry/opentelemetry-js/pull/3541) @doochik * feat: support TraceState in SamplingResult [#3530](https://github.com/open-telemetry/opentelemetry-js/pull/3530) @raphael-theriault-swi * feat(sdk-trace-base): add diagnostic logging when spans are dropped [#3610](https://github.com/open-telemetry/opentelemetry-js/pull/3610) @neoeinstein +* feat: add unit to view instrument selection criteria [#3647](https://github.com/open-telemetry/opentelemetry-js/pull/3647) @jlabatut ### :bug: (Bug Fix) From 17411ddc6b8c19f7c3a5fea49b100edf521bd514 Mon Sep 17 00:00:00 2001 From: Julian Labatut Date: Thu, 2 Mar 2023 23:02:07 +0100 Subject: [PATCH 3/4] test(sdk-metrics): add instrumentUnit tests (#3647) Signed-off-by: Julian Labatut --- .../sdk-metrics/test/MeterProvider.test.ts | 63 +++++++++++++++++++ .../test/view/ViewRegistry.test.ts | 44 +++++++++++++ 2 files changed, 107 insertions(+) diff --git a/packages/sdk-metrics/test/MeterProvider.test.ts b/packages/sdk-metrics/test/MeterProvider.test.ts index 48055695ed..012c434f90 100644 --- a/packages/sdk-metrics/test/MeterProvider.test.ts +++ b/packages/sdk-metrics/test/MeterProvider.test.ts @@ -463,6 +463,69 @@ describe('MeterProvider', () => { } ); }); + + it('with instrument unit should apply view to only the selected instrument unit', async () => { + // Add view that renames 'test-counter-ms' and 'test-counter-s' to 'renamed-instrument' on 'meter1' + const meterProvider = new MeterProvider({ + resource: defaultResource, + views: [ + new View({ + name: 'renamed-instrument', + instrumentName: 'test-counter-ms', + meterName: 'meter1', + }), + new View({ + name: 'renamed-instrument', + instrumentName: 'test-counter-s', + meterName: 'meter1', + }), + ], + }); + + const reader = new TestMetricReader(); + meterProvider.addMetricReader(reader); + + // Create meter and counters, with different units. + const meter = meterProvider.getMeter('meter1', 'v1.0.0'); + meter.createCounter('test-counter-ms', { unit: 'ms' }); + meter.createCounter('test-counter-s', { unit: 's' }); + + // Perform collection. + const { resourceMetrics, errors } = await reader.collect(); + + assert.strictEqual(errors.length, 0); + // Results came only from one Meter + assert.strictEqual(resourceMetrics.scopeMetrics.length, 1); + + // InstrumentationScope matches the only created Meter. + assertScopeMetrics(resourceMetrics.scopeMetrics[0], { + name: 'meter1', + version: 'v1.0.0', + }); + + // Two metrics are collected ('renamed-instrument'-ms and 'renamed-instrument'-s) + assert.strictEqual(resourceMetrics.scopeMetrics[0].metrics.length, 2); + + // Both 'renamed-instrument' are still exported with their units. + assertMetricData( + resourceMetrics.scopeMetrics[0].metrics[0], + DataPointType.SUM, + { + name: 'renamed-instrument', + type: InstrumentType.COUNTER, + unit: 'ms', + } + ); + assertMetricData( + resourceMetrics.scopeMetrics[0].metrics[1], + DataPointType.SUM, + { + name: 'renamed-instrument', + type: InstrumentType.COUNTER, + unit: 's', + } + ); + }); }); describe('shutdown', () => { diff --git a/packages/sdk-metrics/test/view/ViewRegistry.test.ts b/packages/sdk-metrics/test/view/ViewRegistry.test.ts index aea2ebe3cf..b766c0fda5 100644 --- a/packages/sdk-metrics/test/view/ViewRegistry.test.ts +++ b/packages/sdk-metrics/test/view/ViewRegistry.test.ts @@ -101,6 +101,50 @@ describe('ViewRegistry', () => { assert.strictEqual(views[0].name, 'histogram'); } }); + + it('should match view with instrument unit', () => { + const registry = new ViewRegistry(); + registry.addView( + new View({ + name: 'ms_view', + instrumentName: 'default_metric', + instrumentUnit: 'ms', + }) + ); + registry.addView( + new View({ + name: 's_view', + instrumentName: 'default_metric', + instrumentUnit: 's', + }) + ); + + { + const views = registry.findViews( + { + ...defaultInstrumentDescriptor, + unit: 'ms', + }, + defaultInstrumentationScope + ); + + assert.strictEqual(views.length, 1); + assert.strictEqual(views[0].name, 'ms_view'); + } + + { + const views = registry.findViews( + { + ...defaultInstrumentDescriptor, + unit: 's', + }, + defaultInstrumentationScope + ); + + assert.strictEqual(views.length, 1); + assert.strictEqual(views[0].name, 's_view'); + } + }); }); describe('MeterSelector', () => { From e47a58fa37841325297fc8747b6ea51bce581cdc Mon Sep 17 00:00:00 2001 From: Julian Labatut Date: Sat, 4 Mar 2023 19:54:57 +0100 Subject: [PATCH 4/4] test(sdk-metrics): update instrumentUnit tests (#3647) Signed-off-by: Julian Labatut --- .../sdk-metrics/test/MeterProvider.test.ts | 65 ++++++++++--------- 1 file changed, 36 insertions(+), 29 deletions(-) diff --git a/packages/sdk-metrics/test/MeterProvider.test.ts b/packages/sdk-metrics/test/MeterProvider.test.ts index 012c434f90..a9f8361b81 100644 --- a/packages/sdk-metrics/test/MeterProvider.test.ts +++ b/packages/sdk-metrics/test/MeterProvider.test.ts @@ -15,7 +15,13 @@ */ import * as assert from 'assert'; -import { MeterProvider, InstrumentType, DataPointType } from '../src'; +import { + MeterProvider, + InstrumentType, + DataPointType, + ExplicitBucketHistogramAggregation, + HistogramMetricData, +} from '../src'; import { assertScopeMetrics, assertMetricData, @@ -465,19 +471,20 @@ describe('MeterProvider', () => { }); it('with instrument unit should apply view to only the selected instrument unit', async () => { - // Add view that renames 'test-counter-ms' and 'test-counter-s' to 'renamed-instrument' on 'meter1' + // Add views with different boundaries for each unit. + const msBoundaries = [0, 1, 2, 3, 4, 5]; + const sBoundaries = [10, 50, 250, 1000]; + const meterProvider = new MeterProvider({ resource: defaultResource, views: [ new View({ - name: 'renamed-instrument', - instrumentName: 'test-counter-ms', - meterName: 'meter1', + instrumentUnit: 'ms', + aggregation: new ExplicitBucketHistogramAggregation(msBoundaries), }), new View({ - name: 'renamed-instrument', - instrumentName: 'test-counter-s', - meterName: 'meter1', + instrumentUnit: 's', + aggregation: new ExplicitBucketHistogramAggregation(sBoundaries), }), ], }); @@ -485,10 +492,18 @@ describe('MeterProvider', () => { const reader = new TestMetricReader(); meterProvider.addMetricReader(reader); - // Create meter and counters, with different units. + // Create meter and histograms, with different units. const meter = meterProvider.getMeter('meter1', 'v1.0.0'); - meter.createCounter('test-counter-ms', { unit: 'ms' }); - meter.createCounter('test-counter-s', { unit: 's' }); + const histogram1 = meter.createHistogram('test-histogram-ms', { + unit: 'ms', + }); + const histogram2 = meter.createHistogram('test-histogram-s', { + unit: 's', + }); + + // Record values for both. + histogram1.record(1); + histogram2.record(1); // Perform collection. const { resourceMetrics, errors } = await reader.collect(); @@ -503,27 +518,19 @@ describe('MeterProvider', () => { version: 'v1.0.0', }); - // Two metrics are collected ('renamed-instrument'-ms and 'renamed-instrument'-s) + // Two metrics are collected ('test-histogram-ms' and 'test-histogram-s') assert.strictEqual(resourceMetrics.scopeMetrics[0].metrics.length, 2); - // Both 'renamed-instrument' are still exported with their units. - assertMetricData( - resourceMetrics.scopeMetrics[0].metrics[0], - DataPointType.SUM, - { - name: 'renamed-instrument', - type: InstrumentType.COUNTER, - unit: 'ms', - } + // Check if the boundaries are applied to the correct instrument. + assert.deepStrictEqual( + (resourceMetrics.scopeMetrics[0].metrics[0] as HistogramMetricData) + .dataPoints[0].value.buckets.boundaries, + msBoundaries ); - assertMetricData( - resourceMetrics.scopeMetrics[0].metrics[1], - DataPointType.SUM, - { - name: 'renamed-instrument', - type: InstrumentType.COUNTER, - unit: 's', - } + assert.deepStrictEqual( + (resourceMetrics.scopeMetrics[0].metrics[1] as HistogramMetricData) + .dataPoints[0].value.buckets.boundaries, + sBoundaries ); }); });