-
Notifications
You must be signed in to change notification settings - Fork 318
Support propagating OTel API created baggage via outgoing W3C headers #9987
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
Changes from all commits
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 |
|---|---|---|
|
|
@@ -338,43 +338,90 @@ class ContextTest extends InstrumentationSpecification { | |
| otelBaggageFromContext.isEmpty() | ||
| otelBaggageFromContextOrNull == null | ||
|
|
||
| when: | ||
| def otelScope = Baggage.builder() | ||
| .put("foo", "otel_value_to_be_replaced") | ||
| .put("FOO","OTEL_UNTOUCHED") | ||
| .put("remove_me_key", "otel_remove_me_value") | ||
| .build() | ||
| .makeCurrent() | ||
|
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. ❓ (For my own understanding). I assume that this
Contributor
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. Note this is
Notice how it's implemented by getting the current
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. Most of this makes sense. The part that I'm confused about is how our Hope the question makes sense!
Contributor
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. sure, that's done by instrumenting the underlying storage: https://github.com/DataDog/dd-trace-java/blob/master/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/context/OpenTelemetryContextStorageInstrumentation.java#L97
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. Ahh that makes sense. We instrument the calls made to OTel Context and specify to use our |
||
| otelBaggage = Baggage.current() | ||
| otelBaggageFromContext = Baggage.fromContext(Context.current()) | ||
| otelBaggageFromContextOrNull = Baggage.fromContextOrNull(Context.current()) | ||
|
|
||
| then: "OTel baggage must be current" | ||
| otelBaggage != null | ||
| otelBaggage.size() == 3 | ||
| otelBaggage.getEntryValue("foo") == "otel_value_to_be_replaced" | ||
| otelBaggage.getEntryValue("FOO") == "OTEL_UNTOUCHED" | ||
| otelBaggage.getEntryValue("remove_me_key") == "otel_remove_me_value" | ||
| otelBaggage.asMap() == otelBaggageFromContext.asMap() | ||
| otelBaggage.asMap() == otelBaggageFromContextOrNull.asMap() | ||
|
|
||
| when: | ||
| def ddContext = datadog.context.Context.current() | ||
| def ddBaggage = datadog.trace.bootstrap.instrumentation.api.Baggage.create([ | ||
| "foo" : "value_to_be_replaced", | ||
| "FOO" : "UNTOUCHED", | ||
| "remove_me_key" : "remove_me_value" | ||
| ]) | ||
| def ddBaggage = datadog.trace.bootstrap.instrumentation.api.Baggage.fromContext(ddContext) | ||
| ddBaggage.addItem("new_foo", "dd_new_value") | ||
| ddBaggage.addItem("foo", "dd_overwrite_value") | ||
| ddBaggage.removeItem("remove_me_key") | ||
| def ddScope = ddContext.with(ddBaggage).attach() | ||
| otelBaggage = Baggage.current() | ||
| otelBaggageFromContext = Baggage.fromContext(Context.current()) | ||
| otelBaggageFromContextOrNull = Baggage.fromContextOrNull(Context.current()) | ||
|
|
||
| then: "baggage must contain Datadog changes" | ||
| otelBaggage != null | ||
| otelBaggage.size() == 3 | ||
| otelBaggage.getEntryValue("foo") == "dd_overwrite_value" | ||
| otelBaggage.getEntryValue("FOO") == "OTEL_UNTOUCHED" | ||
| otelBaggage.getEntryValue("new_foo") == "dd_new_value" | ||
| otelBaggage.asMap() == otelBaggageFromContext.asMap() | ||
| otelBaggage.asMap() == otelBaggageFromContextOrNull.asMap() | ||
|
|
||
| when: | ||
| ddScope.close() | ||
| otelScope.close() | ||
|
|
||
| then: "current baggage must be empty or null" | ||
| Baggage.current().isEmpty() | ||
|
|
||
| when: | ||
| ddContext = datadog.context.Context.current() | ||
| ddBaggage = datadog.trace.bootstrap.instrumentation.api.Baggage.create([ | ||
| "foo" : "dd_value_to_be_replaced", | ||
| "FOO" : "DD_UNTOUCHED", | ||
| "remove_me_key" : "dd_remove_me_value" | ||
| ]) | ||
| ddScope = ddContext.with(ddBaggage).attach() | ||
| otelBaggage = Baggage.current() | ||
| otelBaggageFromContext = Baggage.fromContext(Context.current()) | ||
| otelBaggageFromContextOrNull = Baggage.fromContextOrNull(Context.current()) | ||
|
|
||
| then: "Datadog baggage must be current" | ||
| otelBaggage != null | ||
| otelBaggage.size() == 3 | ||
| otelBaggage.getEntryValue("foo") == "value_to_be_replaced" | ||
| otelBaggage.getEntryValue("FOO") == "UNTOUCHED" | ||
| otelBaggage.getEntryValue("remove_me_key") == "remove_me_value" | ||
| otelBaggage.getEntryValue("foo") == "dd_value_to_be_replaced" | ||
| otelBaggage.getEntryValue("FOO") == "DD_UNTOUCHED" | ||
| otelBaggage.getEntryValue("remove_me_key") == "dd_remove_me_value" | ||
| otelBaggage.asMap() == otelBaggageFromContext.asMap() | ||
| otelBaggage.asMap() == otelBaggageFromContextOrNull.asMap() | ||
|
|
||
| when: | ||
| def builder = otelBaggage.toBuilder() | ||
| builder.put("new_foo", "new_value") | ||
| builder.put("foo", "overwrite_value") | ||
| builder.put("new_foo", "otel_new_value") | ||
| builder.put("foo", "otel_overwrite_value") | ||
| builder.remove("remove_me_key") | ||
| def otelScope = builder.build().makeCurrent() | ||
| otelScope = builder.build().makeCurrent() | ||
| otelBaggage = Baggage.current() | ||
| otelBaggageFromContext = Baggage.fromContext(Context.current()) | ||
| otelBaggageFromContextOrNull = Baggage.fromContextOrNull(Context.current()) | ||
|
|
||
| then: "baggage must contain OTel changes" | ||
| otelBaggage != null | ||
| otelBaggage.size() == 3 | ||
| otelBaggage.getEntryValue("foo") == "overwrite_value" | ||
| otelBaggage.getEntryValue("FOO") == "UNTOUCHED" | ||
| otelBaggage.getEntryValue("new_foo") == "new_value" | ||
| otelBaggage.getEntryValue("foo") == "otel_overwrite_value" | ||
| otelBaggage.getEntryValue("FOO") == "DD_UNTOUCHED" | ||
| otelBaggage.getEntryValue("new_foo") == "otel_new_value" | ||
| otelBaggage.asMap() == otelBaggageFromContext.asMap() | ||
| otelBaggage.asMap() == otelBaggageFromContextOrNull.asMap() | ||
|
|
||
|
|
||
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.
🎯 suggestion: That might be an over optimization but (follow me here), what about implementing a custom Map implementation that projects the
Map<String, BaggageEntry>returned by (OTel)Baggage.asMap()into aMap<String, String>?We can then use (DD)
Baggage.create(Map)from it, meaning having zero collection allocation as OTel uses theReadOnlyArrayMapprojection and we can have ours to translate value to their string counter parts.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.
One hiccup is that projection would be read-only - and our internal Baggage container has mutable operations (
addItem/removeItem) that assume a mutable map.So I'd prefer to do that as part of a refactoring to optimize the internal Baggage API, ideally making it inherently immutable once created/built.