-
Notifications
You must be signed in to change notification settings - Fork 518
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
feat: Add new baggage span processor package #2114
feat: Add new baggage span processor package #2114
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2114 +/- ##
==========================================
+ Coverage 90.97% 95.12% +4.14%
==========================================
Files 146 10 -136
Lines 7492 697 -6795
Branches 1502 143 -1359
==========================================
- Hits 6816 663 -6153
+ Misses 676 34 -642 |
I think .github/component_owners.yml should be also updated Usually the |
…opentelemetry-js-contrib into mike/baggage-span-processor
Thanks for the review @Flarna - I've updated the directory structure and fixed the readme title. |
Thanks for the suggestions @hectorhdzg - I've made some updates. Let me know what you think. |
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.
Thanks @MikeGoldsmith ! I've added a few suggestions for changes.
We'll also want to add to
|
Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>
Co-authored-by: Jamie Danielson <jamieedanielson@gmail.com>
Thanks for review @JamieDanielson - I've worked through your feedback and suggestions. I think this is ready for another review. |
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.
LGTM
…b into mike/baggage-span-processor # Conflicts: # .release-please-manifest.json
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.
I think many users can benefit from this capability, thank you.
Added 2 non-blocking suggestions / discussions which I think might worth consideration so that we don't end up introducing breaking changes in the future or promoting something that might be unsafe by default.
@MikeGoldsmith current PR already add a useful feature so let me know if you prefer not to address those at the moment.
@Flarna @hectorhdzg @blumamir Is there anything else you would like to see before this PR is accepted? |
Ping @open-telemetry/javascript-maintainers - looking for further feedback on this PR. |
Which problem is this PR solving?
Adds new baggage span processor as discussed in the following issue:
I've added to /packges directory - let me know if you'd prefer it somewhere else.
Short description of the changes