From f3a6310733479f4aa8d1b2aff5841a16ae894714 Mon Sep 17 00:00:00 2001 From: David Luna Date: Thu, 21 Nov 2024 14:10:10 +0100 Subject: [PATCH] refactor(sdk-trace-base): remove `_registeredSpanProcessors` from BasicTracerProvider (#5177) --- CHANGELOG_NEXT.md | 1 + .../opentelemetry-sdk-node/test/sdk.test.ts | 96 ++++++------- .../src/BasicTracerProvider.ts | 25 ++-- .../opentelemetry-sdk-trace-base/src/Span.ts | 6 +- .../test/common/BasicTracerProvider.test.ts | 128 ++++++++++++++---- .../test/NodeTracerProvider.test.ts | 44 ++++-- 6 files changed, 202 insertions(+), 98 deletions(-) diff --git a/CHANGELOG_NEXT.md b/CHANGELOG_NEXT.md index 064956eab0..9729d712fe 100644 --- a/CHANGELOG_NEXT.md +++ b/CHANGELOG_NEXT.md @@ -23,5 +23,6 @@ ### :house: (Internal) * chore: remove checks for unsupported node versions [#4341](https://github.com/open-telemetry/opentelemetry-js/pull/4341) @dyladan +* refactor(sdk-trace-base): remove `BasicTracerProvider._registeredSpanProcessors` private property. [#5134](https://github.com/open-telemetry/opentelemetry-js/pull/5177) @david-luna ### :bug: (Bug Fix) diff --git a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts index efda2a509f..ca0d26870f 100644 --- a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts +++ b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts @@ -50,6 +50,7 @@ import { NoopSpanProcessor, IdGenerator, AlwaysOffSampler, + SpanProcessor, } from '@opentelemetry/sdk-trace-base'; import * as assert from 'assert'; import * as semver from 'semver'; @@ -257,13 +258,25 @@ describe('Node SDK', () => { ); const apiTracerProvider = trace.getTracerProvider() as ProxyTracerProvider; - assert.ok(apiTracerProvider.getDelegate() instanceof NodeTracerProvider); + const nodeTracerProvider = apiTracerProvider.getDelegate(); + + assert.ok(nodeTracerProvider instanceof NodeTracerProvider); + + const spanProcessor = nodeTracerProvider.getActiveSpanProcessor() as any; + + assert( + spanProcessor.constructor.name === 'MultiSpanProcessor', + 'is MultiSpanProcessor' + ); - const listOfProcessors = - sdk['_tracerProvider']!['_registeredSpanProcessors']!; + const listOfProcessors = spanProcessor[ + '_spanProcessors' + ] as SpanProcessor[]; - assert(sdk['_tracerProvider'] instanceof NodeTracerProvider); - assert(listOfProcessors.length === 3); + assert( + listOfProcessors.length === 3, + 'it has the right amount of processors' + ); assert(listOfProcessors[0] instanceof NoopSpanProcessor); assert(listOfProcessors[1] instanceof SimpleSpanProcessor); assert(listOfProcessors[2] instanceof BatchSpanProcessor); @@ -1099,6 +1112,18 @@ describe('Node SDK', () => { describe('setup exporter from env', () => { let stubLoggerError: Sinon.SinonStub; + const getSdkSpanProcessors = (sdk: NodeSDK) => { + const tracerProvider = sdk['_tracerProvider']; + + assert(tracerProvider instanceof NodeTracerProvider); + + const activeSpanProcessor = tracerProvider.getActiveSpanProcessor(); + + assert(activeSpanProcessor.constructor.name === 'MultiSpanProcessor'); + + return (activeSpanProcessor as any)['_spanProcessors'] as SpanProcessor[]; + }; + beforeEach(() => { stubLoggerError = Sinon.stub(diag, 'warn'); }); @@ -1109,8 +1134,7 @@ describe('setup exporter from env', () => { it('should use default exporter when nor env neither SDK config is given', async () => { const sdk = new NodeSDK(); sdk.start(); - const listOfProcessors = - sdk['_tracerProvider']!['_registeredSpanProcessors']!; + const listOfProcessors = getSdkSpanProcessors(sdk); assert(listOfProcessors.length === 1); assert(listOfProcessors[0] instanceof BatchSpanProcessor); @@ -1124,8 +1148,7 @@ describe('setup exporter from env', () => { traceExporter, }); sdk.start(); - const listOfProcessors = - sdk['_tracerProvider']!['_registeredSpanProcessors']!; + const listOfProcessors = getSdkSpanProcessors(sdk); assert(listOfProcessors.length === 1); assert(listOfProcessors[0] instanceof BatchSpanProcessor); @@ -1140,8 +1163,7 @@ describe('setup exporter from env', () => { spanProcessor, }); sdk.start(); - const listOfProcessors = - sdk['_tracerProvider']!['_registeredSpanProcessors']!; + const listOfProcessors = getSdkSpanProcessors(sdk); assert(listOfProcessors.length === 1); assert(listOfProcessors[0] instanceof SimpleSpanProcessor); @@ -1156,8 +1178,7 @@ describe('setup exporter from env', () => { traceExporter, }); sdk.start(); - const listOfProcessors = - sdk['_tracerProvider']!['_registeredSpanProcessors']!; + const listOfProcessors = getSdkSpanProcessors(sdk); assert(listOfProcessors.length === 1); assert(listOfProcessors[0] instanceof BatchSpanProcessor); @@ -1172,8 +1193,7 @@ describe('setup exporter from env', () => { sampler: new AlwaysOffSampler(), }); sdk.start(); - const listOfProcessors = - sdk['_tracerProvider']!['_registeredSpanProcessors']!; + const listOfProcessors = getSdkSpanProcessors(sdk); assert.ok( sdk['_tracerProvider']!['_config']?.sampler instanceof AlwaysOffSampler @@ -1190,9 +1210,7 @@ describe('setup exporter from env', () => { env.OTEL_EXPORTER_OTLP_TRACES_PROTOCOL = 'grpc'; const sdk = new NodeSDK(); sdk.start(); - - const listOfProcessors = - sdk['_tracerProvider']!['_registeredSpanProcessors']!; + const listOfProcessors = getSdkSpanProcessors(sdk); assert(listOfProcessors.length === 1); assert(listOfProcessors[0] instanceof BatchSpanProcessor); @@ -1208,9 +1226,7 @@ describe('setup exporter from env', () => { env.OTEL_EXPORTER_OTLP_TRACES_PROTOCOL = 'grpc'; const sdk = new NodeSDK(); sdk.start(); - - const listOfProcessors = - sdk['_tracerProvider']!['_registeredSpanProcessors']!; + const listOfProcessors = getSdkSpanProcessors(sdk); assert(listOfProcessors.length === 1); assert(listOfProcessors[0] instanceof BatchSpanProcessor); @@ -1231,12 +1247,10 @@ describe('setup exporter from env', () => { 'OTEL_TRACES_EXPORTER contains "none". SDK will not be initialized.' ); - const listOfProcessors = - sdk['_tracerProvider']!['_registeredSpanProcessors']!; - const activeProcessor = sdk['_tracerProvider']?.getActiveSpanProcessor(); + const listOfProcessors = getSdkSpanProcessors(sdk); - assert(listOfProcessors.length === 0); - assert(activeProcessor instanceof NoopSpanProcessor); + assert(listOfProcessors.length === 1); + assert(listOfProcessors[0] instanceof NoopSpanProcessor); delete env.OTEL_TRACES_EXPORTER; await sdk.shutdown(); }); @@ -1246,8 +1260,7 @@ describe('setup exporter from env', () => { const sdk = new NodeSDK(); sdk.start(); - const listOfProcessors = - sdk['_tracerProvider']!['_registeredSpanProcessors']!; + const listOfProcessors = getSdkSpanProcessors(sdk); assert(listOfProcessors.length === 1); assert(listOfProcessors[0] instanceof BatchSpanProcessor); @@ -1267,8 +1280,7 @@ describe('setup exporter from env', () => { 'OTEL_TRACES_EXPORTER contains "none" along with other exporters. Using default otlp exporter.' ); - const listOfProcessors = - sdk['_tracerProvider']!['_registeredSpanProcessors']!; + const listOfProcessors = getSdkSpanProcessors(sdk); assert(listOfProcessors.length === 1); assert(listOfProcessors[0] instanceof BatchSpanProcessor); @@ -1303,8 +1315,7 @@ describe('setup exporter from env', () => { const sdk = new NodeSDK(); sdk.start(); - const listOfProcessors = - sdk['_tracerProvider']!['_registeredSpanProcessors']!; + const listOfProcessors = getSdkSpanProcessors(sdk); assert(listOfProcessors.length === 1); assert(listOfProcessors[0] instanceof BatchSpanProcessor); @@ -1321,8 +1332,7 @@ describe('setup exporter from env', () => { const sdk = new NodeSDK(); sdk.start(); - const listOfProcessors = - sdk['_tracerProvider']!['_registeredSpanProcessors']!; + const listOfProcessors = getSdkSpanProcessors(sdk); assert(listOfProcessors.length === 2); assert(listOfProcessors[0] instanceof BatchSpanProcessor); @@ -1341,8 +1351,7 @@ describe('setup exporter from env', () => { const sdk = new NodeSDK(); sdk.start(); - const listOfProcessors = - sdk['_tracerProvider']!['_registeredSpanProcessors']!; + const listOfProcessors = getSdkSpanProcessors(sdk); assert(listOfProcessors.length === 1); assert(listOfProcessors[0] instanceof BatchSpanProcessor); @@ -1359,8 +1368,7 @@ describe('setup exporter from env', () => { const sdk = new NodeSDK(); sdk.start(); - const listOfProcessors = - sdk['_tracerProvider']!['_registeredSpanProcessors']!; + const listOfProcessors = getSdkSpanProcessors(sdk); assert(listOfProcessors.length === 2); assert(listOfProcessors[0] instanceof BatchSpanProcessor); @@ -1379,8 +1387,7 @@ describe('setup exporter from env', () => { const sdk = new NodeSDK(); sdk.start(); - const listOfProcessors = - sdk['_tracerProvider']!['_registeredSpanProcessors']!; + const listOfProcessors = getSdkSpanProcessors(sdk); assert(listOfProcessors.length === 3); assert(listOfProcessors[0] instanceof BatchSpanProcessor); @@ -1400,8 +1407,7 @@ describe('setup exporter from env', () => { const sdk = new NodeSDK(); sdk.start(); - const listOfProcessors = - sdk['_tracerProvider']!['_registeredSpanProcessors']!; + const listOfProcessors = getSdkSpanProcessors(sdk); assert(listOfProcessors.length === 2); assert(listOfProcessors[0] instanceof SimpleSpanProcessor); @@ -1417,8 +1423,7 @@ describe('setup exporter from env', () => { const sdk = new NodeSDK(); sdk.start(); - const listOfProcessors = - sdk['_tracerProvider']!['_registeredSpanProcessors']!; + const listOfProcessors = getSdkSpanProcessors(sdk); assert(listOfProcessors.length === 1); assert(listOfProcessors[0] instanceof SimpleSpanProcessor); @@ -1433,8 +1438,7 @@ describe('setup exporter from env', () => { const sdk = new NodeSDK(); sdk.start(); - const listOfProcessors = - sdk['_tracerProvider']!['_registeredSpanProcessors']!; + const listOfProcessors = getSdkSpanProcessors(sdk); assert(listOfProcessors.length === 1); assert(listOfProcessors[0] instanceof SimpleSpanProcessor); diff --git a/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts b/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts index d09a8e634b..42f1775855 100644 --- a/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts +++ b/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts @@ -67,10 +67,9 @@ export class BasicTracerProvider implements TracerProvider { >(); private readonly _config: TracerConfig; - private readonly _registeredSpanProcessors: SpanProcessor[] = []; private readonly _tracers: Map = new Map(); - private activeSpanProcessor: SpanProcessor; + private activeSpanProcessor: MultiSpanProcessor; readonly resource: IResource; constructor(config: TracerConfig = {}) { @@ -89,20 +88,20 @@ export class BasicTracerProvider implements TracerProvider { resource: this.resource, }); + const spanProcessors: SpanProcessor[] = []; + if (config.spanProcessors?.length) { - this._registeredSpanProcessors = [...config.spanProcessors]; - this.activeSpanProcessor = new MultiSpanProcessor( - this._registeredSpanProcessors - ); + spanProcessors.push(...config.spanProcessors); } else { const defaultExporter = this._buildExporterFromEnv(); - if (defaultExporter !== undefined) { - const batchProcessor = new BatchSpanProcessor(defaultExporter); - this.activeSpanProcessor = batchProcessor; - } else { - this.activeSpanProcessor = new NoopSpanProcessor(); - } + spanProcessors.push( + defaultExporter + ? new BatchSpanProcessor(defaultExporter) + : new NoopSpanProcessor() + ); } + + this.activeSpanProcessor = new MultiSpanProcessor(spanProcessors); } getTracer( @@ -154,7 +153,7 @@ export class BasicTracerProvider implements TracerProvider { forceFlush(): Promise { const timeout = this._config.forceFlushTimeoutMillis; - const promises = this._registeredSpanProcessors.map( + const promises = this.activeSpanProcessor['_spanProcessors'].map( (spanProcessor: SpanProcessor) => { return new Promise(resolve => { let state: ForceFlushState; diff --git a/packages/opentelemetry-sdk-trace-base/src/Span.ts b/packages/opentelemetry-sdk-trace-base/src/Span.ts index cfd23d46db..e8c1480c89 100644 --- a/packages/opentelemetry-sdk-trace-base/src/Span.ts +++ b/packages/opentelemetry-sdk-trace-base/src/Span.ts @@ -97,10 +97,8 @@ export class SpanImpl implements Span { private readonly _startTimeProvided: boolean; /** - * Constructs a new Span instance. - * - * @deprecated calling Span constructor directly is not supported. Please use tracer.startSpan. - * */ + * Constructs a new SpanImpl instance. + */ constructor( parentTracer: Tracer, context: Context, 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 8044669dbd..4cd158adcf 100644 --- a/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts +++ b/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts @@ -45,6 +45,7 @@ import { SimpleSpanProcessor, } from '../../src'; import { SpanImpl } from '../../src/Span'; +import { MultiSpanProcessor } from '../../src/MultiSpanProcessor'; class DummyPropagator implements TextMapPropagator { inject(context: Context, carrier: any, setter: TextMapSetter): void { @@ -91,25 +92,45 @@ describe('BasicTracerProvider', () => { it('should use noop span processor by default', () => { const tracer = new BasicTracerProvider(); - assert.ok(tracer['activeSpanProcessor'] instanceof NoopSpanProcessor); + 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', () => { const errorStub = sinon.spy(diag, 'error'); const tracer = new BasicTracerProvider(); - assert.ok(tracer['activeSpanProcessor'] instanceof NoopSpanProcessor); + assert.ok(tracer['activeSpanProcessor'] instanceof MultiSpanProcessor); + assert.ok( + tracer['activeSpanProcessor']['_spanProcessors'].length === 1 + ); + assert.ok( + tracer['activeSpanProcessor']['_spanProcessors'][0] instanceof + NoopSpanProcessor + ); sinon.assert.notCalled(errorStub); }); }); describe('when user sets unavailable exporter', () => { it('should use noop span processor by default and show diag error', () => { - const errorStub = sinon.spy(diag, 'error'); envSource.OTEL_TRACES_EXPORTER = 'someExporter'; - + const errorStub = sinon.spy(diag, 'error'); const tracer = new BasicTracerProvider(); - assert.ok(tracer['activeSpanProcessor'] instanceof NoopSpanProcessor); + 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.' @@ -122,16 +143,22 @@ describe('BasicTracerProvider', () => { it('should use the span processors defined in the config', () => { const traceExporter = new ConsoleSpanExporter(); const spanProcessor = new SimpleSpanProcessor(traceExporter); - const tracer = new BasicTracerProvider({ spanProcessors: [spanProcessor], }); + + assert.ok(tracer['activeSpanProcessor'] instanceof MultiSpanProcessor); + assert.ok( + tracer['activeSpanProcessor']['_spanProcessors'].length === 1 + ); assert.ok( - tracer['_registeredSpanProcessors'][0] instanceof SimpleSpanProcessor + tracer['activeSpanProcessor']['_spanProcessors'][0] instanceof + SimpleSpanProcessor ); assert.ok( - tracer['_registeredSpanProcessors'][0]['_exporter'] instanceof - ConsoleSpanExporter + tracer['activeSpanProcessor']['_spanProcessors'][0][ + '_exporter' + ] instanceof ConsoleSpanExporter ); }); }); @@ -437,13 +464,21 @@ describe('BasicTracerProvider', () => { W3CTraceContextPropagator ); /* BasicTracerProvider has no exporters by default, so skipping testing the exporter getter */ - provider.register(); - const processor = provider.getActiveSpanProcessor(); - assert(processor instanceof BatchSpanProcessor); - // @ts-expect-error access configured to verify its the correct one - const exporter = processor._exporter; - assert(exporter instanceof DummyExporter); + + 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 + ); sinon.assert.calledOnceWithExactly( setGlobalPropagatorStub, @@ -485,11 +520,20 @@ describe('BasicTracerProvider', () => { const provider = new CustomTracerProvider({}); provider.register(); - const processor = provider.getActiveSpanProcessor(); - assert(processor instanceof BatchSpanProcessor); - // @ts-expect-error access configured to verify its the correct one - const exporter = processor._exporter; - assert(exporter instanceof DummyExporter); + + 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 + ); sinon.assert.calledOnceWithExactly( setGlobalPropagatorStub, @@ -589,11 +633,22 @@ describe('BasicTracerProvider', () => { envSource.OTEL_TRACES_EXPORTER = 'memory'; const provider = new CustomTracerProvider({}); provider.register(); - const processor = provider.getActiveSpanProcessor(); - assert(processor instanceof BatchSpanProcessor); - // @ts-expect-error access configured to verify its the correct one - const exporter = processor._exporter; - assert(exporter instanceof InMemorySpanExporter); + + 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 + ); }); }); }); @@ -779,6 +834,29 @@ 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 6b7cacdb48..2bf1e769f8 100644 --- a/packages/opentelemetry-sdk-trace-node/test/NodeTracerProvider.test.ts +++ b/packages/opentelemetry-sdk-trace-node/test/NodeTracerProvider.test.ts @@ -302,11 +302,23 @@ describe('NodeTracerProvider', () => { const provider = new CustomTracerProvider({}); provider.register(); - const processor = provider.getActiveSpanProcessor(); - assert(processor instanceof BatchSpanProcessor); - // @ts-expect-error access configured to verify its the correct one - const exporter = processor._exporter; - assert(exporter instanceof DummyExporter); + + 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(propagation['_getGlobalPropagator'](), propagator); }); @@ -347,11 +359,23 @@ describe('NodeTracerProvider', () => { const provider = new CustomTracerProvider({}); provider.register(); - const processor = provider.getActiveSpanProcessor(); - assert(processor instanceof BatchSpanProcessor); - // @ts-expect-error access configured to verify its the correct one - const exporter = processor._exporter; - assert(exporter instanceof DummyExporter); + + 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(propagation['_getGlobalPropagator'](), propagator); });