[Tracing] Add interoperability between the Datadog Baggage API and the OpenTelemetry Baggage API#7921
Conversation
5d57d03 to
cbfdd00
Compare
c9cbcf3 to
b258b5b
Compare
BenchmarksBenchmark execution time: 2026-03-16 16:16:35 Comparing candidate commit f8b5443 in PR branch Found 10 performance improvements and 9 performance regressions! Performance is the same for 157 metrics, 16 unstable metrics. scenario:Benchmarks.Trace.AgentWriterBenchmark.WriteAndFlushEnrichedTraces net6.0
scenario:Benchmarks.Trace.AgentWriterBenchmark.WriteAndFlushEnrichedTraces netcoreapp3.1
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.AllCycleMoreComplexBody net6.0
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.AllCycleSimpleBody net472
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.AllCycleSimpleBody netcoreapp3.1
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorMoreComplexBody netcoreapp3.1
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody netcoreapp3.1
scenario:Benchmarks.Trace.AspNetCoreBenchmark.SendRequest net6.0
scenario:Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSlice net6.0
scenario:Benchmarks.Trace.GraphQLBenchmark.ExecuteAsync net6.0
scenario:Benchmarks.Trace.ILoggerBenchmark.EnrichedLog net472
scenario:Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark netcoreapp3.1
scenario:Benchmarks.Trace.RedisBenchmark.SendReceive net472
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishScope net6.0
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishScope netcoreapp3.1
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishSpan netcoreapp3.1
scenario:Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin netcoreapp3.1
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (7921) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-time benchmarks measure the whole time it takes to execute a program, and are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are highlighted in **red**. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). Duration chartsFakeDbCommand (.NET Framework 4.8)gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7921) - mean (77ms) : 74, 79
master - mean (76ms) : 73, 78
section Bailout
This PR (7921) - mean (82ms) : 80, 84
master - mean (79ms) : 77, 82
section CallTarget+Inlining+NGEN
This PR (7921) - mean (1,114ms) : 1052, 1176
master - mean (1,103ms) : 1057, 1149
FakeDbCommand (.NET Core 3.1)gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7921) - mean (120ms) : 116, 124
master - mean (118ms) : 115, 122
section Bailout
This PR (7921) - mean (121ms) : 119, 124
master - mean (120ms) : 116, 123
section CallTarget+Inlining+NGEN
This PR (7921) - mean (772ms) : 744, 799
master - mean (763ms) : 741, 785
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7921) - mean (105ms) : 103, 108
master - mean (104ms) : 100, 107
section Bailout
This PR (7921) - mean (107ms) : 104, 110
master - mean (105ms) : 102, 109
section CallTarget+Inlining+NGEN
This PR (7921) - mean (762ms) : 718, 806
master - mean (748ms) : 709, 786
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7921) - mean (105ms) : 102, 108
master - mean (103ms) : 100, 106
section Bailout
This PR (7921) - mean (106ms) : 104, 108
master - mean (104ms) : 103, 106
section CallTarget+Inlining+NGEN
This PR (7921) - mean (701ms) : 667, 734
master - mean (693ms) : 662, 723
HttpMessageHandler (.NET Framework 4.8)gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7921) - mean (195ms) : 192, 199
master - mean (194ms) : 190, 199
section Bailout
This PR (7921) - mean (199ms) : 195, 203
master - mean (199ms) : 195, 202
section CallTarget+Inlining+NGEN
This PR (7921) - mean (1,167ms) : 1106, 1227
master - mean (1,157ms) : 1102, 1213
HttpMessageHandler (.NET Core 3.1)gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7921) - mean (279ms) : 273, 285
master - mean (280ms) : 273, 287
section Bailout
This PR (7921) - mean (278ms) : 274, 283
master - mean (280ms) : 274, 286
section CallTarget+Inlining+NGEN
This PR (7921) - mean (905ms) : 865, 945
master - mean (907ms) : 881, 933
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7921) - mean (272ms) : 268, 277
master - mean (272ms) : 266, 277
section Bailout
This PR (7921) - mean (273ms) : 269, 278
master - mean (272ms) : 268, 277
section CallTarget+Inlining+NGEN
This PR (7921) - mean (939ms) : 905, 973
master - mean (948ms) : 908, 987
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7921) - mean (271ms) : 265, 276
master - mean (271ms) : 266, 277
section Bailout
This PR (7921) - mean (270ms) : 266, 274
master - mean (273ms) : 267, 280
section CallTarget+Inlining+NGEN
This PR (7921) - mean (844ms) : 814, 873
master - mean (853ms) : 811, 896
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
a2c8de8 to
0633f47
Compare
17bb9a3 to
60625e2
Compare
| SetInstrumentationVerification(); | ||
|
|
||
| await Fixture.TryStartApp(this); | ||
| string[] baggageItems = [ |
There was a problem hiding this comment.
Note for reviewers: All of the test cases start with this baggage as the original contents. However, the OpenTelemetry Baggage API currently treats keys in a case-insensitive manner, so the final baggage output will be missing one of the baggage items below whose keys only differ by case. This is not aligned with our Datadog implementation and more importantly with the OpenTelemetry specification, so I'll submit a PR upstream to fix that. Once that change is shipped, we can then update the OpenTelemetry.Api package and update our snapshots.
There was a problem hiding this comment.
I've posted a PR upstream to get the OTel API to use case-sensitive baggage names: open-telemetry/opentelemetry-dotnet#6931
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60625e2113
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
...dog.Trace/ClrProfiler/AutoInstrumentation/OpenTelemetry/OTelBaggage_SetBaggageIntegration.cs
Show resolved
Hide resolved
…t updates to OpenTelemetry.Baggage.Current are mirrored to Datadog.Trace.Baggage.Current, and vice-versa (though they will only be observed when retrieving the latest OpenTelemetry.Baggage.Current value).
…nTelemetry.Baggage API's without passing a OpenTelemetry.Baggage argument, the underlying OpenTelemetry.Baggage.Current is used as the source for the modifications. This change makes sure that the underlying OpenTelemetry.Baggage.Current store is updated with the contents of Datadog.Trace.Baggage.Current before the modifications are made.
- OpenTelemetry.Baggage.GetBaggage(Baggage) - OpenTelemetry.Baggage.GetBaggage(string, Baggage) - OpenTelemetry.Baggage.GetEnumerator(Baggage)
…ntegration. The test cases hit every method and ensures that we correctly update OpenTelemetry.Baggage.Current
…e OpenTelemetry.Baggage.Current when the user has provided the default Baggage struct. Otherwise, the Baggage argument provided by the user will be used as the source for baggage modification so there's no reason to update OpenTelemetry.Baggage.Current.
60625e2 to
61edb29
Compare
andrewlock
left a comment
There was a problem hiding this comment.
Haven't finished yet, but added some comments/questions to get you started 🙂
tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/OpenTelemetry/IApiBaggage.cs
Show resolved
Hide resolved
|
|
||
| internal interface IBaggageHolder | ||
| { | ||
| [DuckField(Name = "Baggage")] |
There was a problem hiding this comment.
OMG the casing inconsistency in this code base 😅
...g.Trace/ClrProfiler/AutoInstrumentation/OpenTelemetry/OTelBaggage_ClearBaggageIntegration.cs
Show resolved
Hide resolved
...g.Trace/ClrProfiler/AutoInstrumentation/OpenTelemetry/OTelBaggage_ClearBaggageIntegration.cs
Outdated
Show resolved
Hide resolved
...g.Trace/ClrProfiler/AutoInstrumentation/OpenTelemetry/OTelBaggage_ClearBaggageIntegration.cs
Show resolved
Hide resolved
...dog.Trace/ClrProfiler/AutoInstrumentation/OpenTelemetry/OTelBaggage_GetCurrentIntegration.cs
Outdated
Show resolved
Hide resolved
...dog.Trace/ClrProfiler/AutoInstrumentation/OpenTelemetry/OTelBaggage_GetCurrentIntegration.cs
Outdated
Show resolved
Hide resolved
....Trace/ClrProfiler/AutoInstrumentation/OpenTelemetry/OTelBaggage_RemoveBaggageIntegration.cs
Outdated
Show resolved
Hide resolved
| // Additional notes: | ||
| // - Since the Datadog Baggage model is mutable (allowing the user to get the Datadog.Trace.Baggage.Current once and continue to mutate that reference), | ||
| // we must clear then add the new baggage items. | ||
| // - The API can be invoked with an arbitrary OpenTelemetry.Baggage object passed via the parameter, so we must replace all baggage items every time |
There was a problem hiding this comment.
To clarify, if the user calls RemoveBaggage("mykey", someBaggage), and passes in a baggage object, that (updated) baggage is set to be the new Baggage.Current value, correct? i.e. all of the SetBaggage and RemoveBaggage() APIs always manipulate the Current baggage, passing in an explicit baggage object just manipulates the "starting" point?
There was a problem hiding this comment.
Yes. Haha.
For the static methods, the baggage update will take place on the baggage object passed in or Baggage.Current if nothing is passed in. Then the resulting baggage is set as the new value of Baggage.Current.
| // Note: When the user sets OpenTelemetry.Baggage.Current, those changes will override the contents of Datadog.Trace.Baggage.Current, | ||
| // so we can always consider Datadog.Trace.Baggage.Current as being up-to-date. | ||
| var baggageHolder = apiBaggage.EnsureBaggageHolder(); | ||
| baggageHolder.Baggage = apiBaggage.Create(Baggage.Current.ToDictionary(kvp => kvp.Key, kvp => kvp.Value)); |
There was a problem hiding this comment.
Given the OTel APIs treat baggage in a case-insensitive manner, what happens if we set the same key here? Are the duplicate keys overwritten, or does it throw?
There was a problem hiding this comment.
No exception, the entry just gets overwritten. But I'm resolving this with open-telemetry/opentelemetry-dotnet#6931
…odEnd updates, since the OpenTelemetry.Baggage.Current only happens when no exceptions are thrown.
…: IApiBaggage' generic type constraint
…nt by utilizing the default value type instance that's passed into the method by our updated bytecode instrumentation.
andrewlock
left a comment
There was a problem hiding this comment.
LGTM in general, just one main comment about tweaking Baggage to have an AsDictionary() method + expose the enumerator, but they're non-blocking optimizations (same for most other comments!)
...dog.Trace/ClrProfiler/AutoInstrumentation/OpenTelemetry/OTelBaggage_GetCurrentIntegration.cs
Outdated
Show resolved
Hide resolved
...dog.Trace/ClrProfiler/AutoInstrumentation/OpenTelemetry/OTelBaggage_SetBaggageIntegration.cs
Show resolved
Hide resolved
...dog.Trace/ClrProfiler/AutoInstrumentation/OpenTelemetry/OTelBaggage_SetCurrentIntegration.cs
Show resolved
Hide resolved
| // Enable OpenTelemetry interop to test that the OTel Baggage API integration works correctly. | ||
| SetEnvironmentVariable("DD_TRACE_OTEL_ENABLED", "true"); |
There was a problem hiding this comment.
We should probably set this in all of our aspnetcore fixtures, regardless, so that we can catch any cases of weird regressions? 🤔 WDYT? We could do it in a separate PR obviously
There was a problem hiding this comment.
Yep sounds good to me!
...pisTests.Single._OTelBaggageApi_path=_otel-baggage_clear-baggage_statusCode=200.verified.txt
Outdated
Show resolved
Hide resolved
..._otel-baggage_set-baggage_foo_case_sensitive_key_overwrite_value_statusCode=200.verified.txt
Show resolved
Hide resolved
…' tag on the server span if there's no baggage. This requires updating the ClearBaggage test since it clears all existing baggage from the current request.
…dating Datadog.Trace.Baggage.Current during the OnMethodEnd handler and only if there was no exception thrown in the Setter.
…ing baggage contents in a thread safe manner, which is used to update OpenTelemetry.Baggage.Current. Also make the GetEnumerator() method public
bouwkast
left a comment
There was a problem hiding this comment.
LGTM I just did a quick view of the auto-instrumentation changes and a glance at the snapshots
Summary of changes
Enable updates to Baggage from either the Datadog Baggage API and the OpenTelemetry Baggage API to be visible to the other.
Relies on #7920 for properly instrumenting the static methods in the
OpenTelemetry.Baggagestruct.Reason for change
This allows users of either Baggage API to make changes to the automatically-propagated Baggage header. Before, only changes made through the Datadog Baggage API would be reflected.
Implementation details
APIs supported
Instruments the following methods in the OpenTelemetry Baggage API:
static OpenTelemetry.Baggage.ClearBaggage(Baggage)static OpenTelemetry.Baggage.Current { get; set; }static OpenTelemetry.Baggage.RemoveBaggage(string, Baggage)static OpenTelemetry.Baggage.SetBaggage(string, string, Baggage)static OpenTelemetry.Baggage.SetBaggage(IEnumerable<KeyValuePair<string, string?>>, Baggage)The following public API's are also supported indirectly by our instrumentation of the
OpenTelemetry.Baggage.Currentgetter:static OpenTelemetry.Baggage.GetBaggage(Baggage)static OpenTelemetry.Baggage.GetBaggage(string, Baggage)static OpenTelemetry.Baggage.GetEnumerator(Baggage)OpenTelemetry Baggage Model
In the OpenTelemetry model, baggage is immutable. This means every time the user wants to update
OpenTelemetry.Baggage.Current, they must either call the setter directly with an new instance ofOpenTelemetry.Baggage, or they must call one of the static APIs.The static APIs are largely intuitive, but it's worth calling two interesting behaviors of the APIs that modify baggage:
OpenTelemetry.Baggageargument is provided (optional), that object will be the source from which the modification will be enacted on. If not, thenOpenTelemetry.Baggage.Currentwill be the source.OpenTelemetry.Baggageis set as the newOpenTelemetry.Baggage.Currentvalue.Interoperability Approach
For this approach,
Datadog.Trace.Baggage.Currentwill be the source of truth for the "current" baggage. For example, our HTTP server instrumentations will first populateDatadog.Trace.Baggage.Currentwith the contents of the incomingbaggageHTTP header, and users of theDatadog.Trace.BaggageAPI can update the mutableDatadog.Trace.Baggage.Current. Then, if the user wants get or setOpenTelemetry.Baggage.Current, that is where our instrumentation must keep it in sync withDatadog.Trace.Baggage.Current.Each time
OpenTelemetry.Baggage.set_Currentis invoked, we must replace the contents ofDatadog.Trace.Baggage.Currententirely by clearing out the existing values and add all of the new KeyValuePairs.Each time
OpenTelemetry.Baggage.get_Currentis invoked, we must create a new (immutable)OpenTelemetry.Baggageusing the contents ofDatadog.Trace.Baggage.Currentand assign it toOpenTelemetry.Baggage.Currentbefore the property is accessed and returned to the API caller.By design, no work is done if only the
Datadog.Trace.BaggageAPIs are used. Also, users are aware of the immutable nature of theOpenTelemetry.BaggageAPIs, so they will be used to doing at least one allocation per baggage modification, but we can certainly improve the overhead of this interop with approaches noted in the 'Other details' section below.Test coverage
Added a new
OtelBaggageControllerto theSamples.AspNetCoreMinimalApisandSamples.AspNetCoreMvc*applications, and added integration test cases (with snapshots) totracer/test/Datadog.Trace.ClrProfiler.IntegrationTests/AspNetCore/AspNetCoreMinimalApisTests.cs.Other details
We can follow up this PR with changes to optimize the sharing of Baggage state between the two API's. In order to do this, we would need to do the following:
Datadog.Trace.Baggageto beDictionary<string,string>Datadog.Trace.Baggageto disallownullvalues in baggage key-value pairs, to align with the OpenTelemetry storage modelOpenTelemetry.Baggagemethods that modify state, take the reference to the internalDictionary<string,string>object and assign it to theDatadog.Trace.Baggage.Currentitems dictionaryMore immediately, we could implement tracking of writes to
Datadog.Trace.Baggage.Currentso we don't have to create a fresh copy of it every timeOpenTelemetry.Baggage.get_Currentis called, but I'd rather separate that out from this PR.