refactor(sdk-trace-base): remove _registeredSpanProcessors from Bas…

…icTracerProvider (#5177)
open-telemetry · Nov 21, 2024 · f3a6310 · f3a6310
1 parent b7343ef
commit f3a6310
Show file tree

Hide file tree

Showing 6 changed files with 202 additions and 98 deletions.
diff --git 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
@@ -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
@@ -67,10 +67,9 @@ export class BasicTracerProvider implements TracerProvider {
   >();
 
   private readonly _config: TracerConfig;
-  private readonly _registeredSpanProcessors: SpanProcessor[] = [];
   private readonly _tracers: Map<string, Tracer> = 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<void> {
     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
@@ -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,