fix(sdk-node): use warn instead of error on unknown OTEL_NODE_RESOURCE_DETECTORS values #5034
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…CTORS values as per issue open-telemetry#4882. I referenced the OpenTelemetry specification document, which clarified that diag.warn should be used for unrecognized values that don't critically break the system.
|
Hi @matschaffer @weyert this PR is ready for review |
pichlermarc
changed the title
pichlermarc
added
bug
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5034 +/- ##
==========================================
- Coverage 93.39% 93.27% -0.13%
==========================================
Files 46 317 +271
Lines 712 8194 +7482
Branches 120 1640 +1520
==========================================
+ Hits 665 7643 +6978
- Misses 47 551 +504
|
pichlermarc
approved these changes
Oct 3, 2024
There was a problem hiding this comment.
@RichardChukwu thanks for fixing this 🙂
I took the liberty to change the PR title. Please also add a changelog entry in ./experimental/CHANGELOG.md (can be categorized as a bugfix) then we can merge this 🙂
bugfix changelog entry
|
@pichlermarc "I've added the changelog entry as requested. Let me know if anything else is needed. Thanks!" |
maryliag
reviewed
Oct 3, 2024
maryliag
approved these changes
Oct 3, 2024
|
Thanks for the approval @maryliag |
pichlermarc
reviewed
Oct 3, 2024
pichlermarc
approved these changes
Oct 3, 2024
Merged
via the queue into
open-telemetry:main
with commit Oct 3, 2024
e0e2b4a
20 of 21 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which problem is this PR solving?
This PR addresses the issue #4882, which involves logging an unrecognized OTEL_NODE_RESOURCE_DETECTORS value. According to the OpenTelemetry specification, unrecognized enum values should generate a warning rather than an error. The current implementation uses diag.error to log this issue, but it should use diag.warn instead to align with the specification and avoid unnecessary error-level logging.
Fixes: #4882
Short description of the changes
Updated opentelemetry-sdk-node/src/utils.ts and auto-instrumentations-node/src/utils.ts to replace diag.error with diag.warn for handling unrecognized OTEL_NODE_RESOURCE_DETECTORS values.
This change ensures that unrecognized values generate a warning, as specified in the OpenTelemetry SDK environment variable guidelines.
Type of change
Bug fix (non-breaking change which fixes an issue)
How Has This Been Tested?
Ran the existing test suite using npm test to confirm that all tests pass successfully.
Manually verified that unrecognized OTEL_NODE_RESOURCE_DETECTORS values log a warning instead of an error.
Checklist:
Followed the style guidelines of this project
Unit tests have been added (if applicable)
Documentation has been updated where necessary