fix(sdk-node): avoid spurious diag errors for unknown OTEL_NODE_RESOU…

…RCE_DETECTORS values (open-telemetry#4879) * fix(sdk-node): avoid spurious diag errors for unknown OTEL_NODE_RESOURCE_DETECTORS values When NodeSDK is configured with explicit 'resourceDetectors' or with 'autoDetectResources: false', then it should not emit diag errors about unknown values in OTEL_NODE_RESOURCE_DETECTORS. This can happen when that envvar is used with @opentelemetry/auto-instrumentation-node Closes: open-telemetry/opentelemetry-js-contrib#2311 * add a changelog entry * lint:fix
Zirak · Sep 14, 2024 · 76c21fa · 76c21fa
1 parent 9bb4ff4
commit 76c21fa
Show file tree

Hide file tree

Showing 3 changed files with 59 additions and 11 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -16,7 +16,8 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/
 
 ### :bug: (Bug Fix)
 
-- deps(opentelemetry-instrumentation): Bump `shimmer` types to 1.2.0 [#4865](https://github.com/open-telemetry/opentelemetry-js/pull/4865) @lforst
+* fix(sdk-node): avoid spurious diag errors for unknown OTEL_NODE_RESOURCE_DETECTORS values [#4879](https://github.com/open-telemetry/opentelemetry-js/pull/4879) @trentm
+* deps(opentelemetry-instrumentation): Bump `shimmer` types to 1.2.0 [#4865](https://github.com/open-telemetry/opentelemetry-js/pull/4865) @lforst
 
 ### :books: (Refine Doc)
 

diff --git a/experimental/packages/opentelemetry-sdk-node/src/sdk.ts b/experimental/packages/opentelemetry-sdk-node/src/sdk.ts
@@ -121,20 +121,19 @@ export class NodeSDK {
     this._configuration = configuration;
 
     this._resource = configuration.resource ?? new Resource({});
-    let defaultDetectors: (Detector | DetectorSync)[] = [];
-    if (process.env.OTEL_NODE_RESOURCE_DETECTORS != null) {
-      defaultDetectors = getResourceDetectorsFromEnv();
+    this._autoDetectResources = configuration.autoDetectResources ?? true;
+    if (!this._autoDetectResources) {
+      this._resourceDetectors = [];
+    } else if (configuration.resourceDetectors != null) {
+      this._resourceDetectors = configuration.resourceDetectors;
+    } else if (process.env.OTEL_NODE_RESOURCE_DETECTORS != null) {
+      this._resourceDetectors = getResourceDetectorsFromEnv();
     } else {
-      defaultDetectors = [envDetector, processDetector, hostDetector];
+      this._resourceDetectors = [envDetector, processDetector, hostDetector];
     }
 
-    this._resourceDetectors =
-      configuration.resourceDetectors ?? defaultDetectors;
-
     this._serviceName = configuration.serviceName;
 
-    this._autoDetectResources = configuration.autoDetectResources ?? true;
-
     // If a tracer provider can be created from manual configuration, create it
     if (
       configuration.traceExporter ||

diff --git a/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts b/experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts
@@ -59,6 +59,7 @@ import { env } from 'process';
 import { TracerProviderWithEnvExporters } from '../src/TracerProviderWithEnvExporter';
 import {
   envDetector,
+  envDetectorSync,
   processDetector,
   hostDetector,
   Resource,
@@ -528,7 +529,7 @@ describe('Node SDK', () => {
       });
     });
 
-    describe('with a debug logger', () => {
+    describe('with a diag logger', () => {
       // Local functions to test if a mocked method is ever called with a specific argument or regex matching for an argument.
       // Needed because of race condition with parallel detectors.
       const callArgsContains = (
@@ -582,6 +583,53 @@ describe('Node SDK', () => {
         await sdk.shutdown();
       });
 
+      describe('with unknown OTEL_NODE_RESOURCE_DETECTORS value', () => {
+        before(() => {
+          process.env.OTEL_NODE_RESOURCE_DETECTORS = 'env,os,no-such-detector';
+        });
+
+        after(() => {
+          delete process.env.OTEL_NODE_RESOURCE_DETECTORS;
+        });
+
+        // 1. If not auto-detecting resources, then NodeSDK should not
+        //    complain about `OTEL_NODE_RESOURCE_DETECTORS` values.
+        // 2. If given resourceDetectors, then NodeSDK should not complain
+        //    about `OTEL_NODE_RESOURCE_DETECTORS` values.
+        //
+        // Practically, these tests help ensure that there is no spurious
+        // diag error message when using OTEL_NODE_RESOURCE_DETECTORS with
+        // @opentelemetry/auto-instrumentations-node, which supports more values
+        // than this package (e.g. 'gcp').
+        it('does not diag.warn when not using the envvar', async () => {
+          const diagMocks = {
+            error: Sinon.fake(),
+            warn: Sinon.fake(),
+            info: Sinon.fake(),
+            debug: Sinon.fake(),
+            verbose: Sinon.fake(),
+          };
+          diag.setLogger(diagMocks, DiagLogLevel.DEBUG);
+
+          const sdk1 = new NodeSDK({
+            autoDetectResources: false,
+          });
+          sdk1.start();
+          await sdk1.shutdown();
+
+          const sdk2 = new NodeSDK({
+            resourceDetectors: [envDetectorSync],
+          });
+          sdk2.start();
+          await sdk2.shutdown();
+
+          assert.ok(
+            !callArgsMatches(diagMocks.error, /no-such-detector/),
+            'diag.error() messages do not mention "no-such-detector"'
+          );
+        });
+      });
+
       describe('with a faulty environment variable', () => {
         beforeEach(() => {
           process.env.OTEL_RESOURCE_ATTRIBUTES = 'bad=\\attribute';