feat(sdk-metrics-base): meter identity (#2901)

open-telemetry · Apr 19, 2022 · 2ba6dd5 · 2ba6dd5
1 parent ed2f033
commit 2ba6dd5
Show file tree

Hide file tree

Showing 9 changed files with 107 additions and 23 deletions.
diff --git 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): 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
@@ -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
@@ -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
@@ -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';
@@ -30,15 +31,17 @@ export class MeterProviderSharedState {
 
   metricCollectors: MetricCollector[] = [];
 
-  meterSharedStates: MeterSharedState[] = [];
+  meterSharedStates: Map<string, MeterSharedState> = 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 = instrumentationLibraryId(instrumentationLibrary);
+    let meterSharedState = this.meterSharedStates.get(id);
+    if (meterSharedState == null) {
+      meterSharedState = new MeterSharedState(this, instrumentationLibrary);
+      this.meterSharedStates.set(id, meterSharedState);
+    }
     return meterSharedState;
   }
 }
diff --git 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/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<ResourceMetrics> {
     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/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> = 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
@@ -19,52 +19,71 @@ 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 => {};
 
 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);
     });
   });
 
   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);
     });
   });
 
   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);
     });
   });
 
   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
@@ -50,12 +50,64 @@ 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();
       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', () => {