feat: add the ability to set the views via the SDK constructor (open-…

…telemetry#3124) * feat: add the ability to set the views via the SDK constructor * docs: update CHANGELOG.md * test: added test for checking if views configured when passed to the SDK * test: update test to check view got applied by renaming an counter metric's name * style: run `lint:fix` command on package * style: remove unused imports * style: fix identation linting issue in `sdk.test.ts` * docs: add reference to views-parameter of the NodeSDK * fix: throw an error when NodeSDK is used incorrectly * fix: improve the way handling views without metric reader * chore: update code * test: update the test cases * fix: update reader, if needed Co-authored-by: Weyert de Boer <[email protected]>
weyert · Aug 17, 2022 · 43f4e5a · 43f4e5a
1 parent 6807def
commit 43f4e5a
Show file tree

Hide file tree

Showing 5 changed files with 199 additions and 16 deletions.
diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md
@@ -10,6 +10,7 @@ All notable changes to experimental packages in this project will be documented
 
 ### :rocket: (Enhancement)
 
+* fix(add-views-to-node-sdk): added the ability to define meter views in `NodeSDK` [#3066](https://github.com/open-telemetry/opentelemetry-js/pull/3124) @weyert
 * feature(add-console-metrics-exporter): add ConsoleMetricExporter [#3120](https://github.com/open-telemetry/opentelemetry-js/pull/3120) @weyert
 * feature(prometheus-serialiser): export the unit block when unit is set in metric descriptor [#3066](https://github.com/open-telemetry/opentelemetry-js/pull/3041) @weyert
 

diff --git a/experimental/packages/opentelemetry-sdk-node/README.md b/experimental/packages/opentelemetry-sdk-node/README.md
@@ -128,6 +128,10 @@ Configure a trace exporter. If an exporter OR span processor is not configured,
 
 Configure tracing parameters. These are the same trace parameters used to [configure a tracer](../../../packages/opentelemetry-sdk-trace-base/src/types.ts#L71).
 
+### views
+
+Configure views of your instruments and accepts an array of [View](../opentelemetry-sdk-metrics-base/src/view/View.ts)-instances. The parameter can be used to configure the explicit bucket sizes of histogram metrics.
+
 ### serviceName
 
 Configure the [service name](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/resource/semantic_conventions/README.md#service).

diff --git a/experimental/packages/opentelemetry-sdk-node/src/sdk.ts b/experimental/packages/opentelemetry-sdk-node/src/sdk.ts
@@ -27,7 +27,7 @@ import {
   Resource,
   ResourceDetectionConfig
 } from '@opentelemetry/resources';
-import { MeterProvider, MetricReader } from '@opentelemetry/sdk-metrics';
+import { MeterProvider, MetricReader, View } from '@opentelemetry/sdk-metrics';
 import {
   BatchSpanProcessor,
   SpanProcessor
@@ -37,15 +37,29 @@ import { SemanticResourceAttributes } from '@opentelemetry/semantic-conventions'
 import { NodeSDKConfiguration } from './types';
 
 /** This class represents everything needed to register a fully configured OpenTelemetry Node.js SDK */
+
+export type MeterProviderConfig = {
+  /**
+   * Reference to the MetricReader instance by the NodeSDK
+   */
+  reader?: MetricReader
+  /**
+   * Lists the views that should be passed when meterProvider
+   *
+   * Note: This is only getting used when NodeSDK is responsible for
+   * instantiated an instance of MeterProvider
+   */
+  views?: View[]
+};
 export class NodeSDK {
   private _tracerProviderConfig?: {
     tracerConfig: NodeTracerConfig;
     spanProcessor: SpanProcessor;
     contextManager?: ContextManager;
     textMapPropagator?: TextMapPropagator;
   };
+  private _meterProviderConfig?: MeterProviderConfig;
   private _instrumentations: InstrumentationOption[];
-  private _metricReader?: MetricReader;
 
   private _resource: Resource;
 
@@ -87,8 +101,17 @@ export class NodeSDK {
       );
     }
 
-    if (configuration.metricReader) {
-      this.configureMeterProvider(configuration.metricReader);
+    if (configuration.metricReader || configuration.views) {
+      const meterProviderConfig: MeterProviderConfig = {};
+      if (configuration.metricReader) {
+        meterProviderConfig.reader = configuration.metricReader;
+      }
+
+      if (configuration.views) {
+        meterProviderConfig.views = configuration.views;
+      }
+
+      this.configureMeterProvider(meterProviderConfig);
     }
 
     let instrumentations: InstrumentationOption[] = [];
@@ -114,16 +137,40 @@ export class NodeSDK {
   }
 
   /** Set configurations needed to register a MeterProvider */
-  public configureMeterProvider(reader: MetricReader): void {
-    this._metricReader = reader;
+  public configureMeterProvider(config: MeterProviderConfig): void {
+    // nothing is set yet, we can set config and return.
+    if (this._meterProviderConfig == null) {
+      this._meterProviderConfig = config;
+      return;
+    }
+
+    // make sure we do not override existing views with other views.
+    if (this._meterProviderConfig.views != null && config.views != null) {
+      throw new Error('Views passed but Views have already been configured.');
+    }
+
+    // set views, but make sure we do not override existing views with null/undefined.
+    if (config.views != null) {
+      this._meterProviderConfig.views = config.views;
+    }
+
+    // make sure we do not override existing reader with another reader.
+    if (this._meterProviderConfig.reader != null && config.reader != null) {
+      throw new Error('MetricReader passed but MetricReader has already been configured.');
+    }
+
+    // set reader, but make sure we do not override existing reader with null/undefined.
+    if (config.reader != null) {
+      this._meterProviderConfig.reader = config.reader;
+    }
   }
 
   /** Detect resource attributes */
   public async detectResources(
     config?: ResourceDetectionConfig
   ): Promise<void> {
     const internalConfig: ResourceDetectionConfig = {
-      detectors: [ envDetector, processDetector],
+      detectors: [envDetector, processDetector],
       ...config,
     };
 
@@ -146,7 +193,7 @@ export class NodeSDK {
     this._resource = this._serviceName === undefined
       ? this._resource
       : this._resource.merge(new Resource(
-        {[SemanticResourceAttributes.SERVICE_NAME]: this._serviceName}
+        { [SemanticResourceAttributes.SERVICE_NAME]: this._serviceName }
       ));
 
     if (this._tracerProviderConfig) {
@@ -164,12 +211,15 @@ export class NodeSDK {
       });
     }
 
-    if (this._metricReader) {
+    if (this._meterProviderConfig) {
       const meterProvider = new MeterProvider({
         resource: this._resource,
+        views: this._meterProviderConfig?.views ?? [],
       });
 
-      meterProvider.addMetricReader(this._metricReader);
+      if (this._meterProviderConfig.reader) {
+        meterProvider.addMetricReader(this._meterProviderConfig.reader);
+      }
 
       this._meterProvider = meterProvider;
 

diff --git a/experimental/packages/opentelemetry-sdk-node/src/types.ts b/experimental/packages/opentelemetry-sdk-node/src/types.ts
@@ -18,7 +18,7 @@ import type { ContextManager, SpanAttributes } from '@opentelemetry/api';
 import { TextMapPropagator } from '@opentelemetry/api';
 import { InstrumentationOption } from '@opentelemetry/instrumentation';
 import { Resource } from '@opentelemetry/resources';
-import { MetricReader } from '@opentelemetry/sdk-metrics';
+import { MetricReader, View } from '@opentelemetry/sdk-metrics';
 import {
   Sampler,
   SpanExporter,
@@ -32,6 +32,7 @@ export interface NodeSDKConfiguration {
   defaultAttributes: SpanAttributes;
   textMapPropagator: TextMapPropagator;
   metricReader: MetricReader;
+  views: View[]
   instrumentations: InstrumentationOption[];
   resource: Resource;
   sampler: Sampler;

diff --git a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts
@@ -28,7 +28,7 @@ import {
   AsyncLocalStorageContextManager,
 } from '@opentelemetry/context-async-hooks';
 import { CompositePropagator } from '@opentelemetry/core';
-import { ConsoleMetricExporter, MeterProvider, PeriodicExportingMetricReader } from '@opentelemetry/sdk-metrics';
+import { AggregationTemporality, ConsoleMetricExporter, InMemoryMetricExporter, InstrumentType, MeterProvider, PeriodicExportingMetricReader, View } from '@opentelemetry/sdk-metrics';
 import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node';
 import {
   assertServiceResource,
@@ -147,6 +147,133 @@ describe('Node SDK', () => {
     });
   });
 
+  async function waitForNumberOfMetrics(exporter: InMemoryMetricExporter, numberOfMetrics: number): Promise<void> {
+    if (numberOfMetrics <= 0) {
+      throw new Error('numberOfMetrics must be greater than or equal to 0');
+    }
+
+    let totalExports = 0;
+    while (totalExports < numberOfMetrics) {
+      await new Promise(resolve => setTimeout(resolve, 20));
+      const exportedMetrics = exporter.getMetrics();
+      totalExports = exportedMetrics.length;
+    }
+  }
+
+  it('should register meter views when provided', async () => {
+    const exporter = new InMemoryMetricExporter(AggregationTemporality.CUMULATIVE);
+    const metricReader = new PeriodicExportingMetricReader({
+      exporter: exporter,
+      exportIntervalMillis: 100,
+      exportTimeoutMillis: 100
+    });
+
+    const sdk = new NodeSDK({
+      metricReader: metricReader,
+      views: [
+        new View({
+          name: 'test-view',
+          instrumentName: 'test_counter',
+          instrumentType: InstrumentType.COUNTER,
+        })
+      ],
+      autoDetectResources: false,
+    });
+
+    await sdk.start();
+
+    assert.strictEqual(context['_getContextManager'](), ctxManager, 'context manager should not change');
+    assert.strictEqual(propagation['_getGlobalPropagator'](), propagator, 'propagator should not change');
+    assert.strictEqual((trace.getTracerProvider() as ProxyTracerProvider).getDelegate(), delegate, 'tracer provider should not have changed');
+
+    const meterProvider = metrics.getMeterProvider() as MeterProvider;
+    assert.ok(meterProvider);
+
+    const meter = meterProvider.getMeter('NodeSDKViews', '1.0.0');
+    const counter = meter.createCounter('test_counter', {
+      description: 'a test description',
+    });
+    counter.add(10);
+
+    await waitForNumberOfMetrics(exporter, 1);
+    const exportedMetrics = exporter.getMetrics();
+    const [firstExportedMetric] = exportedMetrics;
+    assert.ok(firstExportedMetric, 'should have one exported metric');
+    const [firstScopeMetric] = firstExportedMetric.scopeMetrics;
+    assert.ok(firstScopeMetric, 'should have one scope metric');
+    assert.ok(firstScopeMetric.scope.name === 'NodeSDKViews', 'scope should match created view');
+    assert.ok(firstScopeMetric.metrics.length > 0, 'should have at least one metrics entry');
+    const [firstMetricRecord] = firstScopeMetric.metrics;
+    assert.ok(firstMetricRecord.descriptor.name === 'test-view', 'should have renamed counter metric');
+
+    await sdk.shutdown();
+  });
+
+  it('should throw error when calling configureMeterProvider when views are already configured', () => {
+    const exporter = new InMemoryMetricExporter(AggregationTemporality.CUMULATIVE);
+    const metricReader = new PeriodicExportingMetricReader({
+      exporter: exporter,
+      exportIntervalMillis: 100,
+      exportTimeoutMillis: 100
+    });
+
+    const sdk = new NodeSDK({
+      metricReader: metricReader,
+      views: [
+        new View({
+          name: 'test-view',
+          instrumentName: 'test_counter',
+          instrumentType: InstrumentType.COUNTER,
+        })
+      ],
+      autoDetectResources: false,
+    });
+
+    assert.throws(() => {
+      sdk.configureMeterProvider({
+        reader: metricReader,
+        views: [
+          new View({
+            name: 'test-view',
+            instrumentName: 'test_counter',
+            instrumentType: InstrumentType.COUNTER,
+          })
+        ]
+      });
+    }, (error: Error) => {
+      return error.message.includes('Views passed but Views have already been configured');
+    });
+  });
+
+  it('should throw error when calling configureMeterProvider when metricReader is already configured', () => {
+    const exporter = new InMemoryMetricExporter(AggregationTemporality.CUMULATIVE);
+    const metricReader = new PeriodicExportingMetricReader({
+      exporter: exporter,
+      exportIntervalMillis: 100,
+      exportTimeoutMillis: 100
+    });
+
+    const sdk = new NodeSDK({
+      metricReader: metricReader,
+      views: [
+        new View({
+          name: 'test-view',
+          instrumentName: 'test_counter',
+          instrumentType: InstrumentType.COUNTER,
+        })
+      ],
+      autoDetectResources: false,
+    });
+
+    assert.throws(() => {
+      sdk.configureMeterProvider({
+        reader: metricReader,
+      });
+    }, (error: Error) => {
+      return error.message.includes('MetricReader passed but MetricReader has already been configured.');
+    });
+  });
+
   describe('detectResources', async () => {
     beforeEach(() => {
       process.env.OTEL_RESOURCE_ATTRIBUTES =
@@ -163,12 +290,12 @@ describe('Node SDK', () => {
           autoDetectResources: true,
         });
         await sdk.detectResources({
-          detectors: [ processDetector, {
+          detectors: [processDetector, {
             detect() {
               throw new Error('Buggy detector');
             }
           },
-          envDetector ]
+          envDetector]
         });
         const resource = sdk['_resource'];
 
@@ -277,7 +404,7 @@ describe('Node SDK', () => {
     });
 
     it('should configure service name via OTEL_SERVICE_NAME env var', async () => {
-      process.env.OTEL_SERVICE_NAME='env-set-name';
+      process.env.OTEL_SERVICE_NAME = 'env-set-name';
       const sdk = new NodeSDK();
 
       await sdk.start();
@@ -290,7 +417,7 @@ describe('Node SDK', () => {
     });
 
     it('should favor config set service name over OTEL_SERVICE_NAME env set service name', async () => {
-      process.env.OTEL_SERVICE_NAME='env-set-name';
+      process.env.OTEL_SERVICE_NAME = 'env-set-name';
       const sdk = new NodeSDK({
         serviceName: 'config-set-name',
       });