-
Notifications
You must be signed in to change notification settings - Fork 150
[Config Registry] 7/8 Integration names to keys #7937
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
base: anna/config-inversion-prevent-direct-env-use-6
Are you sure you want to change the base?
[Config Registry] 7/8 Integration names to keys #7937
Conversation
dad2367 to
f18f8e4
Compare
e1c988c to
ccacb2d
Compare
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (7937) and master.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Metric | Master (Mean ± 95% CI) | Current (Mean ± 95% CI) | Change | Status |
|---|---|---|---|---|
| .NET Framework 4.8 - Bailout | ||||
| duration | 72.33 ± (72.16 - 72.38) ms | 77.61 ± (77.46 - 77.81) ms | +7.3% | ❌⬆️ |
Full Metrics Comparison
FakeDbCommand
| Metric | Master (Mean ± 95% CI) | Current (Mean ± 95% CI) | Change | Status |
|---|---|---|---|---|
| .NET Framework 4.8 - Baseline | ||||
| duration | 68.20 ± (68.25 - 68.43) ms | 72.84 ± (72.78 - 73.07) ms | +6.8% | ✅⬆️ |
| .NET Framework 4.8 - Bailout | ||||
| duration | 72.33 ± (72.16 - 72.38) ms | 77.61 ± (77.46 - 77.81) ms | +7.3% | ❌⬆️ |
| .NET Framework 4.8 - CallTarget+Inlining+NGEN | ||||
| duration | 1002.46 ± (1007.75 - 1017.31) ms | 1046.95 ± (1048.12 - 1053.32) ms | +4.4% | ✅⬆️ |
| .NET Core 3.1 - Baseline | ||||
| process.internal_duration_ms | 21.82 ± (21.80 - 21.85) ms | 22.65 ± (22.61 - 22.69) ms | +3.8% | ✅⬆️ |
| process.time_to_main_ms | 78.59 ± (78.45 - 78.73) ms | 85.46 ± (85.25 - 85.67) ms | +8.7% | ✅⬆️ |
| runtime.dotnet.exceptions.count | 0 ± (0 - 0) | 0 ± (0 - 0) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 10.91 ± (10.90 - 10.91) MB | 10.91 ± (10.90 - 10.91) MB | +0.0% | ✅⬆️ |
| runtime.dotnet.threads.count | 12 ± (12 - 12) | 12 ± (12 - 12) | +0.0% | ✅ |
| .NET Core 3.1 - Bailout | ||||
| process.internal_duration_ms | 21.83 ± (21.80 - 21.87) ms | 22.61 ± (22.56 - 22.65) ms | +3.6% | ✅⬆️ |
| process.time_to_main_ms | 79.72 ± (79.63 - 79.81) ms | 86.96 ± (86.74 - 87.18) ms | +9.1% | ✅⬆️ |
| runtime.dotnet.exceptions.count | 0 ± (0 - 0) | 0 ± (0 - 0) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 10.94 ± (10.94 - 10.95) MB | 10.95 ± (10.94 - 10.95) MB | +0.0% | ✅⬆️ |
| runtime.dotnet.threads.count | 13 ± (13 - 13) | 13 ± (13 - 13) | +0.0% | ✅ |
| .NET Core 3.1 - CallTarget+Inlining+NGEN | ||||
| process.internal_duration_ms | 241.80 ± (238.51 - 245.08) ms | 259.81 ± (255.72 - 263.91) ms | +7.5% | ✅⬆️ |
| process.time_to_main_ms | 468.78 ± (468.25 - 469.31) ms | 522.91 ± (517.69 - 528.13) ms | +11.5% | ✅⬆️ |
| runtime.dotnet.exceptions.count | 0 ± (0 - 0) | 0 ± (0 - 0) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 48.39 ± (48.37 - 48.42) MB | 48.35 ± (48.32 - 48.37) MB | -0.1% | ✅ |
| runtime.dotnet.threads.count | 28 ± (28 - 28) | 28 ± (28 - 28) | -0.6% | ✅ |
| .NET 6 - Baseline | ||||
| process.internal_duration_ms | 20.64 ± (20.61 - 20.66) ms | 22.10 ± (22.05 - 22.14) ms | +7.1% | ✅⬆️ |
| process.time_to_main_ms | 68.12 ± (68.00 - 68.25) ms | 76.24 ± (76.04 - 76.44) ms | +11.9% | ✅⬆️ |
| runtime.dotnet.exceptions.count | 0 ± (0 - 0) | 0 ± (0 - 0) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 10.61 ± (10.60 - 10.61) MB | 10.64 ± (10.64 - 10.64) MB | +0.3% | ✅⬆️ |
| runtime.dotnet.threads.count | 10 ± (10 - 10) | 10 ± (10 - 10) | +0.0% | ✅ |
| .NET 6 - Bailout | ||||
| process.internal_duration_ms | 20.62 ± (20.60 - 20.64) ms | 21.98 ± (21.94 - 22.03) ms | +6.6% | ✅⬆️ |
| process.time_to_main_ms | 69.11 ± (69.05 - 69.17) ms | 77.39 ± (77.21 - 77.58) ms | +12.0% | ✅⬆️ |
| runtime.dotnet.exceptions.count | 0 ± (0 - 0) | 0 ± (0 - 0) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 10.66 ± (10.65 - 10.66) MB | 10.67 ± (10.67 - 10.68) MB | +0.2% | ✅⬆️ |
| runtime.dotnet.threads.count | 11 ± (11 - 11) | 11 ± (11 - 11) | +0.0% | ✅ |
| .NET 6 - CallTarget+Inlining+NGEN | ||||
| process.internal_duration_ms | 244.21 ± (241.80 - 246.61) ms | 232.20 ± (228.21 - 236.19) ms | -4.9% | ✅ |
| process.time_to_main_ms | 445.79 ± (445.34 - 446.25) ms | 470.77 ± (470.04 - 471.50) ms | +5.6% | ✅⬆️ |
| runtime.dotnet.exceptions.count | 0 ± (0 - 0) | 0 ± (0 - 0) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 49.08 ± (49.05 - 49.11) MB | 49.17 ± (49.14 - 49.20) MB | +0.2% | ✅⬆️ |
| runtime.dotnet.threads.count | 28 ± (28 - 28) | 28 ± (28 - 28) | +0.2% | ✅⬆️ |
| .NET 8 - Baseline | ||||
| process.internal_duration_ms | 18.78 ± (18.75 - 18.81) ms | 19.44 ± (19.40 - 19.48) ms | +3.5% | ✅⬆️ |
| process.time_to_main_ms | 67.21 ± (67.09 - 67.33) ms | 73.05 ± (72.90 - 73.19) ms | +8.7% | ✅⬆️ |
| runtime.dotnet.exceptions.count | 0 ± (0 - 0) | 0 ± (0 - 0) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 7.65 ± (7.65 - 7.66) MB | 7.66 ± (7.65 - 7.67) MB | +0.1% | ✅⬆️ |
| runtime.dotnet.threads.count | 10 ± (10 - 10) | 10 ± (10 - 10) | +0.0% | ✅ |
| .NET 8 - Bailout | ||||
| process.internal_duration_ms | 18.83 ± (18.80 - 18.85) ms | 19.42 ± (19.38 - 19.47) ms | +3.2% | ✅⬆️ |
| process.time_to_main_ms | 68.47 ± (68.40 - 68.55) ms | 74.04 ± (73.87 - 74.21) ms | +8.1% | ✅⬆️ |
| runtime.dotnet.exceptions.count | 0 ± (0 - 0) | 0 ± (0 - 0) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 7.71 ± (7.70 - 7.72) MB | 7.72 ± (7.71 - 7.73) MB | +0.1% | ✅⬆️ |
| runtime.dotnet.threads.count | 11 ± (11 - 11) | 11 ± (11 - 11) | +0.0% | ✅ |
| .NET 8 - CallTarget+Inlining+NGEN | ||||
| process.internal_duration_ms | 177.28 ± (176.44 - 178.13) ms | 187.68 ± (186.73 - 188.63) ms | +5.9% | ✅⬆️ |
| process.time_to_main_ms | 428.74 ± (428.08 - 429.40) ms | 456.92 ± (456.06 - 457.78) ms | +6.6% | ✅⬆️ |
| runtime.dotnet.exceptions.count | 0 ± (0 - 0) | 0 ± (0 - 0) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 36.49 ± (36.46 - 36.53) MB | 36.74 ± (36.70 - 36.77) MB | +0.7% | ✅⬆️ |
| runtime.dotnet.threads.count | 27 ± (27 - 27) | 27 ± (27 - 27) | -0.3% | ✅ |
HttpMessageHandler
| Metric | Master (Mean ± 95% CI) | Current (Mean ± 95% CI) | Change | Status |
|---|---|---|---|---|
| .NET Framework 4.8 - Baseline | ||||
| duration | 192.14 ± (191.92 - 192.71) ms | 192.54 ± (192.45 - 193.25) ms | +0.2% | ✅⬆️ |
| .NET Framework 4.8 - Bailout | ||||
| duration | 195.23 ± (194.93 - 195.43) ms | 195.79 ± (195.83 - 196.60) ms | +0.3% | ✅⬆️ |
| .NET Framework 4.8 - CallTarget+Inlining+NGEN | ||||
| duration | 1115.71 ± (1119.45 - 1129.34) ms | 1135.72 ± (1141.66 - 1150.65) ms | +1.8% | ✅⬆️ |
| .NET Core 3.1 - Baseline | ||||
| process.internal_duration_ms | 186.16 ± (185.79 - 186.52) ms | 186.57 ± (186.19 - 186.95) ms | +0.2% | ✅⬆️ |
| process.time_to_main_ms | 80.45 ± (80.26 - 80.64) ms | 80.55 ± (80.36 - 80.75) ms | +0.1% | ✅⬆️ |
| runtime.dotnet.exceptions.count | 3 ± (3 - 3) | 3 ± (3 - 3) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 16.13 ± (16.10 - 16.16) MB | 16.08 ± (16.05 - 16.11) MB | -0.3% | ✅ |
| runtime.dotnet.threads.count | 20 ± (20 - 20) | 20 ± (20 - 20) | -0.9% | ✅ |
| .NET Core 3.1 - Bailout | ||||
| process.internal_duration_ms | 186.36 ± (186.07 - 186.66) ms | 185.80 ± (185.46 - 186.13) ms | -0.3% | ✅ |
| process.time_to_main_ms | 81.59 ± (81.45 - 81.73) ms | 81.56 ± (81.42 - 81.70) ms | -0.0% | ✅ |
| runtime.dotnet.exceptions.count | 3 ± (3 - 3) | 3 ± (3 - 3) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 16.15 ± (16.12 - 16.17) MB | 16.19 ± (16.16 - 16.22) MB | +0.3% | ✅⬆️ |
| runtime.dotnet.threads.count | 21 ± (21 - 21) | 21 ± (21 - 21) | -0.6% | ✅ |
| .NET Core 3.1 - CallTarget+Inlining+NGEN | ||||
| process.internal_duration_ms | 429.31 ± (426.42 - 432.21) ms | 438.98 ± (435.84 - 442.12) ms | +2.3% | ✅⬆️ |
| process.time_to_main_ms | 469.73 ± (469.21 - 470.25) ms | 499.02 ± (495.19 - 502.86) ms | +6.2% | ✅⬆️ |
| runtime.dotnet.exceptions.count | 3 ± (3 - 3) | 3 ± (3 - 3) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 58.82 ± (58.70 - 58.94) MB | 58.88 ± (58.75 - 59.00) MB | +0.1% | ✅⬆️ |
| runtime.dotnet.threads.count | 29 ± (29 - 29) | 29 ± (29 - 29) | -0.0% | ✅ |
| .NET 6 - Baseline | ||||
| process.internal_duration_ms | 190.82 ± (190.38 - 191.26) ms | 191.45 ± (191.08 - 191.83) ms | +0.3% | ✅⬆️ |
| process.time_to_main_ms | 70.07 ± (69.86 - 70.27) ms | 70.04 ± (69.86 - 70.21) ms | -0.0% | ✅ |
| runtime.dotnet.exceptions.count | 4 ± (4 - 4) | 4 ± (4 - 4) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 15.85 ± (15.68 - 16.01) MB | 16.17 ± (16.03 - 16.30) MB | +2.0% | ✅⬆️ |
| runtime.dotnet.threads.count | 18 ± (18 - 18) | 18 ± (18 - 19) | +1.8% | ✅⬆️ |
| .NET 6 - Bailout | ||||
| process.internal_duration_ms | 189.58 ± (189.34 - 189.82) ms | 191.11 ± (190.70 - 191.52) ms | +0.8% | ✅⬆️ |
| process.time_to_main_ms | 70.86 ± (70.74 - 70.98) ms | 71.19 ± (71.06 - 71.32) ms | +0.5% | ✅⬆️ |
| runtime.dotnet.exceptions.count | 4 ± (4 - 4) | 4 ± (4 - 4) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 15.87 ± (15.70 - 16.05) MB | 16.20 ± (16.07 - 16.33) MB | +2.1% | ✅⬆️ |
| runtime.dotnet.threads.count | 19 ± (18 - 19) | 19 ± (19 - 20) | +3.9% | ✅⬆️ |
| .NET 6 - CallTarget+Inlining+NGEN | ||||
| process.internal_duration_ms | 438.58 ± (434.75 - 442.41) ms | 443.96 ± (440.73 - 447.19) ms | +1.2% | ✅⬆️ |
| process.time_to_main_ms | 448.69 ± (448.17 - 449.20) ms | 455.32 ± (454.70 - 455.94) ms | +1.5% | ✅⬆️ |
| runtime.dotnet.exceptions.count | 4 ± (4 - 4) | 4 ± (4 - 4) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 58.77 ± (58.63 - 58.91) MB | 58.95 ± (58.81 - 59.09) MB | +0.3% | ✅⬆️ |
| runtime.dotnet.threads.count | 29 ± (29 - 29) | 29 ± (29 - 29) | -0.1% | ✅ |
| .NET 8 - Baseline | ||||
| process.internal_duration_ms | 188.47 ± (188.17 - 188.77) ms | 189.30 ± (188.93 - 189.68) ms | +0.4% | ✅⬆️ |
| process.time_to_main_ms | 69.13 ± (69.00 - 69.26) ms | 69.51 ± (69.34 - 69.67) ms | +0.5% | ✅⬆️ |
| runtime.dotnet.exceptions.count | 4 ± (4 - 4) | 4 ± (4 - 4) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 11.78 ± (11.74 - 11.83) MB | 11.79 ± (11.77 - 11.81) MB | +0.1% | ✅⬆️ |
| runtime.dotnet.threads.count | 18 ± (18 - 18) | 18 ± (18 - 18) | +0.1% | ✅⬆️ |
| .NET 8 - Bailout | ||||
| process.internal_duration_ms | 187.96 ± (187.62 - 188.31) ms | 188.35 ± (188.08 - 188.61) ms | +0.2% | ✅⬆️ |
| process.time_to_main_ms | 70.42 ± (70.30 - 70.54) ms | 70.66 ± (70.55 - 70.77) ms | +0.3% | ✅⬆️ |
| runtime.dotnet.exceptions.count | 4 ± (4 - 4) | 4 ± (4 - 4) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 11.82 ± (11.79 - 11.85) MB | 11.88 ± (11.85 - 11.91) MB | +0.5% | ✅⬆️ |
| runtime.dotnet.threads.count | 19 ± (19 - 19) | 19 ± (19 - 19) | -0.1% | ✅ |
| .NET 8 - CallTarget+Inlining+NGEN | ||||
| process.internal_duration_ms | 364.26 ± (362.82 - 365.69) ms | 363.37 ± (361.56 - 365.17) ms | -0.2% | ✅ |
| process.time_to_main_ms | 433.43 ± (432.77 - 434.09) ms | 438.83 ± (438.15 - 439.52) ms | +1.2% | ✅⬆️ |
| runtime.dotnet.exceptions.count | 4 ± (4 - 4) | 4 ± (4 - 4) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 48.10 ± (48.05 - 48.14) MB | 48.35 ± (48.32 - 48.39) MB | +0.5% | ✅⬆️ |
| runtime.dotnet.threads.count | 29 ± (29 - 29) | 28 ± (28 - 29) | -0.7% | ✅ |
Comparison explanation
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 highlighted in **red**. The following thresholds were used for comparing the execution times:
- Welch test with statistical test for significance of 5%
- Only results indicating a difference greater than 5% and 5 ms are considered.
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).
Duration charts
FakeDbCommand (.NET Framework 4.8)
gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7937) - mean (73ms) : 71, 75
master - mean (68ms) : 67, 70
section Bailout
This PR (7937) - mean (78ms) : crit, 76, 79
master - mean (72ms) : 71, 73
section CallTarget+Inlining+NGEN
This PR (7937) - mean (1,051ms) : 1013, 1088
master - mean (1,013ms) : 943, 1082
FakeDbCommand (.NET Core 3.1)
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7937) - mean (115ms) : 112, 118
master - mean (106ms) : 103, 108
section Bailout
This PR (7937) - mean (116ms) : crit, 114, 119
master - mean (107ms) : 105, 108
section CallTarget+Inlining+NGEN
This PR (7937) - mean (842ms) : crit, 661, 1024
master - mean (736ms) : 685, 786
FakeDbCommand (.NET 6)
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7937) - mean (105ms) : 101, 110
master - mean (94ms) : 92, 96
section Bailout
This PR (7937) - mean (106ms) : crit, 104, 109
master - mean (94ms) : 93, 95
section CallTarget+Inlining+NGEN
This PR (7937) - mean (732ms) : 657, 807
master - mean (715ms) : 680, 750
FakeDbCommand (.NET 8)
gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7937) - mean (100ms) : 97, 103
master - mean (92ms) : 90, 95
section Bailout
This PR (7937) - mean (101ms) : crit, 99, 104
master - mean (93ms) : 92, 95
section CallTarget+Inlining+NGEN
This PR (7937) - mean (674ms) : crit, 656, 692
master - mean (636ms) : 622, 651
HttpMessageHandler (.NET Framework 4.8)
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7937) - mean (193ms) : 189, 197
master - mean (192ms) : 188, 196
section Bailout
This PR (7937) - mean (196ms) : 193, 200
master - mean (195ms) : 193, 198
section CallTarget+Inlining+NGEN
This PR (7937) - mean (1,146ms) : 1082, 1210
master - mean (1,124ms) : 1053, 1195
HttpMessageHandler (.NET Core 3.1)
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7937) - mean (275ms) : 269, 281
master - mean (275ms) : 270, 280
section Bailout
This PR (7937) - mean (275ms) : 271, 280
master - mean (276ms) : 271, 280
section CallTarget+Inlining+NGEN
This PR (7937) - mean (967ms) : 870, 1064
master - mean (926ms) : 880, 972
HttpMessageHandler (.NET 6)
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7937) - mean (270ms) : 265, 275
master - mean (269ms) : 264, 274
section Bailout
This PR (7937) - mean (270ms) : 265, 275
master - mean (268ms) : 265, 272
section CallTarget+Inlining+NGEN
This PR (7937) - mean (928ms) : 882, 974
master - mean (916ms) : 855, 976
HttpMessageHandler (.NET 8)
gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (7937) - mean (269ms) : 261, 276
master - mean (268ms) : 263, 272
section Bailout
This PR (7937) - mean (269ms) : 265, 273
master - mean (268ms) : 263, 273
section CallTarget+Inlining+NGEN
This PR (7937) - mean (834ms) : 811, 857
master - mean (829ms) : 811, 847
f18f8e4 to
303b602
Compare
ccacb2d to
cf34d67
Compare
303b602 to
70df7c9
Compare
cf34d67 to
c8caa83
Compare
|
…nes in the whole solution (#7931) (This PR is actually #7697 that accidentally got closed) ## Context Part of **Configuration Inversion (Step 5)** - Stack progress: 1. [#7548](#7548) - Add GitLab step and JSON configuration file 2. [#7688](#7688) - Cleanup configuration / platform keys + source generator 3. [#7698](#7698) - Aliases handling via source generator 4. [#7689](#7689) - Analyzers for platform and ConfigurationBuilder 5. **→ [#7931](#7697) - Replace manual ConfigurationKeys by generated ones in the whole solution (this PR)** 6. [#7932](#7932) - Forbid use of System.Environment methods and adapt everywhere 7. [#7937](#7937) - Integration names to generated keys ## Summary of changes Fixed the [ConfigurationKeysGenerator](cci:2://file:///Users/anna.yafi/go/src/github.com/DataDog/dd-trace-dotnet3/tracer/src/Datadog.Trace.SourceGenerators/Configuration/ConfigurationKeysGenerator.cs:21:0-958:1) to properly read and apply the [configuration_keys_mapping.json](cci:7://file:///Users/anna.yafi/go/src/github.com/DataDog/dd-trace-dotnet3/tracer/src/Datadog.Trace/Configuration/configuration_keys_mapping.json:0:0-0:0) file, and extracted common file header comments to a reusable constant. **Key changes:** - Fixed JSON array extraction in [ParseMappingFile](cci:1://file:///Users/anna.yafi/go/src/github.com/DataDog/dd-trace-dotnet3/tracer/src/Datadog.Trace.SourceGenerators/Configuration/ConfigurationKeysGenerator.cs:350:4-524:5) method - the generator was incorrectly trying to extract the `"mappings"` field as an object instead of an array - Extracted configuration generator comments to `Constants.ConfigurationGeneratorComment` for reuse across multiple generators - Updated both [ConfigurationKeysGenerator](cci:2://file:///Users/anna.yafi/go/src/github.com/DataDog/dd-trace-dotnet3/tracer/src/Datadog.Trace.SourceGenerators/Configuration/ConfigurationKeysGenerator.cs:21:0-958:1) and [ConfigKeyAliasesSwitcherGenerator](cci:2://file:///Users/anna.yafi/go/src/github.com/DataDog/dd-trace-dotnet3/tracer/src/Datadog.Trace.SourceGenerators/Configuration/ConfigKeyAliasesSwitcherGenerator.cs:24:0-340:1) to use the shared constant - Added documentation as to how to add a key now ## Reason for change The [configuration_keys_mapping.json](cci:7://file:///Users/anna.yafi/go/src/github.com/DataDog/dd-trace-dotnet3/tracer/src/Datadog.Trace/Configuration/configuration_keys_mapping.json:0:0-0:0) file was being ignored during code generation, causing the generator to produce incorrect constant names. This meant that any manual edits to constant names in the mapping file were not being respected, and the generated code would use auto-generated names instead of the explicitly mapped ones. Additionally, the file header comments explaining that files are auto-generated were duplicated across generators, violating DRY principles. ## Implementation details 1. **Fixed array extraction logic**: Replaced the call to [JsonReader.ExtractJsonObjectSection()](cci:1://file:///Users/anna.yafi/go/src/github.com/DataDog/dd-trace-dotnet3/tracer/src/Datadog.Trace.SourceGenerators/Configuration/JsonReader.cs:16:4-91:5) with custom array extraction code that: - Finds the `"mappings":` key in the JSON - Locates the opening `[` bracket - Tracks bracket nesting to find the matching closing `]` - Extracts the complete array content 2. **Resolved variable scope issue**: Reused existing `inString` and `escapeNext` variables from the outer scope instead of redeclaring them, fixing compilation errors. 3. **Centralized header comments**: Created `Constants.ConfigurationGeneratorComment` containing the standardized auto-generation notice and updated both generators to use it. ## Test coverage - Verified the generator builds successfully without errors - The mapping file is now properly parsed and applied during code generation - Generated constant names now match the mappings defined in [configuration_keys_mapping.json](cci:7://file:///Users/anna.yafi/go/src/github.com/DataDog/dd-trace-dotnet3/tracer/src/Datadog.Trace/Configuration/configuration_keys_mapping.json:0:0-0:0) ## Other details This fix ensures that the explicit naming conventions defined in [configuration_keys_mapping.json](cci:7://file:///Users/anna.yafi/go/src/github.com/DataDog/dd-trace-dotnet3/tracer/src/Datadog.Trace/Configuration/configuration_keys_mapping.json:0:0-0:0) are respected, maintaining consistency with the existing codebase and preventing future confusion when constant names don't match their expected values. --------- Co-authored-by: Andrew Lock <[email protected]>
70df7c9 to
d197570
Compare
c8caa83 to
85c37d3
Compare
d197570 to
272e31a
Compare
85c37d3 to
bea964f
Compare
272e31a to
c89c676
Compare
bea964f to
d62b438
Compare
c89c676 to
41c88a5
Compare
d62b438 to
ae8195d
Compare
41c88a5 to
e091036
Compare
ae8195d to
2a87981
Compare
e091036 to
b7ac294
Compare
2a87981 to
c14af6e
Compare
b7ac294 to
f164b06
Compare
c14af6e to
99e80f7
Compare
f164b06 to
756c6b3
Compare
99e80f7 to
0d9d4be
Compare
756c6b3 to
60360fa
Compare
7e8b0c6 to
8461312
Compare
2b3c655 to
0b49864
Compare
8461312 to
be89b22
Compare
9227c7e to
a1ceb97
Compare
353984d to
de53524
Compare
0af9145 to
d16d6c6
Compare
26977ff to
db36d61
Compare
d16d6c6 to
ff1db4e
Compare
db36d61 to
46a9848
Compare
ff1db4e to
1bb6dc6
Compare
andrewlock
left a comment
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.
Looks good in principal, but I think we need to make some tweaks to the generated code
| context.AddSource(enumToGenerate.ExtensionsName + "_EnumExtensions.g.cs", SourceText.From(result, Encoding.UTF8)); | ||
|
|
||
| // If this is the IntegrationId enum, also generate IntegrationNameToKeys | ||
| if (enumToGenerate.FullyQualifiedName == "Datadog.Trace.Configuration.IntegrationId") |
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 feels so hacky, but I love it 😂
| sb.AppendLine(Constants.FileHeader); | ||
| sb.AppendLine("namespace Datadog.Trace.Configuration"); | ||
| sb.AppendLine("{"); | ||
| sb.AppendLine(" /// <summary>"); |
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 looks neat, but doing these as individual sb.AppendLine is very expensive. You want to do as big a block as you can in general, so raw strings are the best option generally.
e.g.
sb.AppendLine(
Constants.FileHeader + // as long as this is a constant, the concat happens at compile time
"""
namespace Datadog.Trace.Configuration
{
/// Generated mapping of integration names to their configuration keys.");
/// </summary>");
internal static partial class IntegrationNameToKeys");
{
private const string ObsoleteMessage = DeprecationMessages.AppAnalytics;");
/// <summary>");
/// All integration enabled keys (canonical + aliases).");
/// </summary>");
public static readonly string[] AllIntegrationEnabledKeys = new[]");
{
""");etc
It also means you don't need to escape anything, just copy-paste the code you expect in there, and "break out" where you need something specific (like the actual values) 🙂
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.
Ok! chhanged in e33db01
| // Single loop to build array keys and all switch cases | ||
| foreach (var member in enumToGenerate.Names) |
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, I would have just iterated it 3 times, and append to the same stringbuilder instead of having multiple builders. The iteration is low overhead compared to the stringbuilder (which will each have to grow their internal array multiple times)
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.
yep thank you changed in e33db01
| sb.AppendLine(" {"); | ||
| sb.AppendLine(" string.Format(\"DD_TRACE_{0}_ENABLED\", integrationName.ToUpperInvariant()),"); | ||
| sb.AppendLine(" string.Format(\"DD_TRACE_{0}_ENABLED\", integrationName),"); | ||
| sb.AppendLine(" $\"DD_{integrationName}_ENABLED\""); | ||
| sb.AppendLine(" }"); |
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 an interesting one tbh, do we even need this branch 🤔 Given we handle this at compile time, we know it will never be called. That said, throwing probably isn't great.
We might want to generate something more like this:
_ => GetEnabledFallbackFor(integrationName),because it should generate smaller code, which has more chance of optimization, and the fallback will never be invoked,
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 changed in e33db01
| _source, | ||
| _telemetry, | ||
| integrationEnabledKeys[0], | ||
| integrationEnabledKeys.Skip(1).ToArray()); |
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.
oh no, Skip(1).ToArray() 🙁 We definitely don't want to do that, If we're going to need to do that, we should change the GetIntegrationEnabledKeys() API to return an array of the correct size (e.g. a "main" key plus an array of fallbacks).
If possible we should probably try to get away from using arrays at all... we may be able to use ReadOnlySpan<string> in some places, or we should use InlineArray (if we can), or worse case we should create a specialized type to avoid creating all these small arrays if possible.
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.
very good point fixed in e33db01
| { | ||
| return integrationName switch | ||
| { | ||
| "HttpMessageHandler" => new[] { "DD_TRACE_HTTPMESSAGEHANDLER_ENABLED", "DD_TRACE_HttpMessageHandler_ENABLED", "DD_HttpMessageHandler_ENABLED" }, |
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 use collection expressions wherever possible
| "HttpMessageHandler" => new[] { "DD_TRACE_HTTPMESSAGEHANDLER_ENABLED", "DD_TRACE_HttpMessageHandler_ENABLED", "DD_HttpMessageHandler_ENABLED" }, | |
| "HttpMessageHandler" => ["DD_TRACE_HTTPMESSAGEHANDLER_ENABLED", "DD_TRACE_HttpMessageHandler_ENABLED", "DD_HttpMessageHandler_ENABLED"], |
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 e33db01
| /// <summary> | ||
| /// All integration enabled keys (canonical + aliases). | ||
| /// </summary> | ||
| public static readonly string[] AllIntegrationEnabledKeys = new[] |
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.
AFAICT, this is only called in a test. But the array will be created as soon as we call IntegrationNameToKeys.Something. We can solve that by making it a method instead (and also marking it as only to be called in tests)
| public static readonly string[] AllIntegrationEnabledKeys = new[] | |
| [TestingOnly] | |
| public static readonly string[] GetAllIntegrationEnabledKeys() => new[] |
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.
added in e33db01
e33db01 to
472f9f9
Compare
472f9f9 to
57ec51a
Compare
Context
Part of Configuration Inversion (Step 6) - Stack progress:
Summary of changes
Extends the
EnumExtensionsGeneratorto generate IntegrationNameToKeys class for theIntegrationIdenum, providing centralized configuration key mapping for integrations including enabled, analytics enabled, and analytics sample rate keys. Also fixes AdoNet instrumentation compilation error.Reason for change
Implementation details
EnumExtensionsGeneratorwith GenerateIntegrationNameToKeys method that generates:AllIntegrationEnabledKeysarray containing all enabled configuration keysIntegrationEnabledKeyPattern,AnalyticsEnabledKeyPattern,AnalyticsSampleRateKeyPattern)Test coverage
IntegrationIdenum