-
Notifications
You must be signed in to change notification settings - Fork 150
[Debugger Default-On] DEBUG-3322 Debugger in-product enablement #7366
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
Conversation
This comment has been minimized.
This comment has been minimized.
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.8)
dateFormat X
axisFormat %s
todayMarker off
section Bailout
This PR (7366) - mean (72ms) : 71, 73
. : milestone, 72,
master - mean (72ms) : 71, 73
. : milestone, 72,
section Baseline
This PR (7366) - mean (68ms) : 67, 70
. : milestone, 68,
master - mean (68ms) : 66, 70
. : milestone, 68,
section CallTarget+Inlining+NGEN
This PR (7366) - mean (1,052ms) : 992, 1113
. : milestone, 1052,
master - mean (1,061ms) : 975, 1147
. : milestone, 1061,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Bailout
This PR (7366) - mean (106ms) : 105, 107
. : milestone, 106,
master - mean (107ms) : 105, 108
. : milestone, 107,
section Baseline
This PR (7366) - mean (105ms) : 103, 108
. : milestone, 105,
master - mean (106ms) : 104, 108
. : milestone, 106,
section CallTarget+Inlining+NGEN
This PR (7366) - mean (744ms) : 724, 764
. : milestone, 744,
master - mean (744ms) : 717, 772
. : milestone, 744,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Bailout
This PR (7366) - mean (101ms) : 100, 102
. : milestone, 101,
master - mean (100ms) : 99, 102
. : milestone, 100,
section Baseline
This PR (7366) - mean (100ms) : 98, 102
. : milestone, 100,
master - mean (100ms) : 97, 103
. : milestone, 100,
section CallTarget+Inlining+NGEN
This PR (7366) - mean (782ms) : 742, 821
. : milestone, 782,
master - mean (777ms) : 741, 814
. : milestone, 777,
gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat X
axisFormat %s
todayMarker off
section Bailout
This PR (7366) - mean (93ms) : 92, 94
. : milestone, 93,
master - mean (93ms) : 91, 94
. : milestone, 93,
section Baseline
This PR (7366) - mean (92ms) : 90, 95
. : milestone, 92,
master - mean (92ms) : 90, 94
. : milestone, 92,
section CallTarget+Inlining+NGEN
This PR (7366) - mean (659ms) : 643, 674
. : milestone, 659,
master - mean (665ms) : 648, 683
. : milestone, 665,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat X
axisFormat %s
todayMarker off
section Bailout
This PR (7366) - mean (198ms) : 195, 201
. : milestone, 198,
master - mean (201ms) : 197, 204
. : milestone, 201,
section Baseline
This PR (7366) - mean (195ms) : 190, 200
. : milestone, 195,
master - mean (196ms) : 190, 202
. : milestone, 196,
section CallTarget+Inlining+NGEN
This PR (7366) - mean (1,173ms) : 1116, 1230
. : milestone, 1173,
master - mean (1,177ms) : 1110, 1243
. : milestone, 1177,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Bailout
This PR (7366) - mean (282ms) : 276, 288
. : milestone, 282,
master - mean (282ms) : 275, 289
. : milestone, 282,
section Baseline
This PR (7366) - mean (281ms) : 276, 286
. : milestone, 281,
master - mean (282ms) : 275, 289
. : milestone, 282,
section CallTarget+Inlining+NGEN
This PR (7366) - mean (937ms) : 894, 980
. : milestone, 937,
master - mean (942ms) : 895, 989
. : milestone, 942,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Bailout
This PR (7366) - mean (285ms) : 279, 291
. : milestone, 285,
master - mean (284ms) : 278, 289
. : milestone, 284,
section Baseline
This PR (7366) - mean (284ms) : 279, 288
. : milestone, 284,
master - mean (284ms) : 279, 288
. : milestone, 284,
section CallTarget+Inlining+NGEN
This PR (7366) - mean (1,007ms) : 967, 1046
. : milestone, 1007,
master - mean (1,003ms) : 960, 1046
. : milestone, 1003,
gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat X
axisFormat %s
todayMarker off
section Bailout
This PR (7366) - mean (271ms) : 266, 277
. : milestone, 271,
master - mean (273ms) : 269, 277
. : milestone, 273,
section Baseline
This PR (7366) - mean (271ms) : 267, 275
. : milestone, 271,
master - mean (271ms) : 266, 276
. : milestone, 271,
section CallTarget+Inlining+NGEN
This PR (7366) - mean (854ms) : 837, 871
. : milestone, 854,
master - mean (862ms) : 833, 890
. : milestone, 862,
|
BenchmarksBenchmarks Report for benchmark platform 🐌Benchmarks for #7366 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.Asm.AppSecBodyBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Asm.AppSecEncoderBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Asm.AppSecWafBenchmark - Faster 🎉 Same allocations ✔️
|
| Benchmark | base/diff | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.Asm.AppSecWafBenchmark.RunWafRealisticBenchmark‑netcoreapp3.1 | 2.077 | 868,931.67 | 418,413.90 | |
| Benchmarks.Trace.Asm.AppSecWafBenchmark.RunWafRealisticBenchmarkWithAttack‑netcoreapp3.1 | 1.726 | 518,038.75 | 300,215.18 |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | RunWafRealisticBenchmark |
net6.0 | 398μs | 161ns | 622ns | 0 | 0 | 0 | 4.55 KB |
| master | RunWafRealisticBenchmark |
netcoreapp3.1 | 817μs | 14.7μs | 144μs | 0 | 0 | 0 | 4.48 KB |
| master | RunWafRealisticBenchmark |
net472 | 437μs | 80.5ns | 301ns | 0 | 0 | 0 | 4.66 KB |
| master | RunWafRealisticBenchmarkWithAttack |
net6.0 | 289μs | 106ns | 367ns | 0 | 0 | 0 | 2.24 KB |
| master | RunWafRealisticBenchmarkWithAttack |
netcoreapp3.1 | 490μs | 6.83μs | 67.9μs | 0 | 0 | 0 | 2.22 KB |
| master | RunWafRealisticBenchmarkWithAttack |
net472 | 313μs | 58.8ns | 212ns | 0 | 0 | 0 | 2.29 KB |
| #7366 | RunWafRealisticBenchmark |
net6.0 | 401μs | 423ns | 1.64μs | 0 | 0 | 0 | 4.55 KB |
| #7366 | RunWafRealisticBenchmark |
netcoreapp3.1 | 419μs | 374ns | 1.45μs | 0 | 0 | 0 | 4.48 KB |
| #7366 | RunWafRealisticBenchmark |
net472 | 436μs | 92.9ns | 360ns | 0 | 0 | 0 | 4.66 KB |
| #7366 | RunWafRealisticBenchmarkWithAttack |
net6.0 | 290μs | 180ns | 697ns | 0 | 0 | 0 | 2.24 KB |
| #7366 | RunWafRealisticBenchmarkWithAttack |
netcoreapp3.1 | 300μs | 243ns | 940ns | 0 | 0 | 0 | 2.22 KB |
| #7366 | RunWafRealisticBenchmarkWithAttack |
net472 | 313μs | 147ns | 568ns | 0 | 0 | 0 | 2.29 KB |
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | SendRequest |
net6.0 | 61μs | 51.2ns | 177ns | 0 | 0 | 0 | 14.52 KB |
| master | SendRequest |
netcoreapp3.1 | 71.2μs | 67.2ns | 260ns | 0 | 0 | 0 | 17.42 KB |
| master | SendRequest |
net472 | 0.00966ns | 0.00336ns | 0.013ns | 0 | 0 | 0 | 0 b |
| #7366 | SendRequest |
net6.0 | 60.1μs | 73.8ns | 266ns | 0 | 0 | 0 | 14.52 KB |
| #7366 | SendRequest |
netcoreapp3.1 | 70.1μs | 79.3ns | 307ns | 0 | 0 | 0 | 17.42 KB |
| #7366 | SendRequest |
net472 | 0.00977ns | 0.00305ns | 0.0118ns | 0 | 0 | 0 | 0 b |
Benchmarks.Trace.CharSliceBenchmark - Same speed ✔️ More allocations ⚠️
More allocations ⚠️ in #7366
Benchmark
Base Allocated
Diff Allocated
Change
Change %
Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSlice‑net6.0
4 B
7 B
3 B
75.00%
Fewer allocations 🎉 in #7366
Benchmark
Base Allocated
Diff Allocated
Change
Change %
Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSliceWithPool‑net472
49 B
47 B
-2 B
-4.08%
Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSliceWithPool‑net6.0
5 B
3 B
-2 B
-40.00%
| Benchmark | Base Allocated | Diff Allocated | Change | Change % |
|---|---|---|---|---|
| Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSlice‑net6.0 | 4 B | 7 B | 3 B | 75.00% |
| Benchmark | Base Allocated | Diff Allocated | Change | Change % |
|---|---|---|---|---|
| Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSliceWithPool‑net472 | 49 B | 47 B | -2 B | -4.08% |
| Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSliceWithPool‑net6.0 | 5 B | 3 B | -2 B | -40.00% |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | OriginalCharSlice |
net6.0 | 1.9ms | 4.67μs | 18.1μs | 0 | 0 | 0 | 640.01 KB |
| master | OriginalCharSlice |
netcoreapp3.1 | 2.08ms | 7.43μs | 26.8μs | 0 | 0 | 0 | 640 KB |
| master | OriginalCharSlice |
net472 | 2.64ms | 578ns | 2.24μs | 100 | 0 | 0 | 641.95 KB |
| master | OptimizedCharSlice |
net6.0 | 1.49ms | 99.8ns | 386ns | 0 | 0 | 0 | 4 B |
| master | OptimizedCharSlice |
netcoreapp3.1 | 1.64ms | 579ns | 2.24μs | 0 | 0 | 0 | 1 B |
| master | OptimizedCharSlice |
net472 | 1.96ms | 324ns | 1.21μs | 0 | 0 | 0 | 73 B |
| master | OptimizedCharSliceWithPool |
net6.0 | 796μs | 30ns | 116ns | 0 | 0 | 0 | 5 B |
| master | OptimizedCharSliceWithPool |
netcoreapp3.1 | 832μs | 72.6ns | 262ns | 0 | 0 | 0 | 1 B |
| master | OptimizedCharSliceWithPool |
net472 | 1.18ms | 241ns | 902ns | 0 | 0 | 0 | 49 B |
| #7366 | OriginalCharSlice |
net6.0 | 1.95ms | 599ns | 2.24μs | 0 | 0 | 0 | 640.01 KB |
| #7366 | OriginalCharSlice |
netcoreapp3.1 | 2.13ms | 2.78μs | 10.8μs | 0 | 0 | 0 | 640 KB |
| #7366 | OriginalCharSlice |
net472 | 2.6ms | 274ns | 1.06μs | 100 | 0 | 0 | 641.95 KB |
| #7366 | OptimizedCharSlice |
net6.0 | 1.35ms | 568ns | 2.2μs | 0 | 0 | 0 | 7 B |
| #7366 | OptimizedCharSlice |
netcoreapp3.1 | 1.68ms | 323ns | 1.21μs | 0 | 0 | 0 | 1 B |
| #7366 | OptimizedCharSlice |
net472 | 1.97ms | 520ns | 2.01μs | 0 | 0 | 0 | 73 B |
| #7366 | OptimizedCharSliceWithPool |
net6.0 | 835μs | 32.1ns | 120ns | 0 | 0 | 0 | 3 B |
| #7366 | OptimizedCharSliceWithPool |
netcoreapp3.1 | 844μs | 146ns | 564ns | 0 | 0 | 0 | 1 B |
| #7366 | OptimizedCharSliceWithPool |
net472 | 1.16ms | 90.6ns | 339ns | 0 | 0 | 0 | 47 B |
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | WriteAndFlushEnrichedTraces |
net6.0 | 683μs | 1.75μs | 6.07μs | 0 | 0 | 0 | 41.7 KB |
| master | WriteAndFlushEnrichedTraces |
netcoreapp3.1 | 680μs | 745ns | 2.88μs | 0 | 0 | 0 | 41.87 KB |
| master | WriteAndFlushEnrichedTraces |
net472 | 968μs | 3.99μs | 14.4μs | 8.33 | 0 | 0 | 56.36 KB |
| #7366 | WriteAndFlushEnrichedTraces |
net6.0 | 701μs | 729ns | 2.82μs | 0 | 0 | 0 | 41.56 KB |
| #7366 | WriteAndFlushEnrichedTraces |
netcoreapp3.1 | 707μs | 4.04μs | 30.5μs | 0 | 0 | 0 | 41.79 KB |
| #7366 | WriteAndFlushEnrichedTraces |
net472 | 903μs | 3.78μs | 14.6μs | 8.33 | 0 | 0 | 56.08 KB |
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | ExecuteNonQuery |
net6.0 | 1.92μs | 6.88ns | 26.6ns | 0 | 0 | 0 | 1.02 KB |
| master | ExecuteNonQuery |
netcoreapp3.1 | 2.74μs | 7.4ns | 28.7ns | 0 | 0 | 0 | 1.02 KB |
| master | ExecuteNonQuery |
net472 | 2.82μs | 2.35ns | 8.48ns | 0.156 | 0.0141 | 0 | 987 B |
| #7366 | ExecuteNonQuery |
net6.0 | 1.86μs | 2.57ns | 9.94ns | 0 | 0 | 0 | 1.02 KB |
| #7366 | ExecuteNonQuery |
netcoreapp3.1 | 2.47μs | 9.36ns | 36.2ns | 0 | 0 | 0 | 1.02 KB |
| #7366 | ExecuteNonQuery |
net472 | 2.73μs | 1.53ns | 5.72ns | 0.149 | 0.0135 | 0 | 987 B |
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | CallElasticsearch |
net6.0 | 1.78μs | 6.16ns | 21.4ns | 0 | 0 | 0 | 1.03 KB |
| master | CallElasticsearch |
netcoreapp3.1 | 2.26μs | 9.56ns | 37ns | 0 | 0 | 0 | 1.03 KB |
| master | CallElasticsearch |
net472 | 3.56μs | 4.71ns | 18.2ns | 0.16 | 0 | 0 | 1.04 KB |
| master | CallElasticsearchAsync |
net6.0 | 1.85μs | 9.52ns | 45.6ns | 0 | 0 | 0 | 1.01 KB |
| master | CallElasticsearchAsync |
netcoreapp3.1 | 2.33μs | 11.1ns | 41.4ns | 0 | 0 | 0 | 1.08 KB |
| master | CallElasticsearchAsync |
net472 | 3.79μs | 2.38ns | 9.23ns | 0.17 | 0 | 0 | 1.1 KB |
| #7366 | CallElasticsearch |
net6.0 | 1.8μs | 8.68ns | 33.6ns | 0 | 0 | 0 | 1.03 KB |
| #7366 | CallElasticsearch |
netcoreapp3.1 | 2.28μs | 6.15ns | 23ns | 0 | 0 | 0 | 1.03 KB |
| #7366 | CallElasticsearch |
net472 | 3.56μs | 3.33ns | 12.4ns | 0.16 | 0 | 0 | 1.04 KB |
| #7366 | CallElasticsearchAsync |
net6.0 | 1.79μs | 8.62ns | 34.5ns | 0 | 0 | 0 | 1.01 KB |
| #7366 | CallElasticsearchAsync |
netcoreapp3.1 | 2.38μs | 7.13ns | 27.6ns | 0 | 0 | 0 | 1.08 KB |
| #7366 | CallElasticsearchAsync |
net472 | 3.81μs | 3.69ns | 13.8ns | 0.171 | 0 | 0 | 1.1 KB |
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | ExecuteAsync |
net6.0 | 1.98μs | 6.54ns | 25.3ns | 0 | 0 | 0 | 952 B |
| master | ExecuteAsync |
netcoreapp3.1 | 2.3μs | 1.89ns | 7.32ns | 0 | 0 | 0 | 952 B |
| master | ExecuteAsync |
net472 | 2.42μs | 2.32ns | 8.69ns | 0.145 | 0 | 0 | 915 B |
| #7366 | ExecuteAsync |
net6.0 | 1.86μs | 2.23ns | 8.34ns | 0 | 0 | 0 | 952 B |
| #7366 | ExecuteAsync |
netcoreapp3.1 | 2.37μs | 6.75ns | 26.1ns | 0 | 0 | 0 | 952 B |
| #7366 | ExecuteAsync |
net472 | 2.51μs | 2.07ns | 8.02ns | 0.138 | 0 | 0 | 915 B |
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | SendAsync |
net6.0 | 6.91μs | 4.85ns | 17.5ns | 0 | 0 | 0 | 2.36 KB |
| master | SendAsync |
netcoreapp3.1 | 8.38μs | 22.4ns | 86.9ns | 0 | 0 | 0 | 2.9 KB |
| master | SendAsync |
net472 | 12.5μs | 7.53ns | 29.2ns | 0.498 | 0 | 0 | 3.18 KB |
| #7366 | SendAsync |
net6.0 | 6.97μs | 6.35ns | 24.6ns | 0 | 0 | 0 | 2.36 KB |
| #7366 | SendAsync |
netcoreapp3.1 | 8.44μs | 24.2ns | 87.3ns | 0 | 0 | 0 | 2.9 KB |
| #7366 | SendAsync |
net472 | 12.2μs | 9.37ns | 36.3ns | 0.487 | 0 | 0 | 3.18 KB |
Benchmarks.Trace.Iast.StringAspectsBenchmark - Same speed ✔️ More allocations ⚠️
More allocations ⚠️ in #7366
Benchmark
Base Allocated
Diff Allocated
Change
Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑netcoreapp3.1
257.73 KB
274.3 KB
16.58 KB
6.43%
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑netcoreapp3.1
42.64 KB
44.33 KB
1.69 KB
3.96%
Fewer allocations 🎉 in #7366
Benchmark
Base Allocated
Diff Allocated
Change
Change %
Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑net472
348.55 KB
286.72 KB
-61.83 KB
-17.74%
| Benchmark | Base Allocated | Diff Allocated | Change | Change % |
|---|---|---|---|---|
| Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑netcoreapp3.1 | 257.73 KB | 274.3 KB | 16.58 KB | 6.43% |
| Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatBenchmark‑netcoreapp3.1 | 42.64 KB | 44.33 KB | 1.69 KB | 3.96% |
| Benchmark | Base Allocated | Diff Allocated | Change | Change % |
|---|---|---|---|---|
| Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark‑net472 | 348.55 KB | 286.72 KB | -61.83 KB | -17.74% |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | StringConcatBenchmark |
net6.0 | 47.6μs | 215ns | 803ns | 0 | 0 | 0 | 43.77 KB |
| master | StringConcatBenchmark |
netcoreapp3.1 | 49.5μs | 280ns | 1.81μs | 0 | 0 | 0 | 42.64 KB |
| master | StringConcatBenchmark |
net472 | 57.2μs | 241ns | 868ns | 0 | 0 | 0 | 57.34 KB |
| master | StringConcatAspectBenchmark |
net6.0 | 465μs | 1.42μs | 5.11μs | 0 | 0 | 0 | 258.26 KB |
| master | StringConcatAspectBenchmark |
netcoreapp3.1 | 541μs | 2.5μs | 10μs | 0 | 0 | 0 | 257.73 KB |
| master | StringConcatAspectBenchmark |
net472 | 407μs | 2.14μs | 10.9μs | 0 | 0 | 0 | 348.55 KB |
| #7366 | StringConcatBenchmark |
net6.0 | 48.3μs | 207ns | 826ns | 0 | 0 | 0 | 43.74 KB |
| #7366 | StringConcatBenchmark |
netcoreapp3.1 | 50.5μs | 295ns | 2.61μs | 0 | 0 | 0 | 44.33 KB |
| #7366 | StringConcatBenchmark |
net472 | 57.4μs | 136ns | 510ns | 0 | 0 | 0 | 57.34 KB |
| #7366 | StringConcatAspectBenchmark |
net6.0 | 456μs | 1.9μs | 6.57μs | 0 | 0 | 0 | 258.04 KB |
| #7366 | StringConcatAspectBenchmark |
netcoreapp3.1 | 537μs | 2.8μs | 14μs | 0 | 0 | 0 | 274.3 KB |
| #7366 | StringConcatAspectBenchmark |
net472 | 410μs | 2.19μs | 11.4μs | 0 | 0 | 0 | 286.72 KB |
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | EnrichedLog |
net6.0 | 2.62μs | 4.41ns | 17.1ns | 0 | 0 | 0 | 1.7 KB |
| master | EnrichedLog |
netcoreapp3.1 | 3.29μs | 9.02ns | 35ns | 0 | 0 | 0 | 1.7 KB |
| master | EnrichedLog |
net472 | 4μs | 4.26ns | 16.5ns | 0.258 | 0 | 0 | 1.64 KB |
| #7366 | EnrichedLog |
net6.0 | 2.59μs | 11.9ns | 47.7ns | 0 | 0 | 0 | 1.7 KB |
| #7366 | EnrichedLog |
netcoreapp3.1 | 3.41μs | 9.62ns | 37.2ns | 0 | 0 | 0 | 1.7 KB |
| #7366 | EnrichedLog |
net472 | 4.02μs | 3.67ns | 14.2ns | 0.241 | 0 | 0 | 1.64 KB |
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | EnrichedLog |
net6.0 | 123μs | 110ns | 398ns | 0 | 0 | 0 | 4.31 KB |
| master | EnrichedLog |
netcoreapp3.1 | 128μs | 203ns | 787ns | 0 | 0 | 0 | 4.31 KB |
| master | EnrichedLog |
net472 | 167μs | 38.1ns | 148ns | 0 | 0 | 0 | 4.52 KB |
| #7366 | EnrichedLog |
net6.0 | 123μs | 102ns | 394ns | 0 | 0 | 0 | 4.31 KB |
| #7366 | EnrichedLog |
netcoreapp3.1 | 128μs | 194ns | 753ns | 0 | 0 | 0 | 4.31 KB |
| #7366 | EnrichedLog |
net472 | 168μs | 110ns | 426ns | 0 | 0 | 0 | 4.52 KB |
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | EnrichedLog |
net6.0 | 4.99μs | 19.9ns | 77.1ns | 0 | 0 | 0 | 2.26 KB |
| master | EnrichedLog |
netcoreapp3.1 | 6.71μs | 20.4ns | 78.9ns | 0 | 0 | 0 | 2.26 KB |
| master | EnrichedLog |
net472 | 7.46μs | 9.81ns | 38ns | 0.3 | 0 | 0 | 2.08 KB |
| #7366 | EnrichedLog |
net6.0 | 4.84μs | 10.6ns | 41.2ns | 0 | 0 | 0 | 2.26 KB |
| #7366 | EnrichedLog |
netcoreapp3.1 | 6.74μs | 26.1ns | 101ns | 0 | 0 | 0 | 2.26 KB |
| #7366 | EnrichedLog |
net472 | 7.65μs | 6.64ns | 24.8ns | 0.306 | 0 | 0 | 2.08 KB |
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | SendReceive |
net6.0 | 1.99μs | 10.1ns | 44.2ns | 0 | 0 | 0 | 1.2 KB |
| master | SendReceive |
netcoreapp3.1 | 2.71μs | 13.2ns | 52.8ns | 0 | 0 | 0 | 1.2 KB |
| master | SendReceive |
net472 | 3.12μs | 7.59ns | 29.4ns | 0.189 | 0 | 0 | 1.2 KB |
| #7366 | SendReceive |
net6.0 | 1.98μs | 1.78ns | 6.65ns | 0 | 0 | 0 | 1.2 KB |
| #7366 | SendReceive |
netcoreapp3.1 | 2.57μs | 1.75ns | 6.78ns | 0 | 0 | 0 | 1.2 KB |
| #7366 | SendReceive |
net472 | 3.21μs | 4.77ns | 18.5ns | 0.177 | 0 | 0 | 1.2 KB |
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | EnrichedLog |
net6.0 | 4.13μs | 16.6ns | 64.2ns | 0 | 0 | 0 | 1.58 KB |
| master | EnrichedLog |
netcoreapp3.1 | 5.6μs | 12.3ns | 47.7ns | 0 | 0 | 0 | 1.63 KB |
| master | EnrichedLog |
net472 | 6.65μs | 11.3ns | 43.9ns | 0.3 | 0 | 0 | 2.03 KB |
| #7366 | EnrichedLog |
net6.0 | 4.26μs | 2.76ns | 10.3ns | 0 | 0 | 0 | 1.58 KB |
| #7366 | EnrichedLog |
netcoreapp3.1 | 5.61μs | 11.1ns | 42.8ns | 0 | 0 | 0 | 1.63 KB |
| #7366 | EnrichedLog |
net472 | 6.78μs | 7.7ns | 29.8ns | 0.306 | 0 | 0 | 2.03 KB |
Benchmarks.Trace.SpanBenchmark - Same speed ✔️ Same allocations ✔️
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | StartFinishSpan |
net6.0 | 752ns | 3.61ns | 14ns | 0 | 0 | 0 | 576 B |
| master | StartFinishSpan |
netcoreapp3.1 | 916ns | 4.94ns | 29.6ns | 0 | 0 | 0 | 576 B |
| master | StartFinishSpan |
net472 | 898ns | 0.0935ns | 0.35ns | 0.0903 | 0 | 0 | 578 B |
| master | StartFinishScope |
net6.0 | 907ns | 4.31ns | 16.7ns | 0 | 0 | 0 | 696 B |
| master | StartFinishScope |
netcoreapp3.1 | 1.15μs | 1.32ns | 5.1ns | 0 | 0 | 0 | 696 B |
| master | StartFinishScope |
net472 | 1.09μs | 0.242ns | 0.937ns | 0.104 | 0 | 0 | 658 B |
| #7366 | StartFinishSpan |
net6.0 | 743ns | 3.54ns | 21.8ns | 0 | 0 | 0 | 576 B |
| #7366 | StartFinishSpan |
netcoreapp3.1 | 936ns | 4.39ns | 18.1ns | 0 | 0 | 0 | 576 B |
| #7366 | StartFinishSpan |
net472 | 969ns | 0.264ns | 0.987ns | 0.0912 | 0 | 0 | 578 B |
| #7366 | StartFinishScope |
net6.0 | 889ns | 4.39ns | 18.1ns | 0 | 0 | 0 | 696 B |
| #7366 | StartFinishScope |
netcoreapp3.1 | 1.13μs | 5.35ns | 20.7ns | 0 | 0 | 0 | 696 B |
| #7366 | StartFinishScope |
net472 | 1.12μs | 0.31ns | 1.2ns | 0.101 | 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 | 1.04μs | 5.36ns | 25.7ns | 0 | 0 | 0 | 696 B |
| master | RunOnMethodBegin |
netcoreapp3.1 | 1.33μs | 6.48ns | 29ns | 0 | 0 | 0 | 696 B |
| master | RunOnMethodBegin |
net472 | 1.4μs | 1.54ns | 5.76ns | 0.105 | 0 | 0 | 658 B |
| #7366 | RunOnMethodBegin |
net6.0 | 1.06μs | 0.852ns | 3.19ns | 0 | 0 | 0 | 696 B |
| #7366 | RunOnMethodBegin |
netcoreapp3.1 | 1.31μs | 6.7ns | 33.5ns | 0 | 0 | 0 | 696 B |
| #7366 | RunOnMethodBegin |
net472 | 1.41μs | 0.819ns | 3.06ns | 0.0988 | 0 | 0 | 658 B |
a971687 to
e22ff90
Compare
e22ff90 to
eee68bd
Compare
58df774 to
ac5d278
Compare
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 wonder what impact on startup this will have by not initializing it in the background. As best I can tell from comparing the execution benchmarks to master, this adds about 12% to startup time when debugging is not enabled, which seems problematic for many scenarios. It's not clear where all the source of that overhead is, but I suspect we could reduce some of it by not initializing most things until the debugger is actually enabled
All of the initialization seems very complex, much more so than I would have anticipated, but I'm not sure how much of that is just due to my not being familiar with the DI code in general.
tracer/src/Datadog.Trace/Configuration/DynamicConfigurationManager.cs
Outdated
Show resolved
Hide resolved
| internal string ServiceName { get; private set; } | ||
|
|
||
| private async Task<bool> WaitForDiscoveryServiceAsync(IDiscoveryService discoveryService, Task shutdownTask) | ||
| private string GetServiceName(TracerSettings tracerSettings) |
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 know it existed before, but the fact that it does is more evidence that our layering here is just wrong 😢 I still think we need to refactor this significantly to accept the fact that there's a dependence of debugger on tracing. It shouldn't be intiailizing independently like this, as it's a recipe for mismatch. All for later of course, but it's becoming clearer and clearer
| try | ||
| { | ||
| return true; | ||
| return TraceUtil.NormalizeTag(tracerSettings.ServiceName ?? TracerManager.Instance.DefaultServiceName); |
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 TracerManager.Instance access is a big issue frankly. It could refer to a completely different TracerSettings object for example. Essentially, this DebuggerManager should be created as part of the TracerManager creation, and then it can just pass in the values you need. Again, something for a different PR that we can work on.
| return _processExit.Task; | ||
| } | ||
|
|
||
| OneTimeSetup(tracerSettings); |
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 have some suspicions that that is the source of the extra overhead we saw coming from the refactor. Essentially you're doing a bunch of one-time setup, on the startup hot path, even if the debugger is never used. I think we should try to do this work lazily (I started working on a PR to do that)
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.
In case the symbol upload is disabled, or all 3 products (CO, ER and DI) are explicitly disabled, the symbols upload should be skipped. The thing is that if the features are not explicitly disabled, but empty, we will want to upload symbols as soon as possible even if they are not currently enabled, in case it is enabled remotely. Otherwise, the user may not see symbols until we upload them all. Note, that we don't start to upload symbols untill we got a rc request to do so.
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.
Fixed in 79622fa
Let me know if you still want to to disable it completly untill we got the first rc request to enable it.
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.
See also ddfb2c4
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.
Let me know if you still want to to disable it completly untill we got the first rc request to enable it
I think we should do that, because otherwise we're doing a bunch of work (and impacting performance) that most of our customers will never benefit from. I think we should definitely be lazy for all of this setup wherever we can be, because it has a clear impact on our benchmarks, and for large apps that could end up being very significant
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.
After bfa3f79b5c87db7e916afbdc9375dce6dfcf2ae0 symdb won't be on by default if di is explicitly disabled
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.
won't be on by default if di is explicitly disabled
Yeah, the concern is that's basically never going to be the case, so it's effectively still always going to be on by default for customers
| public bool DynamicInstrumentationEnabled | ||
| { | ||
| get => | ||
| (_internalDynamicInstrumentationEnabled == true && DynamicSettings.DynamicInstrumentationEnabled == null) | ||
| || (_internalDynamicInstrumentationEnabled == null && DynamicSettings.DynamicInstrumentationEnabled == true); | ||
| init { } | ||
| } |
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 suggest you rewrite this to be similar to the tracer version e.g.
| public bool DynamicInstrumentationEnabled | |
| { | |
| get => | |
| (_internalDynamicInstrumentationEnabled == true && DynamicSettings.DynamicInstrumentationEnabled == null) | |
| || (_internalDynamicInstrumentationEnabled == null && DynamicSettings.DynamicInstrumentationEnabled == true); | |
| init { } | |
| } | |
| public bool DynamicInstrumentationEnabled | |
| => DynamicSettings.DynamicInstrumentationEnabled ?? _internalDynamicInstrumentationEnabled; |
And then remove all the other changes above, make _internalDynamicInstrumentationEnabled a bool and do this change:
- DynamicInstrumentationEnabled = config
+ _internalDynamicInstrumentationEnabled = config
.WithKeys(ConfigurationKeys.Debugger.DynamicInstrumentationEnabled)
.AsBool(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.
But then how can I distinguish between empty/null to explicit false? I need to know that to decide if a feature might be enabled in the future or not.
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 need to know that to decide if a feature might be enabled in the future or not.
Remote configuration overrides the value anyway, so it can always be added in the future I think? And if not, then we have a layering problem and confusion issue across products :/
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.
Does it make sense now? bfa3f79b5c87db7e916afbdc9375dce6dfcf2ae0
tracer/src/Datadog.Trace/Debugger/ExceptionAutoInstrumentation/ExceptionTrackManager.cs
Outdated
Show resolved
Hide resolved
## Summary of changes Disables the debugger initialization if it's not explicitly enabled ## Reason for change We have been seeing some hangs in CI which seem to be caused by the debugger (based on log messages) and I suspect are due to [this refactoring](015d218). Rather than revert that large piece of refactoring (and causing massive conflicts for the [remote-enablement PR](#7366)), for now we primarily want to ensure customers _not_ using the debugger are not impacted. ## Implementation details Check the debugger and exception replay settings to see if the debugger is required. If not, bail out of initialization immediately. ## Test coverage This is the test
1782ee2 to
41e5d62
Compare
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.
submitting first batch of comments, will keep reviewing and potentially send another batch.
Nice work!
| else | ||
| { | ||
| Log.Information($"Dynamic Instrumentation is disabled. To enable it, please set {ConfigurationKeys.Debugger.DynamicInstrumentationEnabled} environment variable to 'true'."); | ||
| if (!manager.DebuggerSettings.DynamicInstrumentationEnabled) |
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.
| /* | ||
| if (debugLogsEnabled != null && debugLogsEnabled.Value != GlobalSettings.Instance.DebugEnabled) | ||
| { | ||
| GlobalSettings.SetDebugEnabledInternal(debugLogsEnabled.Value); | ||
| Security.Instance.SetDebugEnabled(debugLogsEnabled.Value); | ||
| Log.Information("Applying new dynamic configuration"); | ||
| NativeMethods.UpdateSettings(new[] { ConfigurationKeys.DebugEnabled }, new[] { debugLogsEnabled.Value ? "1" : "0" }); | ||
| } | ||
| */ |
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.
do we need this comment or it's a left over?
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 was there before and I'm not sure if should I delete it or not
| var debugger = new DynamicInstrumentation(settings, discoveryService, rcmSubscriptionManagerMock, lineProbeResolver, snapshotUploader, diagnosticsUploader, probeStatusPoller, updater, new DogStatsd.NoOpStatsd()); | ||
| debugger.Initialize(); | ||
|
|
||
| await Task.Delay(1000); |
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.
why do we need this delay?
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.
To let the DI background initialization to run theoritically so in case we have a bug in our enabling condition this test will fail.
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.
We should not have any Task.Delay in tests, because it will make them flaky. I practically guarantee that this will take longer than 1s in some cases, because the CI machines run at basically 100% capacity.
I strongly suggest having a deterministic way to know whether initialization has completed. If you can't, that suggests we need to redesign the code - APIs should be testable by default (e.g. no static access too 😉)
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.
please let's avoid using any Thread.Sleep or Task.Delay in tests.
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.
Fixed in 5683a7c78bfbca902bc8eb61710df96ff20c78ad
I strongly suggest having a deterministic way to know whether initialization has completed
We have a way for that, but in this case we need the opposite. Anyway I prefer to leave it as is was before, we don't have to change it now.
| DynamicInstrumentation? instance = null; | ||
| var created = false; | ||
| lock (_syncLock) | ||
| { | ||
| if (DynamicInstrumentation == null) | ||
| { | ||
| // ignore ObjectDisposedException or SemaphoreFullException | ||
| _dynamicInstrumentation = DebuggerFactory.CreateDynamicInstrumentation( | ||
| discoveryService, | ||
| RcmSubscriptionManager.Instance, | ||
| tracerSettings, | ||
| ServiceName, | ||
| debuggerSettings, | ||
| tracerManager.GitMetadataTagsProvider); | ||
| created = true; | ||
| } | ||
|
|
||
| instance = _dynamicInstrumentation; | ||
| } | ||
|
|
||
| if (created && instance != null) | ||
| { | ||
| Log.Debug("Dynamic Instrumentation has been created"); | ||
| instance.Initialize(); | ||
| tracerManager.Telemetry.ProductChanged(TelemetryProductType.DynamicInstrumentation, enabled: true, error: null); | ||
| } |
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.
since you lock only specific part, do we care if we end up calling instance.Initialize on DynamicInstrumentation instance while another thread tries to create/destroy it potentially? Do we have potential race here?
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.
Fixed in 1d6bfc7
ddfb2c4 to
1d6bfc7
Compare
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.
Thanks, I left a few comments, in particular I think we need to try to reduce the impact when the debugger is not enabled.
That said, I think the main thing missing at this point is integration tests for the remote enablement (unless they're already there and I just missed them!). I think we should have tests for:
- start with the debugger enabled, disable it, then re-enable it
- start with the debugger disabled, enable it, then re-disable it
I believe there are some ASM tests that do something similar, so might be worth looking at those for inspiration! Thanks
| private readonly TaskCompletionSource<bool> _processExit; | ||
| private volatile bool _isDebuggerEndpointAvailable; | ||
| private int _initialized; | ||
| private CancellationTokenSource? _diDebounceCts; |
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.
We shouldn't use CancellationTokenSource in paths that can be hit during shutdown, because it can cause crashes. We generally have to use TaskCompletionSource instead (even though it's a bit of a pain)
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.
Thanks for pointing that. Fixed in f3d6f4c949964231419ab232c0b5939abb08ac8e
| return _processExit.Task; | ||
| } | ||
|
|
||
| OneTimeSetup(tracerSettings); |
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.
Let me know if you still want to to disable it completly untill we got the first rc request to enable it
I think we should do that, because otherwise we're doing a bunch of work (and impacting performance) that most of our customers will never benefit from. I think we should definitely be lazy for all of this setup wherever we can be, because it has a clear impact on our benchmarks, and for large apps that could end up being very significant
| writer.WriteEndObject(); | ||
| } | ||
|
|
||
| Log.Information("DATADOG DEBUGGER CONFIGURATION - {Configuration}", stringWriter.ToString()); |
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.
Given you always write this log (which I think is fine, as it mirrors the tracer version 👍), you can probably change the other logs that are used by tests to use this one, instead of writing additional logs
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 think that we use today only this initialization log in tests (there ar emore but not related to initialization)
Dynamic Instrumentation is disabled. To enable it, please set DD_DYNAMIC_INSTRUMENTATION_ENABLED environment variable to 'true'.
We can change the test to use the configuration value but we still want this log (without this log).
| if (originalState != 0) | ||
| if (!_settings.DynamicInstrumentationEnabled) | ||
| { | ||
| Log.Information("Dynamic Instrumentation is disabled. To enable it, please set DD_DYNAMIC_INSTRUMENTATION_ENABLED environment variable to 'true'."); |
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.
nit: You're already writing this log elsewhere, we should remove one of them (this is a problem today, we write the same log twice)
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.
Fixed in 9cc8537e8fce4bd242b9c5798bfc51c00d140e38
| } | ||
|
|
||
| Log.Information("Dynamic Instrumentation initialization started"); | ||
| Log.Information("Dynamic Instrumentation initialization started"); |
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.
nit: this should probably be debug
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.
Fixed in 9cc8537e8fce4bd242b9c5798bfc51c00d140e38
| Interlocked.Exchange(ref _initState, 0); | ||
| } | ||
| // Start initialization in background | ||
| _ = Task.Run(InitializeAsync) |
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's so much "in the background" stuff that I get lost 😢 Given everything is started "in the background" anyway in Instrumentation, do we really need to make everything explicitly use Task.Run? It would be much easier to follow if the DebuggerManager for example, could just do something like this:
Task allTasks = Task.WhenAll(
DynamicInstrumentation.InitializeAsync(),
SymbolUploader.InitializeAsync(),
SomethingElse.InitializeAsync());Not required, and could be another PR if you want, but I think it would make a lot of these things much easier to follow
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.
Deleted some Task.Run in d3b96db
| Enabled = config.WithKeys(ConfigurationKeys.Debugger.ExceptionReplayEnabled).AsBool(false); | ||
|
|
||
| _internalExceptionReplayEnabled = config.WithKeys(ConfigurationKeys.Debugger.ExceptionReplayEnabled).AsBool(); |
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.
You are reading the same setting from all of the sources, multiple times, this is expensive, and shouldn't be necessary.
If you need to do this (I'm still not entirely convinced you do - you should always be able to enable via remote config) you can do it like this:
| Enabled = config.WithKeys(ConfigurationKeys.Debugger.ExceptionReplayEnabled).AsBool(false); | |
| _internalExceptionReplayEnabled = config.WithKeys(ConfigurationKeys.Debugger.ExceptionReplayEnabled).AsBool(); | |
| var enabledResult = config.WithKeys(ConfigurationKeys.Debugger.ExceptionReplayEnabled).AsBool(false); | |
| Enabled = enabledResult.WithDefault(false); | |
| CanBeEnabled = !(enabledResult.ConfigurationResult.IsValid && enabledResult.ConfigurationResult.Result == false); |
Note that I changed the name of the property (and removed the unecessary field), because it's currently confusing to have ExceptionReplaySettings.Enabled and IsExceptionReplayDisabled meaning different things
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.
Does it make sense now? bfa3f79b5c87db7e916afbdc9375dce6dfcf2ae0
| _exceptionProcessorTask = Task.Factory.StartNew( | ||
| async () => await StartExceptionProcessingAsync(_cts.Token).ConfigureAwait(false), TaskCreationOptions.LongRunning) | ||
| () => StartExceptionProcessingAsync(), | ||
| TaskCreationOptions.LongRunning) |
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.
You probably need to make sure you're using the default scheduler if you're using StartNew instead of Task.Run()
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.
Fixed in dd64f6e89a16c1969abd9c1deb54d70d0f8c89c4
| _ = Task.Run( | ||
| async () => | ||
| { | ||
| try | ||
| { | ||
| await SymbolsUploader.StartFlushingAsync().ConfigureAwait(false); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Log.Error(ex, "Failed to initialize symbol uploader"); | ||
| } | ||
| }); |
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.
Why not
.ContinueWith(
t => Log.Error(t?.Exception, "Failed to initialize symbol uploader"),
TaskContinuationOptions.OnlyOnFaulted);
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.
Fixed in dd64f6e89a16c1969abd9c1deb54d70d0f8c89c4
| _exceptionProcessorTask = Task.Factory.StartNew( | ||
| async () => await StartExceptionProcessingAsync(_cts.Token).ConfigureAwait(false), TaskCreationOptions.LongRunning) | ||
| () => StartExceptionProcessingAsync(), | ||
| TaskCreationOptions.LongRunning) |
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.
This is a LongRunning task, a new thread is created for this one, but in StartExceptionProcessingAsync you have a .ConfigureAwait(false) so this new thread will be lost at that first Task.WhenAny, because on return the continuation will be schedule to a thread pool thread.
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.
Thanks for pointing that. Fixed in dd64f6e89a16c1969abd9c1deb54d70d0f8c89c4
d3b96db to
310fcc4
Compare
| bool coDisabled = !debuggerSettings.CodeOriginForSpansCanBeEnabled; | ||
| bool erDisabled = !manager.ExceptionReplaySettings.CanBeEnabled || debuggerSettings.DynamicSettings.ExceptionReplayEnabled == false; | ||
|
|
||
| if (diDisabled && coDisabled && erDisabled) |
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.
Just FYI, my concern is that these will almost never all be set (because customers would have to explicitly disable all of them) so this effectively means we will be initializing for all customers - we'll need to check the execution benchmarks to check that this doesn't have an adverse impact in general 🙂
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.
That true but we also check later that SymDb is true which will be true only if DI is not false so even if the customer set only DI to false, we will not upload symbols.
Yeah, the concern is that's basically never going to be the case, so it's effectively still always going to be on by default for customers
Ok, I undrstand. So let's do the opposite, during startap enable initialize DebuggerManager only if one of the products is enabled (== true)
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.
Fixed in f65a260d976c7b380e2e166f6cb689018d5b6cf4
| return _processExit.Task; | ||
| } | ||
|
|
||
| OneTimeSetup(tracerSettings); |
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.
won't be on by default if di is explicitly disabled
Yeah, the concern is that's basically never going to be the case, so it's effectively still always going to be on by default for customers
| SymbolDatabaseUploadEnabled = config.WithKeys(ConfigurationKeys.Debugger.SymbolDatabaseUploadEnabled).AsBool(true) | ||
| && _internalDynamicInstrumentationEnabled != 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's important that the default value reflects the actual value, so we shouldn't manipulate the value afterwards. e.g. the following is equivalent, but records the "real" default value
| SymbolDatabaseUploadEnabled = config.WithKeys(ConfigurationKeys.Debugger.SymbolDatabaseUploadEnabled).AsBool(true) | |
| && _internalDynamicInstrumentationEnabled != false; | |
| SymbolDatabaseUploadEnabled = config.WithKeys(ConfigurationKeys.Debugger.SymbolDatabaseUploadEnabled).AsBool(_internalDynamicInstrumentationEnabled != 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.
Fixed in 256bd507d924cc973d55d3bb5467e6c7e5570cf3
| } | ||
| public bool CodeOriginForSpansEnabled => _internalCodeOriginForSpansEnabled == true || DynamicSettings.CodeOriginEnabled == true; | ||
|
|
||
| public bool CodeOriginForSpansCanBeEnabled => _internalCodeOriginForSpansEnabled != false && DynamicSettings.CodeOriginEnabled != 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.
I don't think "can be enabled" should be controlled by dynamic config? 🤔:
| public bool CodeOriginForSpansCanBeEnabled => _internalCodeOriginForSpansEnabled != false && DynamicSettings.CodeOriginEnabled != false; | |
| public bool CodeOriginForSpansCanBeEnabled => _internalCodeOriginForSpansEnabled != 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.
Fixed in 256bd507d924cc973d55d3bb5467e6c7e5570cf3
| var enabledResult = config.WithKeys(ConfigurationKeys.Debugger.ExceptionReplayEnabled).AsBool(); | ||
| Enabled = enabledResult ?? false; | ||
| CanBeEnabled = enabledResult != 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.
This doesn't record the correct value, so you need to use AsBoolResult() and then call WithDefault() etc afterwards
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.
Fixed in 256bd507d924cc973d55d3bb5467e6c7e5570cf3
e317a6d to
480ab96
Compare
480ab96 to
96c7641
Compare
…nly when di might be enabled
…code origin tag (CO is on when DI is on)
This reverts commit b7586c6.
…gerManagerTests.cs
cdbc59a to
0808bc4
Compare
## Summary of changes This [PR ](#7366) breaks ER system tests because it ommits an obosolete environment variable key, this PR fix that by returning that key.
## Summary of changes This PR finalizes the work introduced in [Debugger In-Product Enablement](#7366) and [RC Multi-Config Support](#7536) by integrating the new debugger dynamic configuration keys into both the dynamic configuration source and the APM tracing merger. ## Test coverage DebuggerManagerDynamicTests
Summary of changes
This PR adds remote enablement support for debugger products.
depends on #7441
Reason for change
Currently, the only option to enable DI (for example) is by setting an environment variable. We want to give customers the ability to enable products on demand.
Implementation details
Gets RC events with new configuration and update the products based on defined rules.
Test coverage
All existing tests
New system tests (DataDog/system-tests#4991)
DebuggerManagerTests
DebuggerManagerDynamicTests