Skip to content

Commit

Permalink
fix(sdk-node): avoid spurious diag errors for unknown OTEL_NODE_RESOU…
Browse files Browse the repository at this point in the history
…RCE_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: open-telemetry/opentelemetry-js-contrib#2311

* add a changelog entry

* lint:fix
  • Loading branch information
trentm authored Jul 30, 2024
1 parent d91dbe1 commit 3f2c707
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 11 deletions.
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
17 changes: 8 additions & 9 deletions experimental/packages/opentelemetry-sdk-node/src/sdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 ||
Expand Down
50 changes: 49 additions & 1 deletion experimental/packages/opentelemetry-sdk-node/test/sdk.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ import { env } from 'process';
import { TracerProviderWithEnvExporters } from '../src/TracerProviderWithEnvExporter';
import {
envDetector,
envDetectorSync,
processDetector,
hostDetector,
Resource,
Expand Down Expand Up @@ -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 = (
Expand Down Expand Up @@ -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';
Expand Down

0 comments on commit 3f2c707

Please sign in to comment.