From 3a87a54ae45afa0c3fde5ffeb47fe7edfe96acc4 Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Fri, 6 Dec 2024 18:20:40 +0100 Subject: [PATCH 1/3] feat(sdk-trace): remove ability to statically register exporters to BasicTracerProvider class --- .../opentelemetry-sdk-node/test/sdk.test.ts | 5 +- .../src/BasicTracerProvider.ts | 32 ---- .../test/common/BasicTracerProvider.test.ts | 177 +----------------- .../test/NodeTracerProvider.test.ts | 57 +----- .../src/WebTracerProvider.ts | 11 ++ 5 files changed, 29 insertions(+), 253 deletions(-) diff --git a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts index c5cf4472317..621478e18b5 100644 --- a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts +++ b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts @@ -1236,7 +1236,7 @@ describe('setup exporter from env', () => { await sdk.shutdown(); }); - it('should use noop span processor when user sets env exporter to none', async () => { + it('should use empty span processor when user sets env exporter to none', async () => { env.OTEL_TRACES_EXPORTER = 'none'; const sdk = new NodeSDK(); sdk.start(); @@ -1249,8 +1249,7 @@ describe('setup exporter from env', () => { const listOfProcessors = getSdkSpanProcessors(sdk); - assert(listOfProcessors.length === 1); - assert(listOfProcessors[0] instanceof NoopSpanProcessor); + assert.strictEqual(listOfProcessors.length, 0); delete env.OTEL_TRACES_EXPORTER; await sdk.shutdown(); }); diff --git a/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts b/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts index b99d6a664f1..545e0e8cdb7 100644 --- a/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts +++ b/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts @@ -33,10 +33,8 @@ import { IResource, Resource } from '@opentelemetry/resources'; import { SpanProcessor, Tracer } from '.'; import { loadDefaultConfig } from './config'; import { MultiSpanProcessor } from './MultiSpanProcessor'; -import { NoopSpanProcessor } from './export/NoopSpanProcessor'; import { SDKRegistrationConfig, TracerConfig } from './types'; import { SpanExporter } from './export/SpanExporter'; -import { BatchSpanProcessor } from './platform'; import { reconfigureLimits } from './utility'; export type PROPAGATOR_FACTORY = () => TextMapPropagator; @@ -61,11 +59,6 @@ export class BasicTracerProvider implements TracerProvider { ['baggage', () => new W3CBaggagePropagator()], ]); - protected static readonly _registeredExporters = new Map< - string, - EXPORTER_FACTORY - >(); - private readonly _config: TracerConfig; private readonly _tracers: Map = new Map(); private readonly _resource: IResource; @@ -91,13 +84,6 @@ export class BasicTracerProvider implements TracerProvider { if (config.spanProcessors?.length) { spanProcessors.push(...config.spanProcessors); - } else { - const defaultExporter = this._buildExporterFromEnv(); - spanProcessors.push( - defaultExporter - ? new BatchSpanProcessor(defaultExporter) - : new NoopSpanProcessor() - ); } this._activeSpanProcessor = new MultiSpanProcessor(spanProcessors); @@ -213,12 +199,6 @@ export class BasicTracerProvider implements TracerProvider { )._registeredPropagators.get(name)?.(); } - protected _getSpanExporter(name: string): SpanExporter | undefined { - return ( - this.constructor as typeof BasicTracerProvider - )._registeredExporters.get(name)?.(); - } - protected _buildPropagatorFromEnv(): TextMapPropagator | undefined { // per spec, propagators from env must be deduplicated const uniquePropagatorNames = Array.from( @@ -255,16 +235,4 @@ export class BasicTracerProvider implements TracerProvider { }); } } - - protected _buildExporterFromEnv(): SpanExporter | undefined { - const exporterName = getEnv().OTEL_TRACES_EXPORTER; - if (exporterName === 'none' || exporterName === '') return; - const exporter = this._getSpanExporter(exporterName); - if (!exporter) { - diag.error( - `Exporter "${exporterName}" requested through environment variable is unavailable.` - ); - } - return exporter; - } } diff --git a/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts b/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts index 62395128f9f..625f79d4090 100644 --- a/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts +++ b/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts @@ -36,9 +36,6 @@ import { BasicTracerProvider, NoopSpanProcessor, Span, - InMemorySpanExporter, - SpanExporter, - BatchSpanProcessor, AlwaysOnSampler, AlwaysOffSampler, ConsoleSpanExporter, @@ -59,8 +56,6 @@ class DummyPropagator implements TextMapPropagator { } } -class DummyExporter extends InMemorySpanExporter {} - describe('BasicTracerProvider', () => { let envSource: Record; let setGlobalPropagatorStub: sinon.SinonSpy<[TextMapPropagator], boolean>; @@ -90,55 +85,19 @@ describe('BasicTracerProvider', () => { assert.ok(tracer instanceof BasicTracerProvider); }); - it('should use noop span processor by default', () => { - const tracer = new BasicTracerProvider(); - assert.ok(tracer['_activeSpanProcessor'] instanceof MultiSpanProcessor); - assert.ok( - tracer['_activeSpanProcessor']['_spanProcessors'].length === 1 - ); - assert.ok( - tracer['_activeSpanProcessor']['_spanProcessors'][0] instanceof - NoopSpanProcessor - ); - }); - it('should use noop span processor by default and no diag error', () => { + it('should use empty span processor by default', () => { const errorStub = sinon.spy(diag, 'error'); const tracer = new BasicTracerProvider(); assert.ok(tracer['_activeSpanProcessor'] instanceof MultiSpanProcessor); - assert.ok( - tracer['_activeSpanProcessor']['_spanProcessors'].length === 1 - ); - assert.ok( - tracer['_activeSpanProcessor']['_spanProcessors'][0] instanceof - NoopSpanProcessor + assert.strictEqual( + tracer['_activeSpanProcessor']['_spanProcessors'].length, + 0 ); sinon.assert.notCalled(errorStub); }); }); - describe('when user sets unavailable exporter', () => { - it('should use noop span processor by default and show diag error', () => { - envSource.OTEL_TRACES_EXPORTER = 'someExporter'; - const errorStub = sinon.spy(diag, 'error'); - const tracer = new BasicTracerProvider(); - - assert.ok(tracer['_activeSpanProcessor'] instanceof MultiSpanProcessor); - assert.ok( - tracer['_activeSpanProcessor']['_spanProcessors'].length === 1 - ); - assert.ok( - tracer['_activeSpanProcessor']['_spanProcessors'][0] instanceof - NoopSpanProcessor - ); - sinon.assert.calledWith( - errorStub, - 'Exporter "someExporter" requested through environment variable is unavailable.' - ); - delete envSource.OTEL_TRACES_EXPORTER; - }); - }); - describe('when user sets span processors', () => { it('should use the span processors defined in the config', () => { const traceExporter = new ConsoleSpanExporter(); @@ -448,14 +407,6 @@ describe('BasicTracerProvider', () => { ...BasicTracerProvider._registeredPropagators, ['custom-propagator', () => new DummyPropagator()], ]); - - protected static override readonly _registeredExporters = new Map< - string, - () => SpanExporter - >([ - ...BasicTracerProvider._registeredExporters, - ['custom-exporter', () => new DummyExporter()], - ]); } const provider = new CustomTracerProvider({}); @@ -465,19 +416,9 @@ describe('BasicTracerProvider', () => { ); /* BasicTracerProvider has no exporters by default, so skipping testing the exporter getter */ provider.register(); - - assert.ok(provider['_activeSpanProcessor'] instanceof MultiSpanProcessor); - assert.ok( - provider['_activeSpanProcessor']['_spanProcessors'].length === 1 - ); - assert.ok( - provider['_activeSpanProcessor']['_spanProcessors'][0] instanceof - BatchSpanProcessor - ); - assert.ok( - provider['_activeSpanProcessor']['_spanProcessors'][0][ - '_exporter' - ] instanceof DummyExporter + assert.strictEqual( + provider['_activeSpanProcessor']['_spanProcessors'].length, + 0 ); sinon.assert.calledOnceWithExactly( @@ -494,11 +435,6 @@ describe('BasicTracerProvider', () => { () => TextMapPropagator >([['custom-propagator', () => new DummyPropagator()]]); - protected static override readonly _registeredExporters = new Map< - string, - () => SpanExporter - >([['custom-exporter', () => new DummyExporter()]]); - protected override _getPropagator( name: string ): TextMapPropagator | undefined { @@ -507,32 +443,13 @@ describe('BasicTracerProvider', () => { CustomTracerProvider._registeredPropagators.get(name)?.() ); } - - protected override _getSpanExporter( - name: string - ): SpanExporter | undefined { - return ( - super._getSpanExporter(name) || - CustomTracerProvider._registeredExporters.get(name)?.() - ); - } } const provider = new CustomTracerProvider({}); provider.register(); - - assert.ok(provider['_activeSpanProcessor'] instanceof MultiSpanProcessor); - assert.ok( - provider['_activeSpanProcessor']['_spanProcessors'].length === 1 - ); - assert.ok( - provider['_activeSpanProcessor']['_spanProcessors'][0] instanceof - BatchSpanProcessor - ); - assert.ok( - provider['_activeSpanProcessor']['_spanProcessors'][0][ - '_exporter' - ] instanceof DummyExporter + assert.strictEqual( + provider['_activeSpanProcessor']['_spanProcessors'].length, + 0 ); sinon.assert.calledOnceWithExactly( @@ -600,57 +517,6 @@ describe('BasicTracerProvider', () => { ); }); }); - - describe('exporter', () => { - class CustomTracerProvider extends BasicTracerProvider { - protected override _getSpanExporter( - name: string - ): SpanExporter | undefined { - return name === 'memory' - ? new InMemorySpanExporter() - : BasicTracerProvider._registeredExporters.get(name)?.(); - } - } - - afterEach(() => { - delete envSource.OTEL_TRACES_EXPORTER; - }); - - it('logs error if there is no exporter registered with a given name', () => { - const errorStub = sinon.spy(diag, 'error'); - - envSource.OTEL_TRACES_EXPORTER = 'missing-exporter'; - const provider = new BasicTracerProvider({}); - provider.register(); - assert.ok( - errorStub.getCall(0).args[0] === - 'Exporter "missing-exporter" requested through environment variable is unavailable.' - ); - errorStub.restore(); - }); - - it('registers trace exporter from environment variable', () => { - envSource.OTEL_TRACES_EXPORTER = 'memory'; - const provider = new CustomTracerProvider({}); - provider.register(); - - assert.ok( - provider['_activeSpanProcessor'] instanceof MultiSpanProcessor - ); - assert.ok( - provider['_activeSpanProcessor']['_spanProcessors'].length === 1 - ); - assert.ok( - provider['_activeSpanProcessor']['_spanProcessors'][0] instanceof - BatchSpanProcessor - ); - assert.ok( - provider['_activeSpanProcessor']['_spanProcessors'][0][ - '_exporter' - ] instanceof InMemorySpanExporter - ); - }); - }); }); describe('.startSpan()', () => { @@ -834,29 +700,6 @@ describe('BasicTracerProvider', () => { }); describe('.forceFlush()', () => { - it('should call forceFlush with the default processor', done => { - sinon.restore(); - const forceFlushStub = sinon.stub( - NoopSpanProcessor.prototype, - 'forceFlush' - ); - forceFlushStub.resolves(); - - const tracerProvider = new BasicTracerProvider(); - - tracerProvider - .forceFlush() - .then(() => { - sinon.restore(); - assert(forceFlushStub.calledOnce); - done(); - }) - .catch(error => { - sinon.restore(); - done(error); - }); - }); - it('should call forceFlush on all registered span processors', done => { sinon.restore(); const forceFlushStub = sinon.stub( diff --git a/packages/opentelemetry-sdk-trace-node/test/NodeTracerProvider.test.ts b/packages/opentelemetry-sdk-trace-node/test/NodeTracerProvider.test.ts index f9017f15343..e076ad0c71d 100644 --- a/packages/opentelemetry-sdk-trace-node/test/NodeTracerProvider.test.ts +++ b/packages/opentelemetry-sdk-trace-node/test/NodeTracerProvider.test.ts @@ -33,10 +33,7 @@ import { AsyncHooksContextManager } from '@opentelemetry/context-async-hooks'; import { AlwaysOffSampler, AlwaysOnSampler, - BatchSpanProcessor, - InMemorySpanExporter, Span, - SpanExporter, } from '@opentelemetry/sdk-trace-base'; import { Resource } from '@opentelemetry/resources'; import { SEMRESATTRS_TELEMETRY_SDK_LANGUAGE } from '@opentelemetry/semantic-conventions'; @@ -265,10 +262,7 @@ describe('NodeTracerProvider', () => { } } - class DummyExporter extends InMemorySpanExporter {} - beforeEach(() => { - process.env.OTEL_TRACES_EXPORTER = 'custom-exporter'; process.env.OTEL_PROPAGATORS = 'custom-propagator'; propagation.disable(); @@ -276,7 +270,6 @@ describe('NodeTracerProvider', () => { }); afterEach(() => { - delete process.env.OTEL_TRACES_EXPORTER; delete process.env.OTEL_PROPAGATORS; propagation.disable(); @@ -293,33 +286,18 @@ describe('NodeTracerProvider', () => { string, () => TextMapPropagator >([['custom-propagator', () => propagator]]); - - protected static override readonly _registeredExporters = new Map< - string, - () => SpanExporter - >([['custom-exporter', () => new DummyExporter()]]); } const provider = new CustomTracerProvider({}); provider.register(); - assert.ok( provider['_activeSpanProcessor'].constructor.name === 'MultiSpanProcessor' ); - assert.ok( - provider['_activeSpanProcessor']['_spanProcessors'].length === 1 - ); - assert.ok( - provider['_activeSpanProcessor']['_spanProcessors'][0] instanceof - BatchSpanProcessor - ); - assert.ok( - provider['_activeSpanProcessor']['_spanProcessors'][0][ - '_exporter' - ] instanceof DummyExporter + assert.strictEqual( + provider['_activeSpanProcessor']['_spanProcessors'].length, + 0 ); - assert.strictEqual(propagation['_getGlobalPropagator'](), propagator); }); @@ -333,11 +311,6 @@ describe('NodeTracerProvider', () => { () => TextMapPropagator >([['custom-propagator', () => propagator]]); - protected static override readonly _registeredExporters = new Map< - string, - () => SpanExporter - >([['custom-exporter', () => new DummyExporter()]]); - protected override _getPropagator( name: string ): TextMapPropagator | undefined { @@ -346,35 +319,17 @@ describe('NodeTracerProvider', () => { CustomTracerProvider._registeredPropagators.get(name)?.() ); } - - protected override _getSpanExporter( - name: string - ): SpanExporter | undefined { - return ( - super._getSpanExporter(name) || - CustomTracerProvider._registeredExporters.get(name)?.() - ); - } } const provider = new CustomTracerProvider({}); provider.register(); - assert.ok( provider['_activeSpanProcessor'].constructor.name === 'MultiSpanProcessor' ); - assert.ok( - provider['_activeSpanProcessor']['_spanProcessors'].length === 1 - ); - assert.ok( - provider['_activeSpanProcessor']['_spanProcessors'][0] instanceof - BatchSpanProcessor - ); - assert.ok( - provider['_activeSpanProcessor']['_spanProcessors'][0][ - '_exporter' - ] instanceof DummyExporter + assert.strictEqual( + provider['_activeSpanProcessor']['_spanProcessors'].length, + 0 ); assert.strictEqual(propagation['_getGlobalPropagator'](), propagator); diff --git a/packages/opentelemetry-sdk-trace-web/src/WebTracerProvider.ts b/packages/opentelemetry-sdk-trace-web/src/WebTracerProvider.ts index 0317f48f863..d8a405273f8 100644 --- a/packages/opentelemetry-sdk-trace-web/src/WebTracerProvider.ts +++ b/packages/opentelemetry-sdk-trace-web/src/WebTracerProvider.ts @@ -26,6 +26,17 @@ import { StackContextManager } from './StackContextManager'; */ export type WebTracerConfig = TracerConfig; +/*function registerWebTracerProvider(tracerProvider: TracerProvider, config?: SDKRegistrationConfig = {}){ + if (config.contextManager === undefined) { + config.contextManager = new StackContextManager(); + } + if (config.contextManager) { + config.contextManager.enable(); + } + + super.register(config); +}*/ + /** * This class represents a web tracer with {@link StackContextManager} */ From 2eae41a02b8635d77b6cbb1213e6f8645e4d1138 Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Fri, 6 Dec 2024 18:46:37 +0100 Subject: [PATCH 2/3] fixup! feat(sdk-trace): remove ability to statically register exporters to BasicTracerProvider class --- CHANGELOG_NEXT.md | 6 ++++++ .../src/WebTracerProvider.ts | 11 ----------- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/CHANGELOG_NEXT.md b/CHANGELOG_NEXT.md index 06717c2ebe2..bef1bb29964 100644 --- a/CHANGELOG_NEXT.md +++ b/CHANGELOG_NEXT.md @@ -16,6 +16,12 @@ * refactor(sdk-trace-base)!: remove `new Span` constructor in favor of `Tracer.startSpan` API [#5048](https://github.com/open-telemetry/opentelemetry-js/pull/5048) @david-luna * refactor(sdk-trace-base)!: remove `BasicTracerProvider.addSpanProcessor` API in favor of constructor options. [#5134](https://github.com/open-telemetry/opentelemetry-js/pull/5134) @david-luna * refactor(sdk-trace-base)!: make `resource` property private in `BasicTracerProvider` and remove `getActiveSpanProcessor` API. [#5192](https://github.com/open-telemetry/opentelemetry-js/pull/5192) @david-luna +* feat(sdk-trace)!: remove ability to have BasicTracerProvider instantiate exporters [#5239](https://github.com/open-telemetry/opentelemetry-js/pull/5239) @pichlermarc + * When extending `BasicTracerProvider`, the class offered multiple methods to facilitate the creation of exporters and auto-pairing with `SpanProcessor`s. + * This functionality has been removed - users may now pass `SpanProcessor`s to the base class constructor when extending + * (user-facing): `_registeredExporters` has been removed + * (user-facing): `_getSpanExporter` has been removed + * (user-facing): `_buildExporterFromEnv` has been removed ### :rocket: (Enhancement) diff --git a/packages/opentelemetry-sdk-trace-web/src/WebTracerProvider.ts b/packages/opentelemetry-sdk-trace-web/src/WebTracerProvider.ts index d8a405273f8..0317f48f863 100644 --- a/packages/opentelemetry-sdk-trace-web/src/WebTracerProvider.ts +++ b/packages/opentelemetry-sdk-trace-web/src/WebTracerProvider.ts @@ -26,17 +26,6 @@ import { StackContextManager } from './StackContextManager'; */ export type WebTracerConfig = TracerConfig; -/*function registerWebTracerProvider(tracerProvider: TracerProvider, config?: SDKRegistrationConfig = {}){ - if (config.contextManager === undefined) { - config.contextManager = new StackContextManager(); - } - if (config.contextManager) { - config.contextManager.enable(); - } - - super.register(config); -}*/ - /** * This class represents a web tracer with {@link StackContextManager} */ From 3e9a07adfa2c1a2e9d0589b03e3f2190f5497b45 Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Fri, 20 Dec 2024 12:31:55 +0100 Subject: [PATCH 3/3] chore: add changelog entry --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d2af8ed127..c94ac0dba74 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,12 @@ For semantic convention package changes, see the [semconv CHANGELOG](packages/se * refactor(sdk-trace-base)!: remove `new Span` constructor in favor of `Tracer.startSpan` API [#5048](https://github.com/open-telemetry/opentelemetry-js/pull/5048) @david-luna * refactor(sdk-trace-base)!: remove `BasicTracerProvider.addSpanProcessor` API in favor of constructor options. [#5134](https://github.com/open-telemetry/opentelemetry-js/pull/5134) @david-luna * refactor(sdk-trace-base)!: make `resource` property private in `BasicTracerProvider` and remove `getActiveSpanProcessor` API. [#5192](https://github.com/open-telemetry/opentelemetry-js/pull/5192) @david-luna +* feat(sdk-trace)!: remove ability to have BasicTracerProvider instantiate exporters [#5239](https://github.com/open-telemetry/opentelemetry-js/pull/5239) @pichlermarc + * When extending `BasicTracerProvider`, the class offered multiple methods to facilitate the creation of exporters and auto-pairing with `SpanProcessor`s. + * This functionality has been removed - users may now pass `SpanProcessor`s to the base class constructor when extending + * (user-facing): `_registeredExporters` has been removed + * (user-facing): `_getSpanExporter` has been removed + * (user-facing): `_buildExporterFromEnv` has been removed ### :rocket: (Enhancement)