-
Notifications
You must be signed in to change notification settings - Fork 140
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
Refactor LifetimeManager
callbacks for UnhandledException
#5557
Conversation
Datadog ReportBranch report: ✅ 0 Failed, 332441 Passed, 2033 Skipped, 22h 25m 14.75s Total Time |
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.
LGTM
tracer/src/Datadog.Trace/Iast/Analyzers/HardcodedSecretsAnalyzer.cs
Outdated
Show resolved
Hide resolved
b037dcb
to
f186113
Compare
Co-authored-by: Kevin Gosse <kevin.gosse@datadoghq.com>
f186113
to
324d922
Compare
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 (5557) - mean (75ms) : 65, 85
. : milestone, 75,
master - mean (75ms) : 66, 83
. : milestone, 75,
section CallTarget+Inlining+NGEN
This PR (5557) - mean (1,002ms) : 978, 1026
. : milestone, 1002,
master - mean (1,005ms) : 981, 1029
. : milestone, 1005,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5557) - mean (109ms) : 106, 112
. : milestone, 109,
master - mean (109ms) : 106, 112
. : milestone, 109,
section CallTarget+Inlining+NGEN
This PR (5557) - mean (720ms) : 692, 747
. : milestone, 720,
master - mean (724ms) : 698, 749
. : milestone, 724,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5557) - mean (94ms) : 91, 97
. : milestone, 94,
master - mean (93ms) : 90, 96
. : milestone, 93,
section CallTarget+Inlining+NGEN
This PR (5557) - mean (675ms) : 654, 697
. : milestone, 675,
master - mean (678ms) : 658, 699
. : milestone, 678,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5557) - mean (192ms) : 186, 198
. : milestone, 192,
master - mean (190ms) : 186, 194
. : milestone, 190,
section CallTarget+Inlining+NGEN
This PR (5557) - mean (1,094ms) : 1058, 1129
. : milestone, 1094,
master - mean (1,075ms) : 1052, 1098
. : milestone, 1075,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5557) - mean (283ms) : 273, 292
. : milestone, 283,
master - mean (275ms) : 271, 279
. : milestone, 275,
section CallTarget+Inlining+NGEN
This PR (5557) - mean (875ms) : 841, 909
. : milestone, 875,
master - mean (860ms) : 837, 882
. : milestone, 860,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5557) - mean (266ms) : 262, 271
. : milestone, 266,
master - mean (264ms) : 260, 268
. : milestone, 264,
section CallTarget+Inlining+NGEN
This PR (5557) - mean (852ms) : 826, 878
. : milestone, 852,
master - mean (850ms) : 820, 881
. : milestone, 850,
|
Benchmarks Report for tracer 🐌Benchmarks for #5557 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 - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️Raw results
|
Benchmarks Report for appsec 🐌Benchmarks for #5557 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 - Faster 🎉 Same allocations ✔️
|
Benchmark | base/diff | Base Median (ns) | Diff Median (ns) | Modality |
---|---|---|---|---|
Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody‑net6.0 | 1.350 | 185.84 | 137.61 |
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | AllCycleSimpleBody |
net6.0 | 72.6μs | 88ns | 341ns | 0.0728 | 0 | 0 | 6 KB |
master | AllCycleSimpleBody |
netcoreapp3.1 | 62.5μs | 84ns | 314ns | 0.0928 | 0 | 0 | 6.94 KB |
master | AllCycleSimpleBody |
net472 | 47.9μs | 50.1ns | 187ns | 1.3 | 0 | 0 | 8.33 KB |
master | AllCycleMoreComplexBody |
net6.0 | 78μs | 168ns | 628ns | 0.117 | 0 | 0 | 9.5 KB |
master | AllCycleMoreComplexBody |
netcoreapp3.1 | 70.5μs | 90.9ns | 352ns | 0.106 | 0 | 0 | 10.36 KB |
master | AllCycleMoreComplexBody |
net472 | 55.6μs | 93.8ns | 363ns | 1.88 | 0.0277 | 0 | 11.84 KB |
master | ObjectExtractorSimpleBody |
net6.0 | 186ns | 0.0823ns | 0.308ns | 0.00395 | 0 | 0 | 280 B |
master | ObjectExtractorSimpleBody |
netcoreapp3.1 | 207ns | 0.273ns | 1.06ns | 0.00372 | 0 | 0 | 272 B |
master | ObjectExtractorSimpleBody |
net472 | 168ns | 0.19ns | 0.658ns | 0.0446 | 0 | 0 | 281 B |
master | ObjectExtractorMoreComplexBody |
net6.0 | 2.99μs | 2.17ns | 8.13ns | 0.0537 | 0 | 0 | 3.78 KB |
master | ObjectExtractorMoreComplexBody |
netcoreapp3.1 | 3.87μs | 1.43ns | 5.53ns | 0.0504 | 0 | 0 | 3.69 KB |
master | ObjectExtractorMoreComplexBody |
net472 | 3.96μs | 4.3ns | 16.7ns | 0.603 | 0.00601 | 0 | 3.8 KB |
#5557 | AllCycleSimpleBody |
net6.0 | 71.7μs | 132ns | 510ns | 0.0708 | 0 | 0 | 6 KB |
#5557 | AllCycleSimpleBody |
netcoreapp3.1 | 62.3μs | 88.3ns | 342ns | 0.0633 | 0 | 0 | 6.94 KB |
#5557 | AllCycleSimpleBody |
net472 | 48.5μs | 69.5ns | 269ns | 1.32 | 0 | 0 | 8.33 KB |
#5557 | AllCycleMoreComplexBody |
net6.0 | 76.8μs | 348ns | 1.3μs | 0.115 | 0 | 0 | 9.5 KB |
#5557 | AllCycleMoreComplexBody |
netcoreapp3.1 | 68.9μs | 69.9ns | 252ns | 0.138 | 0 | 0 | 10.36 KB |
#5557 | AllCycleMoreComplexBody |
net472 | 55.1μs | 49ns | 183ns | 1.88 | 0.0277 | 0 | 11.84 KB |
#5557 | ObjectExtractorSimpleBody |
net6.0 | 138ns | 0.0624ns | 0.233ns | 0.00394 | 0 | 0 | 280 B |
#5557 | ObjectExtractorSimpleBody |
netcoreapp3.1 | 203ns | 0.101ns | 0.379ns | 0.00368 | 0 | 0 | 272 B |
#5557 | ObjectExtractorSimpleBody |
net472 | 169ns | 0.207ns | 0.803ns | 0.0446 | 0 | 0 | 281 B |
#5557 | ObjectExtractorMoreComplexBody |
net6.0 | 2.95μs | 1.02ns | 3.66ns | 0.0532 | 0 | 0 | 3.78 KB |
#5557 | ObjectExtractorMoreComplexBody |
netcoreapp3.1 | 3.9μs | 2.04ns | 7.91ns | 0.0505 | 0 | 0 | 3.69 KB |
#5557 | ObjectExtractorMoreComplexBody |
net472 | 4.37μs | 2.34ns | 9.05ns | 0.602 | 0.00657 | 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 | 39.2μs | 23.5ns | 91.1ns | 0.451 | 0 | 0 | 32.4 KB |
master | EncodeArgs |
netcoreapp3.1 | 56.1μs | 26ns | 101ns | 0.446 | 0 | 0 | 32.4 KB |
master | EncodeArgs |
net472 | 69μs | 69.6ns | 269ns | 5.14 | 0.0689 | 0 | 32.5 KB |
master | EncodeLegacyArgs |
net6.0 | 73.9μs | 28.1ns | 105ns | 0 | 0 | 0 | 2.14 KB |
master | EncodeLegacyArgs |
netcoreapp3.1 | 106μs | 424ns | 1.64μs | 0 | 0 | 0 | 2.14 KB |
master | EncodeLegacyArgs |
net472 | 151μs | 117ns | 455ns | 0.303 | 0 | 0 | 2.15 KB |
#5557 | EncodeArgs |
net6.0 | 39.5μs | 16.7ns | 62.6ns | 0.457 | 0 | 0 | 32.4 KB |
#5557 | EncodeArgs |
netcoreapp3.1 | 55.3μs | 38.6ns | 150ns | 0.437 | 0 | 0 | 32.4 KB |
#5557 | EncodeArgs |
net472 | 68.6μs | 48.1ns | 186ns | 5.14 | 0.0685 | 0 | 32.5 KB |
#5557 | EncodeLegacyArgs |
net6.0 | 81.8μs | 39.4ns | 147ns | 0 | 0 | 0 | 2.14 KB |
#5557 | EncodeLegacyArgs |
netcoreapp3.1 | 104μs | 107ns | 416ns | 0 | 0 | 0 | 2.14 KB |
#5557 | EncodeLegacyArgs |
net472 | 154μs | 146ns | 546ns | 0.309 | 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 | 184μs | 171ns | 662ns | 0 | 0 | 0 | 2.34 KB |
master | RunWafRealisticBenchmark |
netcoreapp3.1 | 195μs | 81.1ns | 303ns | 0 | 0 | 0 | 2.3 KB |
master | RunWafRealisticBenchmark |
net472 | 214μs | 109ns | 422ns | 0.321 | 0 | 0 | 2.36 KB |
master | RunWafRealisticBenchmarkWithAttack |
net6.0 | 122μs | 212ns | 820ns | 0 | 0 | 0 | 1.44 KB |
master | RunWafRealisticBenchmarkWithAttack |
netcoreapp3.1 | 132μs | 193ns | 748ns | 0 | 0 | 0 | 1.42 KB |
master | RunWafRealisticBenchmarkWithAttack |
net472 | 142μs | 41.7ns | 156ns | 0.213 | 0 | 0 | 1.45 KB |
#5557 | RunWafRealisticBenchmark |
net6.0 | 183μs | 294ns | 1.14μs | 0 | 0 | 0 | 2.34 KB |
#5557 | RunWafRealisticBenchmark |
netcoreapp3.1 | 199μs | 189ns | 731ns | 0 | 0 | 0 | 2.3 KB |
#5557 | RunWafRealisticBenchmark |
net472 | 214μs | 140ns | 541ns | 0.319 | 0 | 0 | 2.36 KB |
#5557 | RunWafRealisticBenchmarkWithAttack |
net6.0 | 122μs | 60.8ns | 219ns | 0 | 0 | 0 | 1.44 KB |
#5557 | RunWafRealisticBenchmarkWithAttack |
netcoreapp3.1 | 131μs | 307ns | 1.19μs | 0 | 0 | 0 | 1.42 KB |
#5557 | RunWafRealisticBenchmarkWithAttack |
net472 | 141μs | 39.2ns | 152ns | 0.211 | 0 | 0 | 1.45 KB |
Benchmarks.Trace.Iast.StringAspectsBenchmark - Same speed ✔️ Fewer allocations 🎉
Fewer allocations 🎉 in #5557
Benchmark
Base Allocated
Diff Allocated
Change
Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472
60.49 KB
57.85 KB
-2.64 KB
-4.36%
Benchmark | Base Allocated | Diff Allocated | Change | Change % |
---|---|---|---|---|
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑net472 | 60.49 KB | 57.85 KB | -2.64 KB | -4.36% |
Raw results
Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
---|---|---|---|---|---|---|---|---|---|
master | StringConcatBenchmark |
net6.0 | 61.4μs | 756ns | 7.56μs | 0 | 0 | 0 | 43.44 KB |
master | StringConcatBenchmark |
netcoreapp3.1 | 59.9μs | 899ns | 8.99μs | 0 | 0 | 0 | 42.64 KB |
master | StringConcatBenchmark |
net472 | 38.2μs | 117ns | 440ns | 0 | 0 | 0 | 60.49 KB |
master | StringConcatAspectBenchmark |
net6.0 | 287μs | 1.34μs | 8.86μs | 0 | 0 | 0 | 203.79 KB |
master | StringConcatAspectBenchmark |
netcoreapp3.1 | 298μs | 1.57μs | 8.46μs | 0 | 0 | 0 | 201.01 KB |
master | StringConcatAspectBenchmark |
net472 | 269μs | 6.34μs | 60.8μs | 0 | 0 | 0 | 221.18 KB |
#5557 | StringConcatBenchmark |
net6.0 | 54.3μs | 540ns | 5.35μs | 0 | 0 | 0 | 43.44 KB |
#5557 | StringConcatBenchmark |
netcoreapp3.1 | 53μs | 167ns | 603ns | 0 | 0 | 0 | 42.64 KB |
#5557 | StringConcatBenchmark |
net472 | 37.9μs | 88.7ns | 320ns | 0 | 0 | 0 | 57.85 KB |
#5557 | StringConcatAspectBenchmark |
net6.0 | 280μs | 1.02μs | 3.52μs | 0 | 0 | 0 | 203.38 KB |
#5557 | StringConcatAspectBenchmark |
netcoreapp3.1 | 298μs | 1.56μs | 8.09μs | 0 | 0 | 0 | 201.45 KB |
#5557 | StringConcatAspectBenchmark |
net472 | 250μs | 3.57μs | 34μs | 0 | 0 | 0 | 221.18 KB |
Throughput/Crank Report:zap: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 (5557) (11.710M) : 0, 11710108
master (11.846M) : 0, 11845958
benchmarks/2.9.0 (11.694M) : 0, 11694440
section Automatic
This PR (5557) (8.104M) : 0, 8103530
master (7.996M) : 0, 7995636
benchmarks/2.9.0 (8.460M) : 0, 8459793
section Trace stats
master (8.343M) : 0, 8342594
section Manual
This PR (5557) (10.155M) : 0, 10154554
master (10.189M) : 0, 10189379
section Manual + Automatic
This PR (5557) (7.630M) : 0, 7629936
master (7.524M) : 0, 7523977
section Version Conflict
master (6.816M) : 0, 6816189
gantt
title Throughput Linux arm64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (5557) (9.605M) : 0, 9605126
master (9.733M) : 0, 9732698
benchmarks/2.9.0 (9.570M) : 0, 9570467
section Automatic
This PR (5557) (6.613M) : 0, 6613258
master (6.314M) : 0, 6314355
section Trace stats
master (6.840M) : 0, 6839753
section Manual
This PR (5557) (8.174M) : 0, 8173834
master (8.247M) : 0, 8247298
section Manual + Automatic
This PR (5557) (6.157M) : 0, 6156748
master (6.114M) : 0, 6114235
section Version Conflict
master (5.631M) : 0, 5631182
gantt
title Throughput Windows x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (5557) (10.131M) : 0, 10131074
master (10.093M) : 0, 10093458
benchmarks/2.9.0 (9.961M) : 0, 9960539
section Automatic
This PR (5557) (7.108M) : 0, 7107986
master (7.072M) : 0, 7072071
benchmarks/2.9.0 (7.528M) : 0, 7527964
section Trace stats
master (7.332M) : 0, 7332341
section Manual
This PR (5557) (8.859M) : 0, 8859016
master (8.698M) : 0, 8697956
section Manual + Automatic
This PR (5557) (6.801M) : 0, 6801159
master (6.833M) : 0, 6833078
section Version Conflict
master (6.094M) : 0, 6094229
|
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.
LGTM
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.
LGTM
Summary of changes
Action
/Func<Task>
toAction<Exception?>
/Func<Exception?, Task
Reason for change
In #5549 we needed to track when the application was shutting down from an UnhandledException, and use this information in the shutdown callbacks. That PR added a public static property for simplicity. This PR refactors that slightly, so that there's no static state to check, and instead we pass the
Exception
directly to the callbacks.Implementation details
Mostly just changed the definition of the callbacks to accept an
Exception?
.I considered creating a "context" object something like this:
which would be future-proof, but it seems a bit overkill at this stage, especially as it's an easy refactoring to make.
Test coverage
Covered by existing tests
Other details