From af786dd448f664c718624c8aabfa4c0b9d3187d7 Mon Sep 17 00:00:00 2001 From: legendecas Date: Mon, 18 Apr 2022 14:06:24 +0800 Subject: [PATCH 1/2] feat(sdk-metrics-base): meter identity --- experimental/CHANGELOG.md | 1 + .../src/state/MeterProviderSharedState.ts | 16 ++++--- .../src/state/MetricCollector.ts | 5 ++- .../test/MeterProvider.test.ts | 45 +++++++++++++++++++ 4 files changed, 60 insertions(+), 7 deletions(-) diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md index 05fc4d21f2f..f6048f74ef9 100644 --- a/experimental/CHANGELOG.md +++ b/experimental/CHANGELOG.md @@ -39,6 +39,7 @@ All notable changes to experimental packages in this project will be documented * feat(instrumentation-xhr): add applyCustomAttributesOnSpan hook #2134 @mhennoch * feat(proto): add @opentelemetry/otlp-transformer package with hand-rolled transformation #2746 @dyladan * feat(sdk-metrics-base): shutdown and forceflush on MeterProvider #2890 @legendecas +* feat(sdk-metrics-base): meter identity #2901 @legendecas ### :bug: (Bug Fix) diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MeterProviderSharedState.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MeterProviderSharedState.ts index 6c35e650bf8..a4d909ddef7 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MeterProviderSharedState.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MeterProviderSharedState.ts @@ -30,15 +30,21 @@ export class MeterProviderSharedState { metricCollectors: MetricCollector[] = []; - meterSharedStates: MeterSharedState[] = []; + meterSharedStates: Map = new Map(); constructor(public resource: Resource) {} getMeterSharedState(instrumentationLibrary: InstrumentationLibrary) { - // TODO: meter identity - // https://github.com/open-telemetry/opentelemetry-js/issues/2593 - const meterSharedState = new MeterSharedState(this, instrumentationLibrary); - this.meterSharedStates.push(meterSharedState); + const id = this.instrumentationLibraryId(instrumentationLibrary); + let meterSharedState = this.meterSharedStates.get(id); + if (meterSharedState == null) { + meterSharedState = new MeterSharedState(this, instrumentationLibrary); + this.meterSharedStates.set(id, meterSharedState); + } return meterSharedState; } + + instrumentationLibraryId(instrumentationLibrary: InstrumentationLibrary) { + return `${instrumentationLibrary.name}:${instrumentationLibrary.version ?? ''}:${instrumentationLibrary.schemaUrl ?? ''}`; + } } diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricCollector.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricCollector.ts index ac522a293a1..540a5e09179 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricCollector.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricCollector.ts @@ -35,8 +35,9 @@ export class MetricCollector implements MetricProducer { async collect(): Promise { const collectionTime = hrTime(); - const instrumentationLibraryMetrics = (await Promise.all(this._sharedState.meterSharedStates - .map(meterSharedState => meterSharedState.collect(this, collectionTime)))); + const meterCollectionPromises = Array.from(this._sharedState.meterSharedStates.values()) + .map(meterSharedState => meterSharedState.collect(this, collectionTime)); + const instrumentationLibraryMetrics = await Promise.all(meterCollectionPromises); return { resource: this._sharedState.resource, diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/test/MeterProvider.test.ts b/experimental/packages/opentelemetry-sdk-metrics-base/test/MeterProvider.test.ts index bce870c6e63..b1e21fad598 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/test/MeterProvider.test.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/test/MeterProvider.test.ts @@ -56,6 +56,51 @@ describe('MeterProvider', () => { const meter = meterProvider.getMeter('meter1', '1.0.0'); assert.strictEqual(meter, NOOP_METER); }); + + it('get meter with same identity', async () => { + const meterProvider = new MeterProvider({ resource: defaultResource }); + const reader = new TestMetricReader(); + meterProvider.addMetricReader(reader); + + // Create meter and instrument. + // name+version pair 1 + meterProvider.getMeter('meter1', 'v1.0.0'); + meterProvider.getMeter('meter1', 'v1.0.0'); + // name+version pair 2 + meterProvider.getMeter('meter2', 'v1.0.0'); + meterProvider.getMeter('meter2', 'v1.0.0'); + // name+version pair 3 + meterProvider.getMeter('meter1', 'v1.0.1'); + meterProvider.getMeter('meter1', 'v1.0.1'); + // name+version+schemaUrl pair 4 + meterProvider.getMeter('meter1', 'v1.0.1', { schemaUrl: 'https://opentelemetry.io/schemas/1.4.0' }); + meterProvider.getMeter('meter1', 'v1.0.1', { schemaUrl: 'https://opentelemetry.io/schemas/1.4.0' }); + + // Perform collection. + const result = await reader.collect(); + + // Results came only from de-duplicated meters. + assert.strictEqual(result?.instrumentationLibraryMetrics.length, 4); + + // InstrumentationLibrary matches from de-duplicated meters. + assertInstrumentationLibraryMetrics(result?.instrumentationLibraryMetrics[0], { + name: 'meter1', + version: 'v1.0.0' + }); + assertInstrumentationLibraryMetrics(result?.instrumentationLibraryMetrics[1], { + name: 'meter2', + version: 'v1.0.0' + }); + assertInstrumentationLibraryMetrics(result?.instrumentationLibraryMetrics[2], { + name: 'meter1', + version: 'v1.0.1' + }); + assertInstrumentationLibraryMetrics(result?.instrumentationLibraryMetrics[3], { + name: 'meter1', + version: 'v1.0.1', + schemaUrl: 'https://opentelemetry.io/schemas/1.4.0', + }); + }); }); describe('addView', () => { From 3508a7f0ec8d5837338b50b90ddc43bc2be50fe6 Mon Sep 17 00:00:00 2001 From: legendecas Date: Mon, 18 Apr 2022 23:51:40 +0800 Subject: [PATCH 2/2] fixup! --- experimental/CHANGELOG.md | 2 +- .../src/Meter.ts | 8 +---- .../src/MeterProvider.ts | 5 +-- .../src/state/MeterProviderSharedState.ts | 7 ++--- .../src/state/MeterSharedState.ts | 6 +++- .../src/utils.ts | 9 ++++++ .../test/Meter.test.ts | 31 +++++++++++++++---- .../test/MeterProvider.test.ts | 7 +++++ 8 files changed, 53 insertions(+), 22 deletions(-) diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md index f6048f74ef9..58681558d42 100644 --- a/experimental/CHANGELOG.md +++ b/experimental/CHANGELOG.md @@ -39,7 +39,7 @@ All notable changes to experimental packages in this project will be documented * feat(instrumentation-xhr): add applyCustomAttributesOnSpan hook #2134 @mhennoch * feat(proto): add @opentelemetry/otlp-transformer package with hand-rolled transformation #2746 @dyladan * feat(sdk-metrics-base): shutdown and forceflush on MeterProvider #2890 @legendecas -* feat(sdk-metrics-base): meter identity #2901 @legendecas +* feat(sdk-metrics-base): return the same meter for identical input to getMeter #2901 @legendecas ### :bug: (Bug Fix) diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/Meter.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/Meter.ts index 96ca69b0ec1..7009ad3ae44 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/src/Meter.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/Meter.ts @@ -15,21 +15,15 @@ */ import * as metrics from '@opentelemetry/api-metrics'; -import { InstrumentationLibrary } from '@opentelemetry/core'; import { createInstrumentDescriptor, InstrumentType } from './InstrumentDescriptor'; import { CounterInstrument, HistogramInstrument, UpDownCounterInstrument } from './Instruments'; -import { MeterProviderSharedState } from './state/MeterProviderSharedState'; import { MeterSharedState } from './state/MeterSharedState'; /** * This class implements the {@link metrics.Meter} interface. */ export class Meter implements metrics.Meter { - private _meterSharedState: MeterSharedState; - - constructor(meterProviderSharedState: MeterProviderSharedState, instrumentationLibrary: InstrumentationLibrary) { - this._meterSharedState = meterProviderSharedState.getMeterSharedState(instrumentationLibrary); - } + constructor(private _meterSharedState: MeterSharedState) {} /** * Create a {@link metrics.Histogram} instrument. diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/MeterProvider.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/MeterProvider.ts index e27676e35ab..5c0e2b6e1dc 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/src/MeterProvider.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/MeterProvider.ts @@ -17,7 +17,6 @@ import * as api from '@opentelemetry/api'; import * as metrics from '@opentelemetry/api-metrics'; import { Resource } from '@opentelemetry/resources'; -import { Meter } from './Meter'; import { MetricReader } from './export/MetricReader'; import { MeterProviderSharedState } from './state/MeterProviderSharedState'; import { InstrumentSelector } from './view/InstrumentSelector'; @@ -115,7 +114,9 @@ export class MeterProvider implements metrics.MeterProvider { return metrics.NOOP_METER; } - return new Meter(this._sharedState, { name, version, schemaUrl: options.schemaUrl }); + return this._sharedState + .getMeterSharedState({ name, version, schemaUrl: options.schemaUrl }) + .meter; } /** diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MeterProviderSharedState.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MeterProviderSharedState.ts index a4d909ddef7..74e9b869e1e 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MeterProviderSharedState.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MeterProviderSharedState.ts @@ -17,6 +17,7 @@ import { HrTime } from '@opentelemetry/api'; import { hrTime, InstrumentationLibrary } from '@opentelemetry/core'; import { Resource } from '@opentelemetry/resources'; +import { instrumentationLibraryId } from '../utils'; import { ViewRegistry } from '../view/ViewRegistry'; import { MeterSharedState } from './MeterSharedState'; import { MetricCollector } from './MetricCollector'; @@ -35,7 +36,7 @@ export class MeterProviderSharedState { constructor(public resource: Resource) {} getMeterSharedState(instrumentationLibrary: InstrumentationLibrary) { - const id = this.instrumentationLibraryId(instrumentationLibrary); + const id = instrumentationLibraryId(instrumentationLibrary); let meterSharedState = this.meterSharedStates.get(id); if (meterSharedState == null) { meterSharedState = new MeterSharedState(this, instrumentationLibrary); @@ -43,8 +44,4 @@ export class MeterProviderSharedState { } return meterSharedState; } - - instrumentationLibraryId(instrumentationLibrary: InstrumentationLibrary) { - return `${instrumentationLibrary.name}:${instrumentationLibrary.version ?? ''}:${instrumentationLibrary.schemaUrl ?? ''}`; - } } diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MeterSharedState.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MeterSharedState.ts index 912cf54557c..f42cde3053c 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MeterSharedState.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MeterSharedState.ts @@ -19,6 +19,7 @@ import * as metrics from '@opentelemetry/api-metrics'; import { InstrumentationLibrary } from '@opentelemetry/core'; import { InstrumentationLibraryMetrics } from '../export/MetricData'; import { createInstrumentDescriptorWithView, InstrumentDescriptor } from '../InstrumentDescriptor'; +import { Meter } from '../Meter'; import { isNotNullish } from '../utils'; import { AsyncMetricStorage } from './AsyncMetricStorage'; import { MeterProviderSharedState } from './MeterProviderSharedState'; @@ -32,8 +33,11 @@ import { SyncMetricStorage } from './SyncMetricStorage'; */ export class MeterSharedState { private _metricStorageRegistry = new MetricStorageRegistry(); + meter: Meter; - constructor(private _meterProviderSharedState: MeterProviderSharedState, private _instrumentationLibrary: InstrumentationLibrary) {} + constructor(private _meterProviderSharedState: MeterProviderSharedState, private _instrumentationLibrary: InstrumentationLibrary) { + this.meter = new Meter(this); + } registerMetricStorage(descriptor: InstrumentDescriptor) { const views = this._meterProviderSharedState.viewRegistry.findViews(descriptor, this._instrumentationLibrary); diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/utils.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/utils.ts index 96475d3be52..3b08c43b78c 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/src/utils.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/utils.ts @@ -15,6 +15,7 @@ */ import { MetricAttributes } from '@opentelemetry/api-metrics'; +import { InstrumentationLibrary } from '@opentelemetry/core'; export type Maybe = T | undefined; @@ -39,6 +40,14 @@ export function hashAttributes(attributes: MetricAttributes): string { }, '|#'); } +/** + * Converting the instrumentation library object to a unique identifier string. + * @param instrumentationLibrary + */ +export function instrumentationLibraryId(instrumentationLibrary: InstrumentationLibrary): string { + return `${instrumentationLibrary.name}:${instrumentationLibrary.version ?? ''}:${instrumentationLibrary.schemaUrl ?? ''}`; +} + /** * Error that is thrown on timeouts. */ diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/test/Meter.test.ts b/experimental/packages/opentelemetry-sdk-metrics-base/test/Meter.test.ts index 40d22831cd1..75bb2408909 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/test/Meter.test.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/test/Meter.test.ts @@ -19,6 +19,7 @@ import * as assert from 'assert'; import { CounterInstrument, HistogramInstrument, UpDownCounterInstrument } from '../src/Instruments'; import { Meter } from '../src/Meter'; import { MeterProviderSharedState } from '../src/state/MeterProviderSharedState'; +import { MeterSharedState } from '../src/state/MeterSharedState'; import { defaultInstrumentationLibrary, defaultResource } from './util'; const noopObservableCallback: ObservableCallback = _observableResult => {}; @@ -26,7 +27,10 @@ const noopObservableCallback: ObservableCallback = _observableResult => {}; describe('Meter', () => { describe('createCounter', () => { it('should create counter', () => { - const meter = new Meter(new MeterProviderSharedState(defaultResource), defaultInstrumentationLibrary); + const meterSharedState = new MeterSharedState( + new MeterProviderSharedState(defaultResource), + defaultInstrumentationLibrary); + const meter = new Meter(meterSharedState); const counter = meter.createCounter('foobar'); assert(counter instanceof CounterInstrument); }); @@ -34,7 +38,10 @@ describe('Meter', () => { describe('createUpDownCounter', () => { it('should create up down counter', () => { - const meter = new Meter(new MeterProviderSharedState(defaultResource), defaultInstrumentationLibrary); + const meterSharedState = new MeterSharedState( + new MeterProviderSharedState(defaultResource), + defaultInstrumentationLibrary); + const meter = new Meter(meterSharedState); const counter = meter.createUpDownCounter('foobar'); assert(counter instanceof UpDownCounterInstrument); }); @@ -42,7 +49,10 @@ describe('Meter', () => { describe('createHistogram', () => { it('should create histogram', () => { - const meter = new Meter(new MeterProviderSharedState(defaultResource), defaultInstrumentationLibrary); + const meterSharedState = new MeterSharedState( + new MeterProviderSharedState(defaultResource), + defaultInstrumentationLibrary); + const meter = new Meter(meterSharedState); const counter = meter.createHistogram('foobar'); assert(counter instanceof HistogramInstrument); }); @@ -50,21 +60,30 @@ describe('Meter', () => { describe('createObservableGauge', () => { it('should create observable gauge', () => { - const meter = new Meter(new MeterProviderSharedState(defaultResource), defaultInstrumentationLibrary); + const meterSharedState = new MeterSharedState( + new MeterProviderSharedState(defaultResource), + defaultInstrumentationLibrary); + const meter = new Meter(meterSharedState); meter.createObservableGauge('foobar', noopObservableCallback); }); }); describe('createObservableCounter', () => { it('should create observable counter', () => { - const meter = new Meter(new MeterProviderSharedState(defaultResource), defaultInstrumentationLibrary); + const meterSharedState = new MeterSharedState( + new MeterProviderSharedState(defaultResource), + defaultInstrumentationLibrary); + const meter = new Meter(meterSharedState); meter.createObservableCounter('foobar', noopObservableCallback); }); }); describe('createObservableUpDownCounter', () => { it('should create observable up-down-counter', () => { - const meter = new Meter(new MeterProviderSharedState(defaultResource), defaultInstrumentationLibrary); + const meterSharedState = new MeterSharedState( + new MeterProviderSharedState(defaultResource), + defaultInstrumentationLibrary); + const meter = new Meter(meterSharedState); meter.createObservableUpDownCounter('foobar', noopObservableCallback); }); }); diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/test/MeterProvider.test.ts b/experimental/packages/opentelemetry-sdk-metrics-base/test/MeterProvider.test.ts index b1e21fad598..814044891a4 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/test/MeterProvider.test.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/test/MeterProvider.test.ts @@ -50,6 +50,13 @@ describe('MeterProvider', () => { assert(meter instanceof Meter); }); + it('should get an identical meter on duplicated calls', () => { + const meterProvider = new MeterProvider(); + const meter1 = meterProvider.getMeter('meter1', '1.0.0'); + const meter2 = meterProvider.getMeter('meter1', '1.0.0'); + assert.strictEqual(meter1, meter2); + }); + it('get a noop meter on shutdown', () => { const meterProvider = new MeterProvider(); meterProvider.shutdown();