From 76c21fad5963523dca6be20c097949142bdafd70 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 30 Jul 2024 00:47:22 -0700 Subject: [PATCH] fix(sdk-node): avoid spurious diag errors for unknown OTEL_NODE_RESOURCE_DETECTORS values (#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: https://github.com/open-telemetry/opentelemetry-js-contrib/issues/2311 * add a changelog entry * lint:fix --- CHANGELOG.md | 3 +- .../opentelemetry-sdk-node/src/sdk.ts | 17 +++---- .../opentelemetry-sdk-node/test/sdk.test.ts | 50 ++++++++++++++++++- 3 files changed, 59 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a9993acea..c41a34ab80 100644 --- 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 index 7cb02a2120..24a7d2332d 100644 --- 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 index 12cf964f20..f394dbc91a 100644 --- 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';