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

Conversation

trentm
Copy link
Contributor

@trentm trentm commented Jul 25, 2024

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,
because it won't use the values in that envvar.
This can happen when that envvar is used with
@opentelemetry/auto-instrumentation-node

Closes: open-telemetry/opentelemetry-js-contrib#2311

…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
@trentm trentm self-assigned this Jul 25, 2024
@trentm trentm requested a review from a team July 25, 2024 20:15
// 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.

Copy link

codecov bot commented Jul 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.58%. Comparing base (ecc88a3) to head (0b05b45).
Report is 62 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4879      +/-   ##
==========================================
+ Coverage   91.04%   92.58%   +1.53%     
==========================================
  Files          89       65      -24     
  Lines        1954      930    -1024     
  Branches      416      157     -259     
==========================================
- Hits         1779      861     -918     
+ Misses        175       69     -106     

see 24 files with indirect coverage changes

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Looks good, thank you for taking care of this 👍

@pichlermarc pichlermarc added this pull request to the merge queue Jul 30, 2024
Merged via the queue into open-telemetry:main with commit 3f2c707 Jul 30, 2024
21 checks passed
@trentm trentm deleted the tm-avoid-double-getResourceDetectorsFromEnv branch July 31, 2024 00:49
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When using auto-instrumentations-node, some values of OTEL_NODE_RESOURCE_DETECTORS are treated as invalid
3 participants