Switch .NET tracer to injecting both base64 & binary headers #6257
Conversation
This file contains 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
Datadog ReportBranch report: ❌ 1 Failed (0 Known Flaky), 451220 Passed, 2732 Skipped, 18h 26m 13.73s Total Time ❌ Failed Tests (1)
|
lucaspimentel
approved these changes
Nov 7, 2024
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing the following branches/commits: Execution-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 shown 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). gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6257) - mean (72ms) : 64, 80
. : milestone, 72,
master - mean (71ms) : 65, 78
. : milestone, 71,
section CallTarget+Inlining+NGEN
This PR (6257) - mean (981ms) : 962, 1000
. : milestone, 981,
master - mean (981ms) : 960, 1003
. : milestone, 981,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6257) - mean (108ms) : 105, 110
. : milestone, 108,
master - mean (108ms) : 106, 110
. : milestone, 108,
section CallTarget+Inlining+NGEN
This PR (6257) - mean (680ms) : 660, 699
. : milestone, 680,
master - mean (683ms) : 665, 702
. : milestone, 683,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6257) - mean (92ms) : 89, 95
. : milestone, 92,
master - mean (92ms) : 89, 95
. : milestone, 92,
section CallTarget+Inlining+NGEN
This PR (6257) - mean (632ms) : 616, 649
. : milestone, 632,
master - mean (638ms) : 623, 653
. : milestone, 638,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6257) - mean (191ms) : 185, 196
. : milestone, 191,
master - mean (190ms) : 186, 194
. : milestone, 190,
section CallTarget+Inlining+NGEN
This PR (6257) - mean (1,103ms) : 1072, 1134
. : milestone, 1103,
master - mean (1,094ms) : 1072, 1115
. : milestone, 1094,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6257) - mean (276ms) : 272, 281
. : milestone, 276,
master - mean (276ms) : 272, 279
. : milestone, 276,
section CallTarget+Inlining+NGEN
This PR (6257) - mean (877ms) : 849, 905
. : milestone, 877,
master - mean (875ms) : 850, 900
. : milestone, 875,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6257) - mean (265ms) : 262, 269
. : milestone, 265,
master - mean (264ms) : 260, 268
. : milestone, 264,
section CallTarget+Inlining+NGEN
This PR (6257) - mean (855ms) : 823, 887
. : milestone, 855,
master - mean (854ms) : 825, 884
. : milestone, 854,
|
Throughput/Crank Report ⚡Throughput results for AspNetCoreSimpleController comparing the following branches/commits: Cases where throughput results for the PR are worse than latest master (5% drop or greater), results are shown in red. Note that these results are based on a single point-in-time result for each branch. For full results, see one of the many, many dashboards! gantt
title Throughput Linux x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (6257) (11.168M) : 0, 11167674
master (11.247M) : 0, 11246955
benchmarks/2.9.0 (11.033M) : 0, 11032866
section Automatic
This PR (6257) (7.257M) : 0, 7256823
master (7.407M) : 0, 7406927
benchmarks/2.9.0 (7.786M) : 0, 7785853
section Trace stats
master (7.695M) : 0, 7695476
section Manual
master (11.240M) : 0, 11240426
section Manual + Automatic
This PR (6257) (6.700M) : 0, 6700131
master (6.871M) : 0, 6870622
section DD_TRACE_ENABLED=0
master (10.206M) : 0, 10206195
gantt
title Throughput Linux arm64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (6257) (9.559M) : 0, 9558760
master (9.563M) : 0, 9563014
benchmarks/2.9.0 (9.495M) : 0, 9494821
section Automatic
This PR (6257) (6.347M) : 0, 6346842
master (6.412M) : 0, 6411572
section Trace stats
master (6.703M) : 0, 6703390
section Manual
master (9.369M) : 0, 9369266
section Manual + Automatic
This PR (6257) (5.939M) : 0, 5939281
master (5.978M) : 0, 5977872
section DD_TRACE_ENABLED=0
master (8.996M) : 0, 8996087
gantt
title Throughput Windows x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (6257) (9.004M) : 0, 9004425
benchmarks/2.9.0 (10.020M) : 0, 10019592
section Automatic
This PR (6257) (6.012M) : 0, 6012109
benchmarks/2.9.0 (7.255M) : 0, 7255257
section Manual + Automatic
This PR (6257) (5.434M) : 0, 5433882
|
Benchmarks Report for tracer 🐌Benchmarks for #6257 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored. Benchmark detailsBenchmarks.Trace.ActivityBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SpanBenchmark - Slower
|
| Benchmark | diff/base | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.SpanBenchmark.StartFinishScope‑net6.0 | 1.195 | 477.52 | 570.43 | |
| Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net6.0 | 1.170 | 399.48 | 467.49 |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | StartFinishSpan |
net6.0 | 399ns | 0.725ns | 2.81ns | 0.00807 | 0 | 0 | 576 B |
| master | StartFinishSpan |
netcoreapp3.1 | 590ns | 1.42ns | 5.51ns | 0.00775 | 0 | 0 | 576 B |
| master | StartFinishSpan |
net472 | 659ns | 0.643ns | 2.32ns | 0.0916 | 0 | 0 | 578 B |
| master | StartFinishScope |
net6.0 | 478ns | 0.619ns | 2.4ns | 0.00968 | 0 | 0 | 696 B |
| master | StartFinishScope |
netcoreapp3.1 | 705ns | 1.06ns | 4.1ns | 0.0095 | 0 | 0 | 696 B |
| master | StartFinishScope |
net472 | 844ns | 1.45ns | 5.62ns | 0.105 | 0 | 0 | 658 B |
| #6257 | StartFinishSpan |
net6.0 | 467ns | 0.544ns | 2.11ns | 0.00815 | 0 | 0 | 576 B |
| #6257 | StartFinishSpan |
netcoreapp3.1 | 561ns | 2.93ns | 14.9ns | 0.00781 | 0 | 0 | 576 B |
| #6257 | StartFinishSpan |
net472 | 669ns | 1.35ns | 5.23ns | 0.0915 | 0 | 0 | 578 B |
| #6257 | StartFinishScope |
net6.0 | 570ns | 0.721ns | 2.79ns | 0.0099 | 0 | 0 | 696 B |
| #6257 | StartFinishScope |
netcoreapp3.1 | 707ns | 3.61ns | 16.5ns | 0.00951 | 0 | 0 | 696 B |
| #6257 | StartFinishScope |
net472 | 877ns | 1.47ns | 5.71ns | 0.104 | 0 | 0 | 658 B |
Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | RunOnMethodBegin |
net6.0 | 656ns | 0.758ns | 2.94ns | 0.00974 | 0 | 0 | 696 B |
| master | RunOnMethodBegin |
netcoreapp3.1 | 903ns | 1.41ns | 5.45ns | 0.00941 | 0 | 0 | 696 B |
| master | RunOnMethodBegin |
net472 | 1.06μs | 1.46ns | 5.66ns | 0.105 | 0 | 0 | 658 B |
| #6257 | RunOnMethodBegin |
net6.0 | 670ns | 1.67ns | 6.48ns | 0.0097 | 0 | 0 | 696 B |
| #6257 | RunOnMethodBegin |
netcoreapp3.1 | 992ns | 1.93ns | 7.47ns | 0.00901 | 0 | 0 | 696 B |
| #6257 | RunOnMethodBegin |
net472 | 1.17μs | 2.8ns | 10.9ns | 0.105 | 0 | 0 | 658 B |
bouwkast
approved these changes
Nov 21, 2024
andrewlock
requested changes
Nov 21, 2024
| { | ||
| var encodedBytes = PathwayContextEncoder.Encode(context.SpanContext.PathwayContext.Value); | ||
| var base64EncodedContext = Convert.ToBase64String(encodedBytes); | ||
| sb.AppendFormat("\"{0}\":\"{1}\",", DataStreamsPropagationHeaders.PropagationKeyBase64, base64EncodedContext); |
There was a problem hiding this comment.
nit: don't use AppendFormat it's much less performant (we can use $"" for the first one because it's embedding a constant so gets handled at compile time)
Suggested change
| sb.AppendFormat("\"{0}\":\"{1}\",", DataStreamsPropagationHeaders.PropagationKeyBase64, base64EncodedContext); | |
| sb.Append($"\"{DataStreamsPropagationHeaders.PropagationKeyBase64}\":\""); | |
| .Append(base64EncodedContext) | |
| .Append('"'); |
There was a problem hiding this comment.
makes sense, replaced.
| var base64EncodedContext = Convert.ToBase64String(encodedBytes); | ||
| sb.AppendFormat("\"{0}\":\"{1}\",", DataStreamsPropagationHeaders.PropagationKeyBase64, base64EncodedContext); | ||
|
|
||
| if (Tracer.Instance.Settings.IsDataStreamsLegacyHeadersEnabled) |
There was a problem hiding this comment.
I feel like this has been said elsewhere, but I think we should def try to pass the tracer in from externally, it's not great to have static access to a tracer instance here (apart from anything else, it's going to make your unit tests impossibly flaky 🙁 )
| if (Tracer.Instance.Settings.IsDataStreamsLegacyHeadersEnabled) | ||
| { | ||
| // Both PropagationKeyBase64 and PropagationKey use the Base64 encoded context | ||
| sb.AppendFormat("\"{0}\":\"{1}\",", DataStreamsPropagationHeaders.PropagationKey, base64EncodedContext); |
There was a problem hiding this comment.
nit: don't use AppendFormat
Suggested change
| sb.AppendFormat("\"{0}\":\"{1}\",", DataStreamsPropagationHeaders.PropagationKey, base64EncodedContext); | |
| ```suggestion | |
| sb.Append($"\"{DataStreamsPropagationHeaders.PropagationKey}\":\""); | |
| .Append(base64EncodedContext) | |
| .Append('"'); |
There was a problem hiding this comment.
makes sense, replaced.
tracer/test/Datadog.Trace.Tests/DataStreamsMonitoring/DataStreamsContextPropagatorTests.cs
Outdated
Show resolved
Hide resolved
## Summary of changes - Fixes the `verify_source_generated_changes_are_persisted.yml` workflow - Adds the missing changes ## Reason for change This workflow was broken when we stopped using a source generator to generate the call target integrations, so it was no longer flagging cases where the call target files have _not_ been correctly updated and need to be rebuilt using Nuke ## Implementation details Add the additional paths to include in the diff comparison ## Test coverage Did a manual test locally. Also committing this in 2 parts, to confirm it flags the currently broken master. ## Other details This missing file updates should have been added in #6397 but were missed because this was broken
## Summary of changes Pin the language version to C#11 (i.e. .NET 7) ## Reason for change Without this, Rider/VS prompts you to use C#12/13 constructs, which then cause the build to fail. ## Implementation details Just pin the version ## Test coverage It works
…c tool (#6273) outputs `configurationFileName` in TryLoadJsonConfigurationFile and we print it ## Summary of changes add the directory where we look for `datadog.json` in the tracer logs and in the diagnostic tool ## Reason for change https://datadoghq.atlassian.net/browse/APMLP-297 Customer confusion over why their configuration isn't picked up. ## Implementation details Log where the directory we check the json for is. ## Test coverage Need to create a test to check this programmatically in the future, but here's the results running it manually on IIS/Process (IIS check calls the process check) datadog.json read:  datadog.json not read:  ## Other details <!-- Fixes #{issue} --> <!--⚠️ Note: where possible, please obtain 2 approvals prior to merging. Unless CODEOWNERS specifies otherwise, for external teams it is typically best to have one review from a team member, and one review from apm-dotnet. Trivial changes do not require 2 reviews. -->
…utableDirectLogSubmissionSettings` (#6400) ## Summary of changes Merge `DirectLogSubmissionSettings` with `ImmutableDirectLogSubmissionSettings` ## Reason for change There was never really a good reason for having these as separate types. It was primarily to make testing a little easier and to mirror the `TracerSettings`/`ImmutableTracerSettings` dichotomy, but as that's going away, this is just unnecessary complexity ## Implementation details - Moved the additional logic that was previously inside `ImmutableDirectLogSubmissionSettings` into `DirectLogSubmissionSettings` - Renamed the `DirectLogSubmissionSettings` properties to match the "Immutable" version (and remove the unnecessary prefix) - Replace all usages of `Immutable*` with `DirectLogSubmissionSettings` - Move `Immutable*Tests` into appropriate file and tweak - Replace mutations with initialization (using `IConfigurationSource`) ## Test coverage Essentially the same. I removed/tweaked some tests that are no longer relevant ## Other details Part of stack - #6370 - #6376 - #6385 - #6386 - #6397 - #6399 - #6400 👈 This PR - #6405 - #6408 - #6415
Updates the package versions for integration tests. Co-authored-by: andrewlock <andrewlock@users.noreply.github.com>
## Summary of changes EFD changes the number of execution of a tests depending on the duration of that test. With this PR we use the same number of execution no matter the duration of the execution. ## Reason for change Our CI is unstable. Ticket: SDTEST-1313
## Summary of changes This PR adds the mechanism to create a memory dump not only on the process id but also the children processes ids of a samples app. ## Reason for change This is useful for a `dotnet test` scenario where the CLI spawn multiple processes (testhost, datacollectorhost) Ticket: SDTEST-1309 --------- Co-authored-by: Andrew Lock <andrew.lock@datadoghq.com>
Updates the package versions for integration tests. Co-authored-by: dependabot[bot] <dependabot[bot]@users.noreply.github.com>
## Summary of changes This PR contains the implementation of the RASP command injection vulnerability. Previously, the shell injection vulnerability was implemented, which is similar but only occurs the flag UseShellExecute is set to false in method process.start(). The definition of this new vulnerability can be found here: https://docs.google.com/document/d/1DDWy3frMXDTAbk-BfnZ1FdRwuPx6Pl7AWyR4zjqRFZw/edit?tab=t.0#heading=h.giijrtyn1fdx ## Reason for change It's required for RASP. ## Implementation details ## Test coverage ## Other details <!-- Fixes #{issue} --> <!--⚠️ Note: where possible, please obtain 2 approvals prior to merging. Unless CODEOWNERS specifies otherwise, for external teams it is typically best to have one review from a team member, and one review from apm-dotnet. Trivial changes do not require 2 reviews. --> --------- Co-authored-by: Anna <anna.yafi@datadoghq.com> Co-authored-by: Andrew Lock <andrew.lock@datadoghq.com>
…tegrationSettings` (#6405) ## Summary of changes Merge `IntegrationSettings` with `ImmutableIntegrationSettings` ## Reason for change This stack of PRs is about doing one-shot configuration instead of mutation. We never mutate these in the tracer after creation, so there's no need for the separate types. ## Implementation details - Moved additional logic (handling `DisabledNames` that was previously inside `ImmutableIntegrationSettings` into `IntegrationSettings` - Replace all usages of `Immutable*` with `IntegrationSettings` - Move `Immutable*Tests` into appropriate file and tweak - Replace mutations with initialization - Reorder initialization of `DisabledIntegrationNames` in `TracerSettings` so that it can be used in the `IntegrationSettings` constructor ## Test coverage All covered by existing details ## Other details Part of Stack - #6370 - #6376 - #6385 - #6386 - #6397 - #6399 - #6400 - #6405 👈 This PR - #6408 - #6415
## Summary of changes Rename the Nuke target to more closely match the file name ## Reason for change It's not clear that the trimming.xml file is created by the "RootDescriptors" task ## Implementation details Rename the target
…terSettings` (#6408) ## Summary of changes Merge `ExporterSettings` with `ImmutableExporterSettings` ## Reason for change This stack of PRs is about doing one-shot configuration instead of mutation. We never mutate these in the tracer after creation, so there's no need for the separate types. ## Implementation details - Made the properties in `ExporterSettings` get-only. This required quite a lot of work because we were doing a lot of mutating of the settings in the "helper" functions. - I only _lightly_ refactored those methods (as much as possible) to avoid setting the properties in the functions and instead returning the details to set later. - These are prime candidates for some _much_ heavier refactoring later, but I didn't want to get bogged down with that in this PR - Replace all usages of `Immutable*` with `ExporterSettings` - Replace usages of `AgentUriInternal` with `AgentUri` - Move `Immutable*Tests` into appropriate file and tweak - Replace mutations with initialization ## Test coverage All covered by existing details ## Other details Part of Stack - #6370 - #6376 - #6385 - #6386 - #6397 - #6399 - #6400 - #6405 - #6408 👈 This PR - #6415
…n LiveDebugger isn't fully initialized (#6092) ## Summary of changes ProbeProcessor can be inialized from various places and it do a low of work. If the LiveDebugger isn't fully initialized, it useless to create it. This PR verify this.
…ettings` (#6415) Merge `TracerSettings` with `ImmutableTracerSettings` This stack of PRs is about doing one-shot configuration instead of mutation. We never mutate these in the tracer after creation, so there's no need for the separate types. - Make the properties in `TracerSettings` get-only. - Make the collections in `TracerSettings` readonly. - Move logic that used to be in the constructor of `ImmutableTracerSettings` into `TracerSettings` - e.g. Service/Version/Env were being changed based on DD_TAGS values. Moved that to TracerSettings and (importantly) added missing telemetry recording of these values. - Added missing recording of _effective_ `DisabledInstegrations` - Moving this logic caused some _tests_ to be broken (checking default values). Updated the expected values of those tests in a single - Replace all usages of `ImmutableTracerSettings` with `TracerSettings` - Move `ITracer` to Datadog.Trace.Manual - It's only used there, and references the manual-version of `ImmutableTracerSettings` which we _want_ to keep. - Move `Immutable*Tests` into appropriate file and tweak - Replace mutations with initialization in tests All covered by existing tests (I hope) 🤞 There's still a _lot_ of scope to improve this Part of Stack - #6370 - #6376 - #6385 - #6386 - #6397 - #6399 - #6400 - #6405 - #6408 - #6415 👈 This PR
## Summary of changes Fix signup tags that weren't correct. Add login tags on failure and other internal tagss as per [RFC](https://docs.google.com/document/d/1RT38U6dTTcB-8muiYV4-aVDCsT_XrliyakjtAPyjUpw/edit?tab=t.0#heading=h.dy0zssue7nq2) ## Reason for change ## Implementation details ## Test coverage ## Other details <!-- Fixes #{issue} --> <!--⚠️ Note: where possible, please obtain 2 approvals prior to merging. Unless CODEOWNERS specifies otherwise, for external teams it is typically best to have one review from a team member, and one review from apm-dotnet. Trivial changes do not require 2 reviews. -->
## Summary of changes Adds support for latest major of GraphQL.NET ## Reason for change We want to support the latest versions (we're quite behind on this one) ## Implementation details Bumped the version numbers as required ## Test coverage Updated testing to test 8.x ## Other details Fixes: https://datadoghq.atlassian.net/browse/AIDM-506
## Summary of changes Adds support for latest `Aerospike.Client` package ## Reason for change We want to support the latest versions of libraries ## Implementation details Mostly just bump the numbers, needed to add a new duck type to handle some refactoring changes in the library ## Test coverage Extended the tests to cover the new version ## Other details Fixes https://datadoghq.atlassian.net/browse/AIDM-507
## Summary of changes Add tests and corrections to support for various database platforms. ## Reason for change Some bugs were reported relating to database tainting not working correctly for some dbs. ## Test coverage Adds new integration tests dependent on docker db instances.
## Summary of changes
Move native definitions from a permanent list to a temporary one that
gets created and freed on demand
## Reason for change
Some crashes were reported in shutdown and it points to definition lists
cleanup
## Implementation details
Instead of storing the lists as static fields, they get created in the
call to GetCallSites and GetCallTargets and destroyed right after
they've been read by the profiler
## Test coverage
## Other details
<!-- Fixes #{issue} -->
<!-- ⚠️ Note: where possible, please obtain 2 approvals prior to
merging. Unless CODEOWNERS specifies otherwise, for external teams it is
typically best to have one review from a team member, and one review
from apm-dotnet. Trivial changes do not require 2 reviews. -->
## Summary of changes The databases `System.Data.SQLite` and `Microsoft.Data.Sqlite` were swapped out in the tests. ## Reason for change Tests were failling on master with `netcoreapp3.0`: There was a specific check to skip the test with Microsoft.Data.Sqlite on Alpine for `netcoreapp3.0`, this was failing on master.
|
replaced by #6448 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary of changes
Reason for change
Data Streams previously used binary encoding in Kafka headers. This was causing issues in cross language communication because of the difference in negative byte handling. That’s why we switched to base64 encoding.
Today, .NET is the only remaining tracer using binary encoding for Kafka, SQS & RabbitMQ.
This is causing 3 issues:
Also, byte headers are causing a crash of the .NET application (not reproduced yet).
Implementation details
DD_DATA_STREAMS_LEGACY_HEADERS(default: true) is enabled for backward compatibility.Test coverage
I've added tests for the following cases:
Inject_WhenLegacyHeadersDisabled_DoesNotIncludeBinaryHeader
Extract_WhenBothHeadersPresent_PrefersBase64Header
InjectedHeaders_HaveCorrectFormat
InjectHeaders_WhenLegacyHeadersDisabled_DoesNotIncludeLegacyHeader
Inject_WhenLegacyHeadersEnabled_IncludesBothHeaders
Extract_WhenBase64HeaderIsMalformed_ReturnsFallbackToBinary
Other details