diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d2af8ed12..c94ac0dba7 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) diff --git a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts index 87c7e0f849..5a4e2fc137 100644 --- a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts +++ b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts @@ -1470,7 +1470,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(); @@ -1483,8 +1483,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 042e98beba..3e444a6e37 100644 --- a/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts +++ b/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts @@ -34,10 +34,8 @@ import { SpanProcessor } from './SpanProcessor'; import { Tracer } from './Tracer'; 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; @@ -62,11 +60,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; @@ -92,13 +85,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); @@ -214,12 +200,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( @@ -256,16 +236,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 62395128f9..625f79d409 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 f9017f1534..e076ad0c71 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);