-
Notifications
You must be signed in to change notification settings - Fork 147
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
Fix W3CTraceContextPropagator for non-standard array types #6654
Conversation
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.
Nice catch 👍
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 (6654) - mean (69ms) : 66, 72
. : milestone, 69,
master - mean (70ms) : 64, 76
. : milestone, 70,
section CallTarget+Inlining+NGEN
This PR (6654) - mean (990ms) : 967, 1013
. : milestone, 990,
master - mean (994ms) : 968, 1019
. : milestone, 994,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6654) - mean (102ms) : 100, 105
. : milestone, 102,
master - mean (102ms) : 100, 104
. : milestone, 102,
section CallTarget+Inlining+NGEN
This PR (6654) - mean (671ms) : 656, 686
. : milestone, 671,
master - mean (675ms) : 662, 688
. : milestone, 675,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6654) - mean (89ms) : 87, 91
. : milestone, 89,
master - mean (90ms) : 87, 92
. : milestone, 90,
section CallTarget+Inlining+NGEN
This PR (6654) - mean (626ms) : 610, 641
. : milestone, 626,
master - mean (635ms) : 608, 662
. : milestone, 635,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6654) - mean (190ms) : 185, 195
. : milestone, 190,
master - mean (190ms) : 186, 195
. : milestone, 190,
section CallTarget+Inlining+NGEN
This PR (6654) - mean (1,101ms) : 1076, 1127
. : milestone, 1101,
master - mean (1,104ms) : 1076, 1131
. : milestone, 1104,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6654) - mean (270ms) : 266, 275
. : milestone, 270,
master - mean (270ms) : 266, 273
. : milestone, 270,
section CallTarget+Inlining+NGEN
This PR (6654) - mean (861ms) : 834, 888
. : milestone, 861,
master - mean (864ms) : 834, 895
. : milestone, 864,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6654) - mean (262ms) : 258, 267
. : milestone, 262,
master - mean (261ms) : 257, 266
. : milestone, 261,
section CallTarget+Inlining+NGEN
This PR (6654) - mean (846ms) : 804, 887
. : milestone, 846,
master - mean (845ms) : 814, 877
. : milestone, 845,
|
Datadog ReportBranch report: ✅ 0 Failed, 241699 Passed, 2379 Skipped, 18h 25m 46.66s Total Time |
Benchmarks Report for tracer 🐌Benchmarks for #6654 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‑netcoreapp3.1 | 1.156 | 670.15 | 775.02 | |
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑netcoreapp3.1 | 1.135 | 547.10 | 620.82 |
Benchmark | base/diff | Base Median (ns) | Diff Median (ns) | Modality |
---|---|---|---|---|
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑net6.0 | 1.189 | 575.79 | 484.27 |
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | StartFinishSpan |
net6.0 | 482ns | 0.868ns | 3.36ns | 0.00798 | 0 | 0 | 576 B |
master | StartFinishSpan |
netcoreapp3.1 | 547ns | 0.762ns | 2.85ns | 0.00786 | 0 | 0 | 576 B |
master | StartFinishSpan |
net472 | 630ns | 1.17ns | 4.54ns | 0.0918 | 0 | 0 | 578 B |
master | StartFinishScope |
net6.0 | 575ns | 0.882ns | 3.42ns | 0.00966 | 0 | 0 | 696 B |
master | StartFinishScope |
netcoreapp3.1 | 669ns | 0.458ns | 1.71ns | 0.0093 | 0 | 0 | 696 B |
master | StartFinishScope |
net472 | 805ns | 1.48ns | 5.75ns | 0.105 | 0 | 0 | 658 B |
#6654 | StartFinishSpan |
net6.0 | 461ns | 0.508ns | 1.97ns | 0.00804 | 0 | 0 | 576 B |
#6654 | StartFinishSpan |
netcoreapp3.1 | 620ns | 0.631ns | 2.36ns | 0.00771 | 0 | 0 | 576 B |
#6654 | StartFinishSpan |
net472 | 577ns | 0.822ns | 3.07ns | 0.0916 | 0 | 0 | 578 B |
#6654 | StartFinishScope |
net6.0 | 483ns | 0.886ns | 3.43ns | 0.0099 | 0 | 0 | 696 B |
#6654 | StartFinishScope |
netcoreapp3.1 | 773ns | 1.87ns | 7.23ns | 0.00952 | 0 | 0 | 696 B |
#6654 | StartFinishScope |
net472 | 820ns | 1.35ns | 5.23ns | 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 | 650ns | 0.737ns | 2.85ns | 0.00982 | 0 | 0 | 696 B |
master | RunOnMethodBegin |
netcoreapp3.1 | 984ns | 1.68ns | 6.49ns | 0.00924 | 0 | 0 | 696 B |
master | RunOnMethodBegin |
net472 | 1.01μs | 2.88ns | 11.2ns | 0.105 | 0 | 0 | 658 B |
#6654 | RunOnMethodBegin |
net6.0 | 672ns | 0.733ns | 2.84ns | 0.00966 | 0 | 0 | 696 B |
#6654 | RunOnMethodBegin |
netcoreapp3.1 | 919ns | 1.09ns | 4.22ns | 0.00919 | 0 | 0 | 696 B |
#6654 | RunOnMethodBegin |
net472 | 1.05μs | 1.3ns | 5.05ns | 0.104 | 0 | 0 | 658 B |
} | ||
|
||
// there were no items | ||
return false; |
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.
It took me a while to figure out the underlying bug. In short, this last line should have been
return hasValue;
instead of
return false;
otherwise there's never any way for this method to return true
!
Summary of changes
The
W3CTraceContextPropagator
would check whether there was a singletraceparent
header value to determine whether or not it should extract W3C headers further. For non-standard array types it would always returnfalse
meaning that headers wouldn't be extracted. This pull request fixes that so that non-standard array types will returntrue
to allow extraction to happen.Reason for change
If we have valid headers we should extract them.
Implementation details
Swapped to
GetEnumerator
as it seems more readable than theforeach
route.If we have one value return
true
instead offalse
.Swapped the
TryGetSingle
andTryGetSingleRare
tointernal
to add direct unit tests to them.Test coverage
TryGetSingle
andTryGetSingleRare
testing fundamental behavior.Extract
to test possible behavior we could see.Other details
Fixes #6596