sync span based baggage APIs with global baggage APIs#7453
sync span based baggage APIs with global baggage APIs#7453
Conversation
Overall package sizeSelf size: 4.64 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 2.0.6 | 81.92 kB | 813.08 kB | | dc-polyfill | 0.1.10 | 26.73 kB | 26.73 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7453 +/- ##
==========================================
- Coverage 80.40% 80.21% -0.20%
==========================================
Files 732 731 -1
Lines 31050 31364 +314
==========================================
+ Hits 24966 25158 +192
- Misses 6084 6206 +122
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
BenchmarksBenchmark execution time: 2026-02-17 15:34:46 Comparing candidate commit 2fa1be7 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 230 metrics, 30 unstable metrics. |
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🔗 Commit SHA: 2fa1be7 | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
ida613
changed the title
ida613
requested review from
a team and
BridgeAR
and removed request for
a team
| removeAllBaggageItems() | ||
| for (const baggage of baggageItems) { | ||
| setBaggageItem(baggage[0], baggage[1].value) | ||
| } |
There was a problem hiding this comment.
Could you elaborate about this? I am unsure what this does.
There was a problem hiding this comment.
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.
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.
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.
so turns out dd baggage should always override otel baggage, updated the code
There was a problem hiding this comment.
Yes I support this approach. Legacy baggage won't be included in this support. We will only keep the Current OTel API Baggage in sync with the global Datadog baggage that's tracked independently of spans.
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 with or active?
Did we already establish a precedence for this scenario or are we only now adding that? My preference would be to prefer global baggage, aka from the baggage HTTP header. How feasible would that be and do you see any issues with that preference?
Sounds good with me, it's not hard to do. I'll work on it :)
There was a problem hiding this comment.
Yes I think it should be fine to only update when calling active or with. 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 active or with is 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 active operation to copy over the Datadog global baggage to overwrite all the OpenTelemetry baggage in the context and our with operation 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/with implementation in more places
There was a problem hiding this comment.
thanks Zach! that's my understanding too. I've updated the code accordingly
There was a problem hiding this comment.
@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.
Yes that's okay 👍🏼
| const SpanContext = require('./span_context') | ||
|
|
||
| function mergeGlobalBaggageWith (baggages) { | ||
| const combinedBaggages = structuredClone(baggages) || {} |
There was a problem hiding this comment.
| const combinedBaggages = structuredClone(baggages) || {} | |
| const combinedBaggages = baggages ? rfdc(baggages) : {} |
Please use rfdc. structuredClone is relatively slow. This must be required at the top and I believe we used a vendored rfdc. Check other usages across the repo.
In addition: we do not need to clone anything if we called JSON.parse() before. We could just do the cloning before we pass it to the method.
There was a problem hiding this comment.
updated, thank you Ruben! this is really insightful :)
There was a problem hiding this comment.
Could you please move the cloning to only happen where it is needed?
There was a problem hiding this comment.
is this good practice when we could also pass an object into it?
There was a problem hiding this comment.
I am not sure I understand. I meant we should just copy that object before passing it through. That way we do not have to copy in all cases but only in one.
There was a problem hiding this comment.
I've removed this helper function in the latest commits
3 participants
What does this PR do?
Make sure baggage added using the global APIs are in sync with the span APIs. Since span APIs are legacy, they take precedence. It is still possible to add baggage using the global APIs and to not have them also added to the legacy baggages.
Motivation
Additional Notes