-
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
base: master
Are you sure you want to change the base?
Changes from 2 commits
f6e65d6
5b5901b
0251bf5
0767df5
704540a
4522c66
0bda3cf
6af793c
5e35934
2fa1be7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
|
|
||
| const { trace, ROOT_CONTEXT, propagation } = require('@opentelemetry/api') | ||
| const { storage } = require('../../../datadog-core') | ||
| const { getAllBaggageItems, setBaggageItem, removeAllBaggageItems } = require('../baggage') | ||
|
|
||
| const tracer = require('../../') | ||
| const SpanContext = require('./span_context') | ||
|
|
@@ -11,6 +12,15 @@ class ContextManager { | |
| this._store = storage('opentelemetry') | ||
| } | ||
|
|
||
| _mergeGlobalBaggageWith (baggages) { | ||
| baggages = baggages || {} | ||
| const globalActiveBaggages = getAllBaggageItems() | ||
| for (const [key, value] of Object.entries(globalActiveBaggages)) { | ||
| if (!baggages[key]) baggages[key] = value | ||
| } | ||
| return baggages | ||
| } | ||
|
|
||
| // converts dd to otel | ||
| active () { | ||
| const store = this._store.getStore() | ||
|
|
@@ -21,7 +31,7 @@ class ContextManager { | |
|
|
||
| // If stored span wraps the active DD span, prefer the stored context | ||
| if (storedSpan && storedSpan._ddSpan === activeSpan) { | ||
| const baggages = JSON.parse(activeSpan.getAllBaggageItems()) | ||
| const baggages = this._mergeGlobalBaggageWith(JSON.parse(activeSpan.getAllBaggageItems())) | ||
| if (Object.keys(baggages).length > 0) { | ||
| const entries = {} | ||
| for (const [key, value] of Object.entries(baggages)) { | ||
|
|
@@ -34,7 +44,7 @@ class ContextManager { | |
| } | ||
|
|
||
| if (!activeSpan) { | ||
| const storedBaggageItems = storedSpan?._spanContext?._ddContext?._baggageItems | ||
| const storedBaggageItems = this._mergeGlobalBaggageWith(storedSpan?._spanContext?._ddContext?._baggageItems) | ||
BridgeAR marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| if (storedBaggageItems) { | ||
| const baggages = storedBaggageItems | ||
| const entries = {} | ||
|
|
@@ -54,7 +64,7 @@ class ContextManager { | |
| } | ||
|
|
||
| // Convert DD baggage to OTel format | ||
| const baggages = JSON.parse(activeSpan.getAllBaggageItems()) | ||
| const baggages = this._mergeGlobalBaggageWith(JSON.parse(activeSpan.getAllBaggageItems())) | ||
| const hasBaggage = Object.keys(baggages).length > 0 | ||
| let otelBaggages | ||
| if (hasBaggage) { | ||
|
|
@@ -86,6 +96,10 @@ class ContextManager { | |
| if (baggages) { | ||
| baggageItems = baggages.getAllEntries() | ||
| } | ||
| removeAllBaggageItems() | ||
| for (const baggage of baggageItems) { | ||
| setBaggageItem(baggage[0], baggage[1].value) | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you elaborate about this? I am unsure what this does.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Sounds good with me, it's not hard to do. I'll work on it :)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I think it should be fine to only update when calling DD APIEach 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 OTel APIEach 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 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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes that's okay 👍🏼 |
||
| if (span && span._ddSpan) { | ||
| // does otel always override datadog? | ||
| span._ddSpan.removeAllBaggageItems() | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.