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

When using auto-instrumentations-node, some values of OTEL_NODE_RESOURCE_DETECTORS are treated as invalid #2311

Closed
qbedard opened this issue Jul 2, 2024 · 1 comment · Fixed by open-telemetry/opentelemetry-js#4879
Assignees
Labels
bug Something isn't working priority:p4 Bugs and spec inconsistencies which do not fall into a higher prioritization

Comments

@qbedard
Copy link

qbedard commented Jul 2, 2024

What version of OpenTelemetry are you using?

@opentelemetry/api@1.9.0
@opentelemetry/auto-instrumentations-node@0.47.1
@opentelemetry/exporter-trace-otlp-http@0.52.1
@opentelemetry/instrumentation-express@0.40.1
@opentelemetry/instrumentation-http@0.52.1
@opentelemetry/instrumentation-undici@0.3.0
@opentelemetry/resource-detector-aws@1.5.1
@opentelemetry/sdk-node@0.52.1
@opentelemetry/sdk-trace-node@1.25.1

What version of Node are you using?

18.12.1

What did you do?

Use auto-instrumentations-node with OTEL_NODE_RESOURCE_DETECTORS set to aws,env,host,os.

What did you expect to see?

AWS resource detector is enabled.

What did you see instead?

Invalid resource detector "aws" specified in the environment variable OTEL_NODE_RESOURCE_DETECTORS

Additional context

This conditional is evaluating to false-y and appears to do so for all resource detectors that are of type Detector or Detector[]. Those that have values of DetectorSync or DetectorSync[] do work, so the issue is likely that a Detector instance is evaluating to false-y and the linked conditional or the detector map needs to be updated to evaluate those values properly.

@trentm
Copy link
Contributor

trentm commented Jul 4, 2024

@qbedard Thanks for the issue, and the PR.

I believe what is happening here is that those Invalid resource detector "aws" specified ... diag.error messages are actually coming from the underlying @opentelemetry/sdk-node package that @opentelemetry/auto-instrumentations-node is using.

For example:

% cat foo.js
console.log('hi')
[16:59:00 trentm@peach:~/tmp/asdf]
% OTEL_NODE_RESOURCE_DETECTORS=foo,gcp,alibaba,aws,env,host,os node -r @opentelemetry/auto-instrumentations-node/register foo.js
Invalid resource detector "foo" specified in the environment variable OTEL_NODE_RESOURCE_DETECTORS
Invalid resource detector "foo" specified in the environment variable OTEL_NODE_RESOURCE_DETECTORS
Invalid resource detector "gcp" specified in the environment variable OTEL_NODE_RESOURCE_DETECTORS
Invalid resource detector "alibaba" specified in the environment variable OTEL_NODE_RESOURCE_DETECTORS
Invalid resource detector "aws" specified in the environment variable OTEL_NODE_RESOURCE_DETECTORS
OTEL_TRACES_EXPORTER is empty. Using default otlp exporter.
OpenTelemetry automatic instrumentation started successfully
hi
Process is not running on K8S [Error: ENOENT: no such file or directory, access '/var/run/secrets/kubernetes.io/serviceaccount/token'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'access',
  path: '/var/run/secrets/kubernetes.io/serviceaccount/token'
}

Observe that there are two diag error messages about the invalid "foo" resource detector name.
What is happening in:

  1. auto-instrumentations-node runs its getResourceDetectorsFromEnv (
    resourceDetectors: getResourceDetectorsFromEnv(),
    ), which successfully loads the "gcp", "alibaba", and "aws" detectors.
  2. Then it calls new NodeSDK (from @opentelemetry/sdk-node). NodeSDK's constructor calls its getResourceDetectorsFromEnv, even if configuration.resourceDetectors is passed in: https://github.com/open-telemetry/opentelemetry-js/blob/a037f84b679386daefd726133be6265c3c43de27/experimental/packages/opentelemetry-sdk-node/src/sdk.ts#L123-L132

I believe an appropriate fix for this would be to improve the NodeSDK constructor to not do the defaultDetectors processing if configuration.resourceDetectors is passed in. That would get rid of some unnecessary processing and would get rid of the diag.error messages.


Would you be interested in attempting a PR that does that?

@trentm trentm self-assigned this Jul 24, 2024
@trentm trentm added the priority:p4 Bugs and spec inconsistencies which do not fall into a higher prioritization label Jul 24, 2024
trentm added a commit to trentm/opentelemetry-js that referenced this issue Jul 25, 2024
…RCE_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
github-merge-queue bot pushed a commit to open-telemetry/opentelemetry-js that referenced this issue Jul 30, 2024
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority:p4 Bugs and spec inconsistencies which do not fall into a higher prioritization
Projects
None yet
2 participants