fix(sdk-node): fix exporter to be read only OTEL_TRACES_EXPORTER is s…

…et to a valid exporter (#3492) * fix(sdk-node): refactor to use otlp as default exporter when env is empty Signed-off-by: Svetlana Brennan <[email protected]> * fix(sdk-node): add changelog Signed-off-by: Svetlana Brennan <[email protected]> * (sdk-node): using existing env source Signed-off-by: Svetlana Brennan <[email protected]> Signed-off-by: Svetlana Brennan <[email protected]>
open-telemetry · Jan 3, 2023 · 126ae93 · 126ae93
1 parent c93ab9e
commit 126ae93
Show file tree

Hide file tree

Showing 7 changed files with 92 additions and 26 deletions.
diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md
@@ -23,6 +23,7 @@ All notable changes to experimental packages in this project will be documented
 * fix(prometheus-sanitization): replace repeated `_` with a single `_` [3470](https://github.com/open-telemetry/opentelemetry-js/pull/3470) @samimusallam
 * fix(prometheus-serializer): correct string used for NaN [#3477](https://github.com/open-telemetry/opentelemetry-js/pull/3477) @JacksonWeber
 * fix(instrumentation-http): close server span when response finishes [#3407](https://github.com/open-telemetry/opentelemetry-js/pull/3407) @legendecas
+* fix(sdk-node): fix exporter to be read only OTEL_TRACES_EXPORTER is set to a valid exporter [3492] @svetlanabrennan
 
 ### :books: (Refine Doc)
 

diff --git a/experimental/packages/opentelemetry-sdk-node/src/TracerProviderWithEnvExporter.ts b/experimental/packages/opentelemetry-sdk-node/src/TracerProviderWithEnvExporter.ts
@@ -84,10 +84,22 @@ export class TracerProviderWithEnvExporters extends NodeTracerProvider {
       Array.from(new Set(getEnv().OTEL_TRACES_EXPORTER.split(',')))
     );
 
-    if (traceExportersList.length === 0 || traceExportersList[0] === 'none') {
+    if (traceExportersList[0] === 'none') {
       diag.warn(
-        'OTEL_TRACES_EXPORTER contains "none" or is empty. SDK will not be initialized.'
+        'OTEL_TRACES_EXPORTER contains "none". SDK will not be initialized.'
       );
+    } else if (traceExportersList.length === 0) {
+      diag.warn('OTEL_TRACES_EXPORTER is empty. Using default otlp exporter.');
+
+      traceExportersList = ['otlp'];
+      this.createExportersFromList(traceExportersList);
+
+      this._spanProcessors = this.configureSpanProcessors(
+        this._configuredExporters
+      );
+      this._spanProcessors.forEach(processor => {
+        this.addSpanProcessor(processor);
+      });
     } else {
       if (
         traceExportersList.length > 1 &&
@@ -99,16 +111,7 @@ export class TracerProviderWithEnvExporters extends NodeTracerProvider {
         traceExportersList = ['otlp'];
       }
 
-      traceExportersList.forEach(exporterName => {
-        const exporter = this._getSpanExporter(exporterName);
-        if (exporter) {
-          this._configuredExporters.push(exporter);
-        } else {
-          diag.warn(
-            `Unrecognized OTEL_TRACES_EXPORTER value: ${exporterName}.`
-          );
-        }
-      });
+      this.createExportersFromList(traceExportersList);
 
       if (this._configuredExporters.length > 0) {
         this._spanProcessors = this.configureSpanProcessors(
@@ -136,6 +139,17 @@ export class TracerProviderWithEnvExporters extends NodeTracerProvider {
     }
   }
 
+  private createExportersFromList(exporterList: string[]) {
+    exporterList.forEach(exporterName => {
+      const exporter = this._getSpanExporter(exporterName);
+      if (exporter) {
+        this._configuredExporters.push(exporter);
+      } else {
+        diag.warn(`Unrecognized OTEL_TRACES_EXPORTER value: ${exporterName}.`);
+      }
+    });
+  }
+
   private configureSpanProcessors(
     exporters: SpanExporter[]
   ): (BatchSpanProcessor | SimpleSpanProcessor)[] {

diff --git a/experimental/packages/opentelemetry-sdk-node/test/TracerProviderWithEnvExporter.test.ts b/experimental/packages/opentelemetry-sdk-node/test/TracerProviderWithEnvExporter.test.ts
@@ -96,15 +96,29 @@ describe('set up trace exporter with env exporters', () => {
       delete env.OTEL_EXPORTER_OTLP_PROTOCOL;
       delete env.OTEL_EXPORTER_OTLP_TRACES_PROTOCOL;
     });
-    it('do not use any exporters when empty value is provided for exporter', async () => {
+    it('use default otlp exporter when user does not set exporter via env or config', async () => {
+      const sdk = new TracerProviderWithEnvExporters();
+      const listOfProcessors = sdk['_spanProcessors']!;
+      const listOfExporters = sdk['_configuredExporters'];
+
+      assert(listOfExporters[0] instanceof OTLPProtoTraceExporter);
+      assert(listOfExporters.length === 1);
+
+      assert(listOfProcessors.length === 1);
+      assert(listOfProcessors[0] instanceof BatchSpanProcessor);
+    });
+    it('use default otlp exporter when empty value is provided for exporter via env', async () => {
       env.OTEL_TRACES_EXPORTER = '';
       const sdk = new TracerProviderWithEnvExporters();
-      const listOfProcessors = sdk['_spanProcessors'];
+      const listOfProcessors = sdk['_spanProcessors']!;
       const listOfExporters = sdk['_configuredExporters'];
 
-      assert(spyGetOtlpProtocol.notCalled);
-      assert(listOfExporters.length === 0);
-      assert(listOfProcessors === undefined);
+      assert(listOfExporters[0] instanceof OTLPProtoTraceExporter);
+      assert(listOfExporters.length === 1);
+
+      assert(listOfProcessors.length === 1);
+      assert(listOfProcessors[0] instanceof BatchSpanProcessor);
+
       env.OTEL_TRACES_EXPORTER = '';
     });
     it('do not use any exporters when none value is only provided', async () => {
@@ -124,7 +138,7 @@ describe('set up trace exporter with env exporters', () => {
 
       assert.strictEqual(
         stubLoggerError.args[0][0],
-        'OTEL_TRACES_EXPORTER contains "none" or is empty. SDK will not be initialized.'
+        'OTEL_TRACES_EXPORTER contains "none". SDK will not be initialized.'
       );
       delete env.OTEL_TRACES_EXPORTER;
     });
@@ -174,6 +188,11 @@ describe('set up trace exporter with env exporters', () => {
 
       assert.strictEqual(
         stubLoggerError.args[0][0],
+        'OTEL_TRACES_EXPORTER is empty. Using default otlp exporter.'
+      );
+
+      assert.strictEqual(
+        stubLoggerError.args[1][0],
         'Unsupported OTLP traces protocol: invalid. Using http/protobuf.'
       );
       delete env.OTEL_EXPORTER_OTLP_PROTOCOL;

diff --git a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts
@@ -769,21 +769,30 @@ describe('setup exporter from env', () => {
 
     assert.strictEqual(
       stubLoggerError.args[0][0],
-      'OTEL_TRACES_EXPORTER contains "none" or is empty. SDK will not be initialized.'
+      'OTEL_TRACES_EXPORTER contains "none". SDK will not be initialized.'
     );
     delete env.OTEL_TRACES_EXPORTER;
   });
-  it('do not use any exporters when empty value is provided for exporter', async () => {
-    env.OTEL_TRACES_EXPORTER = '';
+  it('use default otlp exporter when user does not set exporter via env or config', async () => {
     const sdk = new NodeSDK();
     await sdk.start();
 
     const listOfProcessors =
       sdk['_tracerProvider']!['_registeredSpanProcessors']!;
-    const activeProcessor = sdk['_tracerProvider']?.getActiveSpanProcessor();
+    assert(sdk['_tracerProvider'] instanceof TracerProviderWithEnvExporters);
+    assert(listOfProcessors.length === 1);
+    assert(listOfProcessors[0] instanceof BatchSpanProcessor);
+  });
+  it('use default otlp exporter when empty value is provided for exporter via env', async () => {
+    env.OTEL_TRACES_EXPORTER = '';
+    const sdk = new NodeSDK();
+    await sdk.start();
 
-    assert(listOfProcessors.length === 0);
-    assert(activeProcessor instanceof NoopSpanProcessor);
+    const listOfProcessors =
+      sdk['_tracerProvider']!['_registeredSpanProcessors']!;
+    assert(sdk['_tracerProvider'] instanceof TracerProviderWithEnvExporters);
+    assert(listOfProcessors.length === 1);
+    assert(listOfProcessors[0] instanceof BatchSpanProcessor);
     env.OTEL_TRACES_EXPORTER = '';
   });
 

diff --git a/packages/opentelemetry-core/src/utils/environment.ts b/packages/opentelemetry-core/src/utils/environment.ts
@@ -172,7 +172,7 @@ export const DEFAULT_ENVIRONMENT: Required<ENVIRONMENT> = {
   OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT: DEFAULT_ATTRIBUTE_COUNT_LIMIT,
   OTEL_SPAN_EVENT_COUNT_LIMIT: 128,
   OTEL_SPAN_LINK_COUNT_LIMIT: 128,
-  OTEL_TRACES_EXPORTER: 'otlp',
+  OTEL_TRACES_EXPORTER: '',
   OTEL_TRACES_SAMPLER: TracesSamplerValues.ParentBasedAlwaysOn,
   OTEL_TRACES_SAMPLER_ARG: '',
   OTEL_EXPORTER_OTLP_INSECURE: '',

diff --git a/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts b/packages/opentelemetry-sdk-trace-base/src/BasicTracerProvider.ts
@@ -275,7 +275,7 @@ export class BasicTracerProvider implements TracerProvider {
 
   protected _buildExporterFromEnv(): SpanExporter | undefined {
     const exporterName = getEnv().OTEL_TRACES_EXPORTER;
-    if (exporterName === 'none') return;
+    if (exporterName === 'none' || exporterName === '') return;
     const exporter = this._getSpanExporter(exporterName);
     if (!exporter) {
       diag.error(

diff --git a/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts b/packages/opentelemetry-sdk-trace-base/test/common/BasicTracerProvider.test.ts
@@ -90,6 +90,29 @@ describe('BasicTracerProvider', () => {
         const tracer = new BasicTracerProvider();
         assert.ok(tracer.activeSpanProcessor 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);
+
+        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 tracer = new BasicTracerProvider();
+        assert.ok(tracer.activeSpanProcessor instanceof NoopSpanProcessor);
+
+        sinon.assert.calledWith(
+          errorStub,
+          'Exporter "someExporter" requested through environment variable is unavailable.'
+        );
+        delete envSource.OTEL_TRACES_EXPORTER;
+      });
     });
 
     describe('when "sampler" option defined', () => {