-
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
Move SpanContextPropagator to TracerManager #6511
Conversation
Datadog ReportBranch report: ✅ 0 Failed, 584370 Passed, 3887 Skipped, 35h 46m 20.19s Total Time New Flaky Tests (2)
|
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 (6511) - mean (68ms) : 66, 71
. : milestone, 68,
master - mean (68ms) : 66, 71
. : milestone, 68,
section CallTarget+Inlining+NGEN
This PR (6511) - mean (981ms) : 950, 1012
. : milestone, 981,
master - mean (979ms) : 948, 1010
. : milestone, 979,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6511) - mean (107ms) : 104, 110
. : milestone, 107,
master - mean (107ms) : 105, 109
. : milestone, 107,
section CallTarget+Inlining+NGEN
This PR (6511) - mean (676ms) : 659, 694
. : milestone, 676,
master - mean (678ms) : 664, 693
. : milestone, 678,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6511) - mean (91ms) : 89, 93
. : milestone, 91,
master - mean (91ms) : 89, 93
. : milestone, 91,
section CallTarget+Inlining+NGEN
This PR (6511) - mean (628ms) : 615, 641
. : milestone, 628,
master - mean (635ms) : 620, 649
. : milestone, 635,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6511) - mean (193ms) : 189, 197
. : milestone, 193,
master - mean (194ms) : 190, 198
. : milestone, 194,
section CallTarget+Inlining+NGEN
This PR (6511) - mean (1,099ms) : 1066, 1131
. : milestone, 1099,
master - mean (1,103ms) : 1066, 1140
. : milestone, 1103,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6511) - mean (279ms) : 275, 283
. : milestone, 279,
master - mean (277ms) : 274, 281
. : milestone, 277,
section CallTarget+Inlining+NGEN
This PR (6511) - mean (873ms) : 841, 906
. : milestone, 873,
master - mean (870ms) : 843, 897
. : milestone, 870,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6511) - mean (267ms) : 263, 272
. : milestone, 267,
master - mean (267ms) : 263, 271
. : milestone, 267,
section CallTarget+Inlining+NGEN
This PR (6511) - mean (849ms) : 804, 894
. : milestone, 849,
master - mean (852ms) : 825, 878
. : milestone, 852,
|
Benchmarks Report for appsec 🐌Benchmarks for #6511 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored. Benchmark detailsBenchmarks.Trace.Asm.AppSecBodyBenchmark - Slower
|
Benchmark | diff/base | Base Median (ns) | Diff Median (ns) | Modality |
---|---|---|---|---|
Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody‑netcoreapp3.1 | 1.140 | 191.16 | 217.83 |
Benchmark | base/diff | Base Median (ns) | Diff Median (ns) | Modality |
---|---|---|---|---|
Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody‑net6.0 | 1.157 | 167.39 | 144.67 |
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | AllCycleSimpleBody |
net6.0 | 202μs | 105ns | 392ns | 2.64 | 0 | 0 | 188.76 KB |
master | AllCycleSimpleBody |
netcoreapp3.1 | 308μs | 152ns | 587ns | 2.64 | 0 | 0 | 196.09 KB |
master | AllCycleSimpleBody |
net472 | 268μs | 148ns | 572ns | 35.8 | 2 | 0 | 225.78 KB |
master | AllCycleMoreComplexBody |
net6.0 | 208μs | 95.7ns | 371ns | 2.71 | 0 | 0 | 192.26 KB |
master | AllCycleMoreComplexBody |
netcoreapp3.1 | 317μs | 88.6ns | 332ns | 2.69 | 0 | 0 | 199.51 KB |
master | AllCycleMoreComplexBody |
net472 | 278μs | 182ns | 704ns | 36.3 | 2.08 | 0 | 229.3 KB |
master | ObjectExtractorSimpleBody |
net6.0 | 167ns | 0.119ns | 0.445ns | 0.00398 | 0 | 0 | 280 B |
master | ObjectExtractorSimpleBody |
netcoreapp3.1 | 191ns | 0.0949ns | 0.329ns | 0.00368 | 0 | 0 | 272 B |
master | ObjectExtractorSimpleBody |
net472 | 166ns | 0.242ns | 0.938ns | 0.0446 | 0 | 0 | 281 B |
master | ObjectExtractorMoreComplexBody |
net6.0 | 2.9μs | 1.84ns | 6.88ns | 0.0533 | 0 | 0 | 3.78 KB |
master | ObjectExtractorMoreComplexBody |
netcoreapp3.1 | 3.92μs | 1.86ns | 6.71ns | 0.0492 | 0 | 0 | 3.69 KB |
master | ObjectExtractorMoreComplexBody |
net472 | 3.61μs | 3.29ns | 12.3ns | 0.603 | 0.00543 | 0 | 3.8 KB |
#6511 | AllCycleSimpleBody |
net6.0 | 205μs | 229ns | 885ns | 2.66 | 0 | 0 | 188.76 KB |
#6511 | AllCycleSimpleBody |
netcoreapp3.1 | 313μs | 159ns | 550ns | 2.67 | 0 | 0 | 196.09 KB |
#6511 | AllCycleSimpleBody |
net472 | 273μs | 383ns | 1.48μs | 35.8 | 2.06 | 0 | 225.78 KB |
#6511 | AllCycleMoreComplexBody |
net6.0 | 212μs | 148ns | 555ns | 2.64 | 0 | 0 | 192.26 KB |
#6511 | AllCycleMoreComplexBody |
netcoreapp3.1 | 321μs | 133ns | 497ns | 2.71 | 0 | 0 | 199.51 KB |
#6511 | AllCycleMoreComplexBody |
net472 | 276μs | 176ns | 681ns | 36.4 | 2.07 | 0 | 229.3 KB |
#6511 | ObjectExtractorSimpleBody |
net6.0 | 145ns | 0.12ns | 0.448ns | 0.00393 | 0 | 0 | 280 B |
#6511 | ObjectExtractorSimpleBody |
netcoreapp3.1 | 218ns | 0.159ns | 0.614ns | 0.00374 | 0 | 0 | 272 B |
#6511 | ObjectExtractorSimpleBody |
net472 | 166ns | 0.442ns | 1.71ns | 0.0446 | 0 | 0 | 281 B |
#6511 | ObjectExtractorMoreComplexBody |
net6.0 | 2.9μs | 2.33ns | 8.72ns | 0.0524 | 0 | 0 | 3.78 KB |
#6511 | ObjectExtractorMoreComplexBody |
netcoreapp3.1 | 3.86μs | 12.5ns | 48.3ns | 0.0512 | 0 | 0 | 3.69 KB |
#6511 | ObjectExtractorMoreComplexBody |
net472 | 3.7μs | 6.11ns | 23.7ns | 0.602 | 0.00556 | 0 | 3.8 KB |
Benchmarks.Trace.Asm.AppSecEncoderBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | EncodeArgs |
net6.0 | 37.7μs | 19.9ns | 77ns | 0.451 | 0 | 0 | 32.4 KB |
master | EncodeArgs |
netcoreapp3.1 | 54.5μs | 40.2ns | 145ns | 0.431 | 0 | 0 | 32.4 KB |
master | EncodeArgs |
net472 | 66.4μs | 67.7ns | 262ns | 5.13 | 0.0658 | 0 | 32.5 KB |
master | EncodeLegacyArgs |
net6.0 | 73μs | 23ns | 82.8ns | 0.0367 | 0 | 0 | 2.14 KB |
master | EncodeLegacyArgs |
netcoreapp3.1 | 107μs | 217ns | 842ns | 0 | 0 | 0 | 2.14 KB |
master | EncodeLegacyArgs |
net472 | 156μs | 123ns | 475ns | 0.314 | 0 | 0 | 2.15 KB |
#6511 | EncodeArgs |
net6.0 | 36.8μs | 20.4ns | 79.1ns | 0.458 | 0 | 0 | 32.4 KB |
#6511 | EncodeArgs |
netcoreapp3.1 | 53.8μs | 48.1ns | 180ns | 0.429 | 0 | 0 | 32.4 KB |
#6511 | EncodeArgs |
net472 | 66.3μs | 26.2ns | 94.6ns | 5.15 | 0.066 | 0 | 32.5 KB |
#6511 | EncodeLegacyArgs |
net6.0 | 73.2μs | 21.4ns | 77ns | 0 | 0 | 0 | 2.14 KB |
#6511 | EncodeLegacyArgs |
netcoreapp3.1 | 107μs | 404ns | 1.56μs | 0 | 0 | 0 | 2.14 KB |
#6511 | EncodeLegacyArgs |
net472 | 154μs | 86.3ns | 334ns | 0.307 | 0 | 0 | 2.15 KB |
Benchmarks.Trace.Asm.AppSecWafBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | RunWafRealisticBenchmark |
net6.0 | 174μs | 72.5ns | 281ns | 0 | 0 | 0 | 2.44 KB |
master | RunWafRealisticBenchmark |
netcoreapp3.1 | 190μs | 186ns | 698ns | 0 | 0 | 0 | 2.39 KB |
master | RunWafRealisticBenchmark |
net472 | 201μs | 79.1ns | 306ns | 0.301 | 0 | 0 | 2.46 KB |
master | RunWafRealisticBenchmarkWithAttack |
net6.0 | 115μs | 35.3ns | 132ns | 0 | 0 | 0 | 1.47 KB |
master | RunWafRealisticBenchmarkWithAttack |
netcoreapp3.1 | 125μs | 189ns | 733ns | 0 | 0 | 0 | 1.46 KB |
master | RunWafRealisticBenchmarkWithAttack |
net472 | 132μs | 44.1ns | 159ns | 0.198 | 0 | 0 | 1.49 KB |
#6511 | RunWafRealisticBenchmark |
net6.0 | 175μs | 120ns | 463ns | 0 | 0 | 0 | 2.44 KB |
#6511 | RunWafRealisticBenchmark |
netcoreapp3.1 | 187μs | 240ns | 928ns | 0 | 0 | 0 | 2.39 KB |
#6511 | RunWafRealisticBenchmark |
net472 | 204μs | 206ns | 796ns | 0.303 | 0 | 0 | 2.46 KB |
#6511 | RunWafRealisticBenchmarkWithAttack |
net6.0 | 115μs | 138ns | 536ns | 0 | 0 | 0 | 1.47 KB |
#6511 | RunWafRealisticBenchmarkWithAttack |
netcoreapp3.1 | 124μs | 199ns | 771ns | 0 | 0 | 0 | 1.46 KB |
#6511 | RunWafRealisticBenchmarkWithAttack |
net472 | 133μs | 68.5ns | 265ns | 0.199 | 0 | 0 | 1.49 KB |
Benchmarks.Trace.Iast.StringAspectsBenchmark - Same speed ✔️ More allocations ⚠️
More allocations ⚠️ in #6511
Benchmark
Base Allocated
Diff Allocated
Change
Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑netcoreapp3.1
254.8 KB
262.71 KB
7.91 KB
3.11%
Fewer allocations 🎉 in #6511
Benchmark
Base Allocated
Diff Allocated
Change
Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472
59.07 KB
57.33 KB
-1.74 KB
-2.95%
Benchmark | Base Allocated | Diff Allocated | Change | Change % |
---|---|---|---|---|
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑netcoreapp3.1 | 254.8 KB | 262.71 KB | 7.91 KB | 3.11% |
Benchmark | Base Allocated | Diff Allocated | Change | Change % |
---|---|---|---|---|
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472 | 59.07 KB | 57.33 KB | -1.74 KB | -2.95% |
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | StringConcatBenchmark |
net6.0 | 58.7μs | 684ns | 6.84μs | 0 | 0 | 0 | 43.44 KB |
master | StringConcatBenchmark |
netcoreapp3.1 | 59.8μs | 711ns | 7μs | 0 | 0 | 0 | 42.64 KB |
master | StringConcatBenchmark |
net472 | 37.7μs | 170ns | 637ns | 0 | 0 | 0 | 59.07 KB |
master | StringConcatAspectBenchmark |
net6.0 | 304μs | 1.45μs | 5.81μs | 0 | 0 | 0 | 253.93 KB |
master | StringConcatAspectBenchmark |
netcoreapp3.1 | 343μs | 2.71μs | 26μs | 0 | 0 | 0 | 254.8 KB |
master | StringConcatAspectBenchmark |
net472 | 289μs | 7.04μs | 67.8μs | 0 | 0 | 0 | 278.53 KB |
#6511 | StringConcatBenchmark |
net6.0 | 61.9μs | 733ns | 7.26μs | 0 | 0 | 0 | 43.44 KB |
#6511 | StringConcatBenchmark |
netcoreapp3.1 | 57μs | 319ns | 1.91μs | 0 | 0 | 0 | 42.64 KB |
#6511 | StringConcatBenchmark |
net472 | 39.3μs | 212ns | 1.16μs | 0 | 0 | 0 | 57.33 KB |
#6511 | StringConcatAspectBenchmark |
net6.0 | 301μs | 5.26μs | 51.2μs | 0 | 0 | 0 | 254.2 KB |
#6511 | StringConcatAspectBenchmark |
netcoreapp3.1 | 350μs | 1.91μs | 11.1μs | 0 | 0 | 0 | 262.71 KB |
#6511 | StringConcatAspectBenchmark |
net472 | 311μs | 7.04μs | 68.3μs | 0 | 0 | 0 | 278.53 KB |
f0311a0
to
bc50f3e
Compare
Benchmarks Report for tracer 🐌Benchmarks for #6511 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 - Faster 🎉 Same allocations ✔️
|
Benchmark | base/diff | Base Median (ns) | Diff Median (ns) | Modality |
---|---|---|---|---|
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑net6.0 | 1.133 | 560.08 | 494.32 |
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | StartFinishSpan |
net6.0 | 397ns | 0.224ns | 0.868ns | 0.00799 | 0 | 0 | 576 B |
master | StartFinishSpan |
netcoreapp3.1 | 589ns | 0.407ns | 1.52ns | 0.00775 | 0 | 0 | 576 B |
master | StartFinishSpan |
net472 | 681ns | 0.441ns | 1.71ns | 0.0917 | 0 | 0 | 578 B |
master | StartFinishScope |
net6.0 | 560ns | 0.431ns | 1.67ns | 0.00976 | 0 | 0 | 696 B |
master | StartFinishScope |
netcoreapp3.1 | 676ns | 1.03ns | 3.7ns | 0.00919 | 0 | 0 | 696 B |
master | StartFinishScope |
net472 | 851ns | 0.809ns | 3.13ns | 0.104 | 0 | 0 | 658 B |
#6511 | StartFinishSpan |
net6.0 | 401ns | 0.204ns | 0.791ns | 0.00809 | 0 | 0 | 576 B |
#6511 | StartFinishSpan |
netcoreapp3.1 | 560ns | 0.417ns | 1.56ns | 0.00766 | 0 | 0 | 576 B |
#6511 | StartFinishSpan |
net472 | 697ns | 0.479ns | 1.66ns | 0.0917 | 0 | 0 | 578 B |
#6511 | StartFinishScope |
net6.0 | 494ns | 0.279ns | 1.08ns | 0.00962 | 0 | 0 | 696 B |
#6511 | StartFinishScope |
netcoreapp3.1 | 658ns | 0.637ns | 2.47ns | 0.00918 | 0 | 0 | 696 B |
#6511 | StartFinishScope |
net472 | 913ns | 0.607ns | 2.35ns | 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 | 685ns | 0.378ns | 1.46ns | 0.00958 | 0 | 0 | 696 B |
master | RunOnMethodBegin |
netcoreapp3.1 | 1μs | 0.795ns | 3.08ns | 0.00912 | 0 | 0 | 696 B |
master | RunOnMethodBegin |
net472 | 1.13μs | 0.361ns | 1.4ns | 0.104 | 0 | 0 | 658 B |
#6511 | RunOnMethodBegin |
net6.0 | 637ns | 0.318ns | 1.19ns | 0.0096 | 0 | 0 | 696 B |
#6511 | RunOnMethodBegin |
netcoreapp3.1 | 958ns | 1.22ns | 4.73ns | 0.00944 | 0 | 0 | 696 B |
#6511 | RunOnMethodBegin |
net472 | 1.08μs | 0.79ns | 3.06ns | 0.104 | 0 | 0 | 658 B |
@@ -38,45 +35,6 @@ internal SpanContextPropagator( | |||
_extractors = extractors?.ToArray() ?? []; | |||
} | |||
|
|||
public static SpanContextPropagator Instance |
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.
Removing this property is causing most of code changes in this PR and a less discoverable API ("How do I get the SpanContextPropagator
?").
What do you think of leaving SpanContextPropagator.Instance
as a get-only helper?
public static SpanContextPropagator Instance => Tracer.Instance.TracerManager.SpanContextPropagator;
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.
I'd rather have the dependency on the tracer be explicit. When you have a local instance of the tracer you should use it instead of Tracer.Instance, having SpanContextPropagator.Instance encourages to do the wrong thing.
I believe @andrewlock would agree (we had a somewhat similar discussion before)
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.
When you have a local instance of the tracer you should use it instead of
Tracer.Instance
Yeah, agreed.
And adding a Tracer.Instance.SpanContextPropagator => Tracer.Instance.TracerManager.SpanContextPropagator
helper isn't that helpful. Thanks.
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.
🙇
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.
The (very small) change in the appsec part is fine.
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.
😄
## Summary of changes Randomize the order of the tests. ## Reason for change Flaky tests are much harder to fix when we discover them long after they have been written. By randomizing the order of the tests, I'm hoping to make them fail earlier. In practice, this could temporarily increase the overall flakiness, but I expect this will reduce the overall effort spent on fixing tests. ## Implementation details In `CustomTestFramework`, randomize the list of all tests in each collections, and the collections themselves. The seed is displayed in the output. When a test order causes tests to fail, this allows to deterministically reproduce that test order. ## Other details Four other issues were found thanks to this: #6535, #6532, #6511, #6509
Summary of changes
Store the
SpanContextPropagator
on theTracerManager
instead of using it as a singleton.Reason for change
SpanContextPropagator
was used as a singleton (throughSpanContextPropagator.Instance
) even though it is dependent on the current tracer configuration. Because of that, some unit tests were impacting other tests despite using a scoped tracer.Implementation details
Added a
SpanContextPropagator
property onTracerManager
and updated all usages. When possible, I reused the available instance of tracer, otherwise I usedTracerManager.Instance
.In the process I discovered that
DistributedContextExtractor
isn't used anymore so I removed it.Other details
Discovered while trying to randomize the order of the tests.