Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(sdk-node): avoid spurious diag errors for unknown OTEL_NODE_RESOURCE_DETECTORS values #4879

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this also have a test to make sure it is still sending the error message when it would? Just in case any code changes remove the message completely

just a nit, feel free to ignore it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair, though I'd rather do that in a separate change... because I'd want to change its behaviour. :) Currently it is a diag.error, but I think it should be a diag.warn.

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk-environment-variables.md#enum-value says:

if the user provides a value the implementation does not recognize, the implementation MUST generate a warning and gracefully ignore the setting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#4882 added for that.

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