-
Notifications
You must be signed in to change notification settings - Fork 370
sync span based baggage APIs with global baggage APIs #7453
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
Open
ida613
wants to merge
10
commits into
master
Choose a base branch
from
ida613/otel-baggage-api-fix
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+51
−97
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
f6e65d6
sync span based baggage apis with global baggage apis
ida613 5b5901b
fix linter errors
ida613 0251bf5
PR comment round 1
ida613 0767df5
fix shallow copy of baggage
ida613 704540a
swap structuredClone with rfdc
ida613 4522c66
let dd baggage always override otel baggage
ida613 0bda3cf
let whichever baggage is set latest take precedence
ida613 6af793c
use global api (draft)
ida613 5e35934
update unit test
ida613 2fa1be7
fix linter errors
ida613 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
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.
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.
Could you elaborate about this? I am unsure what this does.
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 calling the global baggage APIs to make sure the currently active baggage is in sync with baggage on the context. It essentially does the same thing as these operations but for global baggage
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.
Should Otel baggage overrule DD baggage? Should it be merged? Do we have any documentation or RFCs around that or was there any discussion around that?
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.
Good question. The code lets whichever comes later override, but I'm not sure if this is what's intended. @rachelyangdog @zacharycmontoya do you think we need to discuss this before proceeding?
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.
so turns out dd baggage should always override otel baggage, updated the code
Uh oh!
There was an error while loading. Please reload this page.
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.
Thank you Zach! I can do this no problem, but currently in node, dd and otel baggages only get synced when we call active or with. In order to do what dotnet does (syncing on each individual operation), we would have to replace the otel baggage operations within dd-trace, which will involve a ton of work. Is it acceptable to only sync when calling
withoractive?Sounds good with me, it's not hard to do. I'll work on it :)
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.
Yes I think it should be fine to only update when calling
activeorwith. Let me describe the interactions I expect, but if my understanding is wrong please point it out so we're operating off the same understanding.DD API
Each time the user wants to read/write to the active global baggage, they call one of the Baggage API's here. Notably, when clearing or setting baggage, these API's will automatically update the current baggage before returning to the caller. As long as we instrument each time
activeorwithis called in the OpenTelemetry context manager, we don't need to update any code here.OTel API
Each time the user wants to read/write to the active global baggage, they will need to get the current baggage by calling getActiveBagage which gets the current context by calling active. Then, once the user has updated the baggage and wants to set it as current, the user must attach the baggage to a Context using the OTel API setBaggage and call the api.context.with API with the Context and a callback, so the baggage will become active.
So with this flow, we just need our
activeoperation to copy over the Datadog global baggage to overwrite all the OpenTelemetry baggage in the context and ourwithoperation to copy over the OpenTelemetry baggage to override all the Datadog baggage.And actually, this is exactly what my .NET implementation does! 😄 It just turns out that the .NET OTel Baggage API's directly set the "current" OTel baggage API within each API call (Set/Remove) so we just have to do the
active/withimplementation in more placesThere 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 Zach! that's my understanding too. I've updated the code accordingly
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.
@zacharycmontoya turns out legacy baggage and the new baggage are extracted/injected and propagated independently, so there will be never be a key conflict. But the legacy baggage never interact with the otel drop in code, is this okay?
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.
Yes that's okay 👍🏼