-
Notifications
You must be signed in to change notification settings - Fork 983
feat(opentelemetry-sdk-node): use declarative config for resource attributes #6044
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6044 +/- ##
==========================================
+ Coverage 95.21% 95.23% +0.01%
==========================================
Files 316 317 +1
Lines 9389 9395 +6
Branches 2167 2166 -1
==========================================
+ Hits 8940 8947 +7
+ Misses 449 448 -1
🚀 New features to boost your workflow:
|
pichlermarc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quick review - I did not have a detailed look at this PR yet.
…ders (open-telemetry#6058) Co-authored-by: Marylia Gutierrez <[email protected]>
Co-authored-by: Marylia Gutierrez <[email protected]>
JamieDanielson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good after the changes we talked about... one question I had around our checking for a valid config file that may have unexpected / unacceptable consequences if someone accidentally puts an invalid value for their config file env var.
|
@JamieDanielson the PR with the changes on error -> warning got merged, so PTAL in this one |
JamieDanielson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exciting!
JamieDanielson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make sure we update the default value before merging this in.
JamieDanielson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had some further discussion during the SIG meeting about whether we want a separate, parallel path in sdk-node that utilizes the config file (instead of implementing the behavior in the current class).
While this will result in some code duplication in the short-term, it also provides a clean slate to implement a newer, better designed configuration that uses functions to initialize the SDK instead of a class, allowing more flexibility in our implementation, and later we can deprecate the NodeSDK class when we are ready. This would also allow us to move quickly with minimal risk because it is essentially feature flagged; we could even put it in an experimental entrypoint.
There is a concern about confusion to end users - will it be obvious which setup path they should take? - but the tradeoff may be worth it, especially if we keep it in an experimental entrypoint and/or add lots of code comments around the experimental path.
|
closing this as we will use a different approach on #6145, starting with a new function |
Start to use configuration package on sdk-node.
For now is a basic usage, just to get feedback on implementation. Things should work normally without changes.
Using values for sdk disable, log level, and resource attributes from config model