From 73618d319442e5d67083d656ddee9ff9f8307760 Mon Sep 17 00:00:00 2001 From: Weyert de Boer Date: Fri, 29 Jul 2022 00:13:31 +0100 Subject: [PATCH 01/13] feat: add the ability to set the views via the SDK constructor --- .../opentelemetry-sdk-node/src/sdk.ts | 11 +++++-- .../opentelemetry-sdk-node/src/types.ts | 3 +- .../opentelemetry-sdk-node/test/sdk.test.ts | 33 ++++++++++++++++++- 3 files changed, 43 insertions(+), 4 deletions(-) diff --git a/experimental/packages/opentelemetry-sdk-node/src/sdk.ts b/experimental/packages/opentelemetry-sdk-node/src/sdk.ts index f1f4815829..aaad7311dc 100644 --- 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-base'; +import { MeterProvider, MetricReader, View } from '@opentelemetry/sdk-metrics-base'; import { BatchSpanProcessor, SpanProcessor @@ -44,6 +44,9 @@ export class NodeSDK { contextManager?: ContextManager; textMapPropagator?: TextMapPropagator; }; + private _meterProviderConfig?: { + views?: View[] + }; private _instrumentations: InstrumentationOption[]; private _metricReader?: MetricReader; @@ -114,8 +117,11 @@ export class NodeSDK { } /** Set configurations needed to register a MeterProvider */ - public configureMeterProvider(reader: MetricReader): void { + public configureMeterProvider(reader: MetricReader, views?: View[]): void { this._metricReader = reader; + this._meterProviderConfig = { + views, + }; } /** Detect resource attributes */ @@ -167,6 +173,7 @@ export class NodeSDK { if (this._metricReader) { const meterProvider = new MeterProvider({ resource: this._resource, + views: this._meterProviderConfig?.views ?? [], }); meterProvider.addMetricReader(this._metricReader); diff --git a/experimental/packages/opentelemetry-sdk-node/src/types.ts b/experimental/packages/opentelemetry-sdk-node/src/types.ts index 90c1e08401..82594b5efd 100644 --- 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 { Sampler, TextMapPropagator } from '@opentelemetry/api'; import { InstrumentationOption } from '@opentelemetry/instrumentation'; import { Resource } from '@opentelemetry/resources'; -import { MetricReader } from '@opentelemetry/sdk-metrics-base'; +import { MetricReader, View } from '@opentelemetry/sdk-metrics-base'; import { SpanExporter, SpanLimits, @@ -31,6 +31,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 index 217800c9c0..ea7ecf5854 100644 --- 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-base'; +import { ConsoleMetricExporter, InstrumentType, MeterProvider, PeriodicExportingMetricReader, View } from '@opentelemetry/sdk-metrics-base'; import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node'; import { assertServiceResource, @@ -147,6 +147,37 @@ describe('Node SDK', () => { }); }); + it('should register meter views when provider', async () => { + const exporter = new ConsoleMetricExporter(); + 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'); + + assert.ok(metrics.getMeterProvider() instanceof MeterProvider); + + await sdk.shutdown(); + }); + describe('detectResources', async () => { beforeEach(() => { process.env.OTEL_RESOURCE_ATTRIBUTES = From 65e7e795208b7b0764ff4df43e514c6547d11e5b Mon Sep 17 00:00:00 2001 From: Weyert de Boer Date: Fri, 29 Jul 2022 00:19:21 +0100 Subject: [PATCH 02/13] docs: update CHANGELOG.md --- experimental/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md index 8ac2ddd66c..1505753eec 100644 --- a/experimental/CHANGELOG.md +++ b/experimental/CHANGELOG.md @@ -8,6 +8,8 @@ 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 + ### :bug: (Bug Fix) ### :books: (Refine Doc) From 878495479f3694f623f78726c7a8000c9c44c229 Mon Sep 17 00:00:00 2001 From: Weyert de Boer Date: Sun, 31 Jul 2022 15:34:33 +0100 Subject: [PATCH 03/13] test: added test for checking if views configured when passed to the SDK --- .../opentelemetry-sdk-node/src/sdk.ts | 2 +- .../opentelemetry-sdk-node/test/sdk.test.ts | 21 ++++++++++++++----- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/experimental/packages/opentelemetry-sdk-node/src/sdk.ts b/experimental/packages/opentelemetry-sdk-node/src/sdk.ts index aaad7311dc..ddf0b0a3ca 100644 --- a/experimental/packages/opentelemetry-sdk-node/src/sdk.ts +++ b/experimental/packages/opentelemetry-sdk-node/src/sdk.ts @@ -91,7 +91,7 @@ export class NodeSDK { } if (configuration.metricReader) { - this.configureMeterProvider(configuration.metricReader); + this.configureMeterProvider(configuration.metricReader, configuration.views); } let instrumentations: InstrumentationOption[] = []; diff --git a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts index ea7ecf5854..1545f9ae41 100644 --- a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts +++ b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts @@ -22,13 +22,13 @@ import { diag, DiagLogLevel, } from '@opentelemetry/api'; -import { metrics, NoopMeterProvider } from '@opentelemetry/api-metrics'; +import { metrics, NoopMeterProvider, ValueType } from '@opentelemetry/api-metrics'; import { AsyncHooksContextManager, AsyncLocalStorageContextManager, } from '@opentelemetry/context-async-hooks'; import { CompositePropagator } from '@opentelemetry/core'; -import { ConsoleMetricExporter, InstrumentType, MeterProvider, PeriodicExportingMetricReader, View } from '@opentelemetry/sdk-metrics-base'; +import { ConsoleMetricExporter, InstrumentDescriptor, InstrumentType, MeterProvider, PeriodicExportingMetricReader, View } from '@opentelemetry/sdk-metrics-base'; import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node'; import { assertServiceResource, @@ -147,7 +147,7 @@ describe('Node SDK', () => { }); }); - it('should register meter views when provider', async () => { + it('should register meter views when provided', async () => { const exporter = new ConsoleMetricExporter(); const metricReader = new PeriodicExportingMetricReader({ exporter: exporter, @@ -155,6 +155,14 @@ describe('Node SDK', () => { exportTimeoutMillis: 100 }); + const metricDescriptor: InstrumentDescriptor = { + name: 'test-view', + description: 'Metric', + unit: '1', + type: InstrumentType.COUNTER, + valueType: ValueType.INT + }; + const sdk = new NodeSDK({ metricReader: metricReader, views: [ @@ -173,8 +181,11 @@ describe('Node SDK', () => { assert.strictEqual(propagation['_getGlobalPropagator'](), propagator, 'propagator should not change'); assert.strictEqual((trace.getTracerProvider() as ProxyTracerProvider).getDelegate(), delegate, 'tracer provider should not have changed'); - assert.ok(metrics.getMeterProvider() instanceof MeterProvider); - + const meterProvider = metrics.getMeterProvider() as MeterProvider; + assert.ok(meterProvider); + const viewRegistry = meterProvider['_sharedState'].viewRegistry; + assert.ok(viewRegistry['_registeredViews'].find(view => view.name === 'test-view')); + assert.ok(viewRegistry.findViews(metricDescriptor, { name: 'test-view' })); await sdk.shutdown(); }); From 82e4c1f587dfa7f84aa330aaf443b3cc61284432 Mon Sep 17 00:00:00 2001 From: Weyert de Boer Date: Thu, 4 Aug 2022 18:03:47 +0100 Subject: [PATCH 04/13] test: update test to check view got applied by renaming an counter metric's name --- .../opentelemetry-sdk-node/test/sdk.test.ts | 48 +++++++++++++------ 1 file changed, 34 insertions(+), 14 deletions(-) diff --git a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts index 1545f9ae41..70d72b6eb7 100644 --- a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts +++ b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts @@ -27,8 +27,8 @@ import { AsyncHooksContextManager, AsyncLocalStorageContextManager, } from '@opentelemetry/context-async-hooks'; -import { CompositePropagator } from '@opentelemetry/core'; -import { ConsoleMetricExporter, InstrumentDescriptor, InstrumentType, MeterProvider, PeriodicExportingMetricReader, View } from '@opentelemetry/sdk-metrics-base'; +import { CompositePropagator, ExportResult } from '@opentelemetry/core'; +import { AggregationTemporality, ConsoleMetricExporter, InMemoryMetricExporter, InstrumentDescriptor, InstrumentType, MeterProvider, PeriodicExportingMetricReader, ResourceMetrics, View } from '@opentelemetry/sdk-metrics-base'; import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node'; import { assertServiceResource, @@ -148,21 +148,26 @@ describe('Node SDK', () => { }); it('should register meter views when provided', async () => { - const exporter = new ConsoleMetricExporter(); + async function waitForNumberOfMetrics(exporter: InMemoryMetricExporter, numberOfMetrics: number): Promise { + 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; + } + } + + const exporter = new InMemoryMetricExporter(AggregationTemporality.CUMULATIVE); const metricReader = new PeriodicExportingMetricReader({ exporter: exporter, exportIntervalMillis: 100, exportTimeoutMillis: 100 }); - const metricDescriptor: InstrumentDescriptor = { - name: 'test-view', - description: 'Metric', - unit: '1', - type: InstrumentType.COUNTER, - valueType: ValueType.INT - }; - const sdk = new NodeSDK({ metricReader: metricReader, views: [ @@ -183,9 +188,24 @@ describe('Node SDK', () => { const meterProvider = metrics.getMeterProvider() as MeterProvider; assert.ok(meterProvider); - const viewRegistry = meterProvider['_sharedState'].viewRegistry; - assert.ok(viewRegistry['_registeredViews'].find(view => view.name === 'test-view')); - assert.ok(viewRegistry.findViews(metricDescriptor, { name: 'test-view' })); + + 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(); }); From 2c61e531463df8896ebe74ea36c1175ee99a8371 Mon Sep 17 00:00:00 2001 From: Weyert de Boer Date: Fri, 5 Aug 2022 10:22:49 +0000 Subject: [PATCH 05/13] style: run `lint:fix` command on package --- .../opentelemetry-sdk-node/test/sdk.test.ts | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts index 70d72b6eb7..f338df4c59 100644 --- a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts +++ b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts @@ -156,7 +156,7 @@ describe('Node SDK', () => { let totalExports = 0; while (totalExports < numberOfMetrics) { await new Promise(resolve => setTimeout(resolve, 20)); - const exportedMetrics = exporter.getMetrics() + const exportedMetrics = exporter.getMetrics(); totalExports = exportedMetrics.length; } } @@ -193,18 +193,18 @@ describe('Node SDK', () => { 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') + 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(); }); From 696fa99a9f27fb9ee287225778926eac781f647f Mon Sep 17 00:00:00 2001 From: Weyert de Boer Date: Fri, 5 Aug 2022 11:32:25 +0100 Subject: [PATCH 06/13] style: remove unused imports --- .../opentelemetry-sdk-node/test/sdk.test.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts index f338df4c59..ba865318bd 100644 --- a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts +++ b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts @@ -22,13 +22,13 @@ import { diag, DiagLogLevel, } from '@opentelemetry/api'; -import { metrics, NoopMeterProvider, ValueType } from '@opentelemetry/api-metrics'; +import { metrics, NoopMeterProvider } from '@opentelemetry/api-metrics'; import { AsyncHooksContextManager, AsyncLocalStorageContextManager, } from '@opentelemetry/context-async-hooks'; -import { CompositePropagator, ExportResult } from '@opentelemetry/core'; -import { AggregationTemporality, ConsoleMetricExporter, InMemoryMetricExporter, InstrumentDescriptor, InstrumentType, MeterProvider, PeriodicExportingMetricReader, ResourceMetrics, View } from '@opentelemetry/sdk-metrics-base'; +import { CompositePropagator } from '@opentelemetry/core'; +import { AggregationTemporality, ConsoleMetricExporter, InMemoryMetricExporter, InstrumentType, MeterProvider, PeriodicExportingMetricReader, View } from '@opentelemetry/sdk-metrics-base'; import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node'; import { assertServiceResource, @@ -225,12 +225,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']; @@ -339,7 +339,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(); @@ -352,7 +352,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', }); From 2f0b3d32dbb892f9c8cdc738caec409563fe24a8 Mon Sep 17 00:00:00 2001 From: Weyert de Boer Date: Fri, 5 Aug 2022 10:49:42 +0000 Subject: [PATCH 07/13] style: fix identation linting issue in `sdk.test.ts` --- experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts index ba865318bd..46a32e6f38 100644 --- a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts +++ b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts @@ -230,7 +230,7 @@ describe('Node SDK', () => { throw new Error('Buggy detector'); } }, - envDetector] + envDetector] }); const resource = sdk['_resource']; From 9e812974007388918e0d439194860d615e56fe66 Mon Sep 17 00:00:00 2001 From: Weyert de Boer Date: Fri, 5 Aug 2022 11:54:40 +0100 Subject: [PATCH 08/13] docs: add reference to views-parameter of the NodeSDK --- experimental/packages/opentelemetry-sdk-node/README.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/experimental/packages/opentelemetry-sdk-node/README.md b/experimental/packages/opentelemetry-sdk-node/README.md index a21de3c741..5b288ba1f3 100644 --- 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). From f0d66eb3d7ff567a3b8890e561ab145f22fa095f Mon Sep 17 00:00:00 2001 From: Weyert de Boer Date: Fri, 5 Aug 2022 14:33:39 +0000 Subject: [PATCH 09/13] fix: throw an error when NodeSDK is used incorrectly --- .../opentelemetry-sdk-node/src/sdk.ts | 42 ++++++++++++------- .../opentelemetry-sdk-node/test/sdk.test.ts | 18 ++++++++ 2 files changed, 44 insertions(+), 16 deletions(-) diff --git a/experimental/packages/opentelemetry-sdk-node/src/sdk.ts b/experimental/packages/opentelemetry-sdk-node/src/sdk.ts index ddf0b0a3ca..a7077188a3 100644 --- a/experimental/packages/opentelemetry-sdk-node/src/sdk.ts +++ b/experimental/packages/opentelemetry-sdk-node/src/sdk.ts @@ -37,6 +37,20 @@ 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; @@ -44,11 +58,8 @@ export class NodeSDK { contextManager?: ContextManager; textMapPropagator?: TextMapPropagator; }; - private _meterProviderConfig?: { - views?: View[] - }; + private _meterProviderConfig?: MeterProviderConfig; private _instrumentations: InstrumentationOption[]; - private _metricReader?: MetricReader; private _resource: Resource; @@ -90,9 +101,7 @@ export class NodeSDK { ); } - if (configuration.metricReader) { - this.configureMeterProvider(configuration.metricReader, configuration.views); - } + this.configureMeterProvider({ reader: configuration.metricReader, views: configuration.views }); let instrumentations: InstrumentationOption[] = []; if (configuration.instrumentations) { @@ -117,11 +126,12 @@ export class NodeSDK { } /** Set configurations needed to register a MeterProvider */ - public configureMeterProvider(reader: MetricReader, views?: View[]): void { - this._metricReader = reader; - this._meterProviderConfig = { - views, - }; + public configureMeterProvider(config: MeterProviderConfig): void { + if (config?.views && !config.reader) { + throw new Error('A list of views have been passed but the NodeSDK expects that the MeterProvider is manually instantiated and can\'t attach Views to the MeterProvider'); + } + + this._meterProviderConfig = config; } /** Detect resource attributes */ @@ -129,7 +139,7 @@ export class NodeSDK { config?: ResourceDetectionConfig ): Promise { const internalConfig: ResourceDetectionConfig = { - detectors: [ envDetector, processDetector], + detectors: [envDetector, processDetector], ...config, }; @@ -152,7 +162,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) { @@ -170,13 +180,13 @@ export class NodeSDK { }); } - if (this._metricReader) { + if (this._meterProviderConfig?.reader) { const meterProvider = new MeterProvider({ resource: this._resource, views: this._meterProviderConfig?.views ?? [], }); - meterProvider.addMetricReader(this._metricReader); + meterProvider.addMetricReader(this._meterProviderConfig.reader); this._meterProvider = meterProvider; diff --git a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts index 46a32e6f38..5125a8fea5 100644 --- a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts +++ b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts @@ -209,6 +209,24 @@ describe('Node SDK', () => { await sdk.shutdown(); }); + it('should warn user when meter views are provided but not a MetricReader', async () => { + assert.throws( + () => { + new NodeSDK({ + views: [ + new View({ + name: 'test-view', + instrumentName: 'test_counter', + instrumentType: InstrumentType.COUNTER, + }) + ], + autoDetectResources: false, + }); + }, (error: Error) => { + return error.message === 'A list of views have been passed but the NodeSDK expects that the MeterProvider is manually instantiated and can\'t attach Views to the MeterProvider'; + }); + }); + describe('detectResources', async () => { beforeEach(() => { process.env.OTEL_RESOURCE_ATTRIBUTES = From 6d5262209198c5a5ecc28b11e6de4a113a4b2301 Mon Sep 17 00:00:00 2001 From: Weyert de Boer Date: Wed, 10 Aug 2022 01:35:53 +0100 Subject: [PATCH 10/13] fix: improve the way handling views without metric reader --- .../opentelemetry-sdk-node/src/sdk.ts | 12 ++++++---- .../opentelemetry-sdk-node/test/sdk.test.ts | 24 +++++++++++++++++-- 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/experimental/packages/opentelemetry-sdk-node/src/sdk.ts b/experimental/packages/opentelemetry-sdk-node/src/sdk.ts index a7077188a3..4511bb9365 100644 --- a/experimental/packages/opentelemetry-sdk-node/src/sdk.ts +++ b/experimental/packages/opentelemetry-sdk-node/src/sdk.ts @@ -101,7 +101,9 @@ export class NodeSDK { ); } - this.configureMeterProvider({ reader: configuration.metricReader, views: configuration.views }); + if (configuration.metricReader || configuration.views) { + this.configureMeterProvider({ reader: configuration.metricReader, views: configuration.views }); + } let instrumentations: InstrumentationOption[] = []; if (configuration.instrumentations) { @@ -127,11 +129,11 @@ export class NodeSDK { /** Set configurations needed to register a MeterProvider */ public configureMeterProvider(config: MeterProviderConfig): void { - if (config?.views && !config.reader) { - throw new Error('A list of views have been passed but the NodeSDK expects that the MeterProvider is manually instantiated and can\'t attach Views to the MeterProvider'); - } - this._meterProviderConfig = config; + + if (!this._meterProviderConfig.reader && this._meterProviderConfig.views) { + throw new Error('You have not passed a MetricReader instance but have passed Views, you need to manually pass the Views to your MeterProvider instance.'); + } } /** Detect resource attributes */ diff --git a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts index 5125a8fea5..d776b3b59d 100644 --- a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts +++ b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts @@ -223,7 +223,27 @@ describe('Node SDK', () => { autoDetectResources: false, }); }, (error: Error) => { - return error.message === 'A list of views have been passed but the NodeSDK expects that the MeterProvider is manually instantiated and can\'t attach Views to the MeterProvider'; + return error.message === 'You have not passed a MetricReader instance but have passed Views, you need to manually pass the Views to your MeterProvider instance.'; + }); + }); + + it('should warn user when meter views are provided but not a MetricReader when calling configureMeterProvider', async () => { + assert.throws( + () => { + const sdk = new NodeSDK({ + autoDetectResources: false, + }); + sdk.configureMeterProvider({ + views: [ + new View({ + name: 'test-view', + instrumentName: 'test_counter', + instrumentType: InstrumentType.COUNTER, + }) + ], + }) + }, (error: Error) => { + return error.message === 'You have not passed a MetricReader instance but have passed Views, you need to manually pass the Views to your MeterProvider instance.'; }); }); @@ -248,7 +268,7 @@ describe('Node SDK', () => { throw new Error('Buggy detector'); } }, - envDetector] + envDetector] }); const resource = sdk['_resource']; From 798aee49b299de4154808ed0f1e57be268f9ab10 Mon Sep 17 00:00:00 2001 From: Weyert de Boer Date: Mon, 15 Aug 2022 19:15:00 +0100 Subject: [PATCH 11/13] chore: update code --- .../opentelemetry-sdk-node/src/sdk.ts | 37 +++++++++++++++-- .../opentelemetry-sdk-node/test/sdk.test.ts | 40 +------------------ 2 files changed, 34 insertions(+), 43 deletions(-) diff --git a/experimental/packages/opentelemetry-sdk-node/src/sdk.ts b/experimental/packages/opentelemetry-sdk-node/src/sdk.ts index 4511bb9365..c442d69c19 100644 --- a/experimental/packages/opentelemetry-sdk-node/src/sdk.ts +++ b/experimental/packages/opentelemetry-sdk-node/src/sdk.ts @@ -102,7 +102,16 @@ export class NodeSDK { } if (configuration.metricReader || configuration.views) { - this.configureMeterProvider({ reader: configuration.metricReader, views: 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[] = []; @@ -129,10 +138,30 @@ export class NodeSDK { /** Set configurations needed to register a MeterProvider */ public configureMeterProvider(config: MeterProviderConfig): void { - this._meterProviderConfig = config; + // 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.'); + } - if (!this._meterProviderConfig.reader && this._meterProviderConfig.views) { - throw new Error('You have not passed a MetricReader instance but have passed Views, you need to manually pass the Views to your MeterProvider instance.'); + // set reader, but make sure we do not override existing reader with null/undefined. + if (config.reader != null) { + this._meterProviderConfig.reader = config.reader; } } diff --git a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts index d776b3b59d..46a32e6f38 100644 --- a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts +++ b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts @@ -209,44 +209,6 @@ describe('Node SDK', () => { await sdk.shutdown(); }); - it('should warn user when meter views are provided but not a MetricReader', async () => { - assert.throws( - () => { - new NodeSDK({ - views: [ - new View({ - name: 'test-view', - instrumentName: 'test_counter', - instrumentType: InstrumentType.COUNTER, - }) - ], - autoDetectResources: false, - }); - }, (error: Error) => { - return error.message === 'You have not passed a MetricReader instance but have passed Views, you need to manually pass the Views to your MeterProvider instance.'; - }); - }); - - it('should warn user when meter views are provided but not a MetricReader when calling configureMeterProvider', async () => { - assert.throws( - () => { - const sdk = new NodeSDK({ - autoDetectResources: false, - }); - sdk.configureMeterProvider({ - views: [ - new View({ - name: 'test-view', - instrumentName: 'test_counter', - instrumentType: InstrumentType.COUNTER, - }) - ], - }) - }, (error: Error) => { - return error.message === 'You have not passed a MetricReader instance but have passed Views, you need to manually pass the Views to your MeterProvider instance.'; - }); - }); - describe('detectResources', async () => { beforeEach(() => { process.env.OTEL_RESOURCE_ATTRIBUTES = @@ -268,7 +230,7 @@ describe('Node SDK', () => { throw new Error('Buggy detector'); } }, - envDetector] + envDetector] }); const resource = sdk['_resource']; From 57af7b24d5255614b90831cc769219e2fb0991cb Mon Sep 17 00:00:00 2001 From: Weyert de Boer Date: Mon, 15 Aug 2022 20:07:37 +0100 Subject: [PATCH 12/13] test: update the test cases --- .../opentelemetry-sdk-node/test/sdk.test.ts | 89 ++++++++++++++++--- 1 file changed, 77 insertions(+), 12 deletions(-) diff --git a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts index 46a32e6f38..747157cee1 100644 --- a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts +++ b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts @@ -147,20 +147,20 @@ describe('Node SDK', () => { }); }); - it('should register meter views when provided', async () => { - async function waitForNumberOfMetrics(exporter: InMemoryMetricExporter, numberOfMetrics: number): Promise { - 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; - } + async function waitForNumberOfMetrics(exporter: InMemoryMetricExporter, numberOfMetrics: number): Promise { + 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, @@ -209,6 +209,71 @@ describe('Node SDK', () => { 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 = From c55f5108cc92250d1b0e8f91b56c4e0e418f4ee2 Mon Sep 17 00:00:00 2001 From: Weyert de Boer Date: Mon, 15 Aug 2022 22:33:56 +0100 Subject: [PATCH 13/13] fix: update reader, if needed --- experimental/packages/opentelemetry-sdk-node/src/sdk.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/experimental/packages/opentelemetry-sdk-node/src/sdk.ts b/experimental/packages/opentelemetry-sdk-node/src/sdk.ts index c442d69c19..706717a03a 100644 --- a/experimental/packages/opentelemetry-sdk-node/src/sdk.ts +++ b/experimental/packages/opentelemetry-sdk-node/src/sdk.ts @@ -211,13 +211,15 @@ export class NodeSDK { }); } - if (this._meterProviderConfig?.reader) { + if (this._meterProviderConfig) { const meterProvider = new MeterProvider({ resource: this._resource, views: this._meterProviderConfig?.views ?? [], }); - meterProvider.addMetricReader(this._meterProviderConfig.reader); + if (this._meterProviderConfig.reader) { + meterProvider.addMetricReader(this._meterProviderConfig.reader); + } this._meterProvider = meterProvider;