Add srv_src field to client stats payload#8339
Add srv_src field to client stats payload#8339NachoEchevarria wants to merge 35 commits intomasterfrom
Conversation
BenchmarksBenchmark execution time: 2026-03-27 17:20:18 Comparing candidate commit ab85a20 in PR branch Found 1 performance improvements and 8 performance regressions! Performance is the same for 264 metrics, 15 unstable metrics.
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8339) and master.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Metric | Master (Mean ± 95% CI) | Current (Mean ± 95% CI) | Change | Status |
|---|---|---|---|---|
| .NET Framework 4.8 - Baseline | ||||
| duration | 193.46 ± (193.25 - 194.07) ms | 220.36 ± (219.82 - 221.33) ms | +13.9% | ❌⬆️ |
| .NET Framework 4.8 - Bailout | ||||
| duration | 196.16 ± (196.05 - 196.84) ms | 232.81 ± (232.17 - 234.83) ms | +18.7% | ❌⬆️ |
| .NET Framework 4.8 - CallTarget+Inlining+NGEN | ||||
| duration | 1150.65 ± (1151.26 - 1157.73) ms | 1263.45 ± (1263.90 - 1273.02) ms | +9.8% | ❌⬆️ |
Full Metrics Comparison
FakeDbCommand
| Metric | Master (Mean ± 95% CI) | Current (Mean ± 95% CI) | Change | Status |
|---|---|---|---|---|
| .NET Framework 4.8 - Baseline | ||||
| duration | 72.15 ± (72.15 - 72.46) ms | 71.82 ± (71.86 - 72.21) ms | -0.5% | ✅ |
| .NET Framework 4.8 - Bailout | ||||
| duration | 76.39 ± (76.17 - 76.50) ms | 76.51 ± (76.26 - 76.60) ms | +0.2% | ✅⬆️ |
| .NET Framework 4.8 - CallTarget+Inlining+NGEN | ||||
| duration | 1073.18 ± (1072.51 - 1078.17) ms | 1077.76 ± (1077.77 - 1085.43) ms | +0.4% | ✅⬆️ |
| .NET Core 3.1 - Baseline | ||||
| process.internal_duration_ms | 22.40 ± (22.35 - 22.45) ms | 22.29 ± (22.26 - 22.33) ms | -0.5% | ✅ |
| process.time_to_main_ms | 83.68 ± (83.44 - 83.92) ms | 83.17 ± (82.97 - 83.36) ms | -0.6% | ✅ |
| runtime.dotnet.exceptions.count | 0 ± (0 - 0) | 0 ± (0 - 0) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 10.91 ± (10.91 - 10.91) MB | 10.92 ± (10.91 - 10.92) MB | +0.1% | ✅⬆️ |
| runtime.dotnet.threads.count | 12 ± (12 - 12) | 12 ± (12 - 12) | +0.0% | ✅ |
| .NET Core 3.1 - Bailout | ||||
| process.internal_duration_ms | 22.25 ± (22.21 - 22.29) ms | 22.32 ± (22.29 - 22.36) ms | +0.3% | ✅⬆️ |
| process.time_to_main_ms | 84.98 ± (84.78 - 85.18) ms | 85.18 ± (84.96 - 85.40) ms | +0.2% | ✅⬆️ |
| 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.95 - 10.96) MB | +0.1% | ✅⬆️ |
| runtime.dotnet.threads.count | 13 ± (13 - 13) | 13 ± (13 - 13) | +0.0% | ✅ |
| .NET Core 3.1 - CallTarget+Inlining+NGEN | ||||
| process.internal_duration_ms | 223.85 ± (222.81 - 224.88) ms | 222.19 ± (221.08 - 223.30) ms | -0.7% | ✅ |
| process.time_to_main_ms | 533.57 ± (532.31 - 534.83) ms | 533.84 ± (532.68 - 535.00) ms | +0.1% | ✅⬆️ |
| runtime.dotnet.exceptions.count | 0 ± (0 - 0) | 0 ± (0 - 0) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 48.32 ± (48.29 - 48.35) MB | 48.25 ± (48.22 - 48.28) MB | -0.1% | ✅ |
| runtime.dotnet.threads.count | 28 ± (28 - 28) | 28 ± (28 - 28) | -1.1% | ✅ |
| .NET 6 - Baseline | ||||
| process.internal_duration_ms | 21.00 ± (20.97 - 21.02) ms | 20.99 ± (20.96 - 21.02) ms | -0.0% | ✅ |
| process.time_to_main_ms | 72.14 ± (71.99 - 72.29) ms | 72.06 ± (71.91 - 72.21) ms | -0.1% | ✅ |
| runtime.dotnet.exceptions.count | 0 ± (0 - 0) | 0 ± (0 - 0) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 10.63 ± (10.63 - 10.63) MB | 10.64 ± (10.63 - 10.64) MB | +0.1% | ✅⬆️ |
| runtime.dotnet.threads.count | 10 ± (10 - 10) | 10 ± (10 - 10) | +0.0% | ✅ |
| .NET 6 - Bailout | ||||
| process.internal_duration_ms | 21.02 ± (20.99 - 21.06) ms | 20.85 ± (20.81 - 20.89) ms | -0.8% | ✅ |
| process.time_to_main_ms | 73.19 ± (73.01 - 73.37) ms | 72.80 ± (72.64 - 72.97) ms | -0.5% | ✅ |
| runtime.dotnet.exceptions.count | 0 ± (0 - 0) | 0 ± (0 - 0) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 10.73 ± (10.73 - 10.73) MB | 10.75 ± (10.75 - 10.75) MB | +0.2% | ✅⬆️ |
| runtime.dotnet.threads.count | 11 ± (11 - 11) | 11 ± (11 - 11) | +0.0% | ✅ |
| .NET 6 - CallTarget+Inlining+NGEN | ||||
| process.internal_duration_ms | 216.01 ± (214.61 - 217.40) ms | 387.14 ± (384.80 - 389.48) ms | +79.2% | ✅⬆️ |
| process.time_to_main_ms | 530.99 ± (529.64 - 532.35) ms | 531.85 ± (530.91 - 532.79) ms | +0.2% | ✅⬆️ |
| runtime.dotnet.exceptions.count | 0 ± (0 - 0) | 0 ± (0 - 0) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 50.12 ± (50.10 - 50.15) MB | 50.36 ± (50.33 - 50.38) MB | +0.5% | ✅⬆️ |
| runtime.dotnet.threads.count | 29 ± (29 - 29) | 28 ± (28 - 28) | -1.3% | ✅ |
| .NET 8 - Baseline | ||||
| process.internal_duration_ms | 19.14 ± (19.09 - 19.18) ms | 19.35 ± (19.31 - 19.38) ms | +1.1% | ✅⬆️ |
| process.time_to_main_ms | 71.36 ± (71.18 - 71.54) ms | 71.97 ± (71.81 - 72.13) ms | +0.9% | ✅⬆️ |
| runtime.dotnet.exceptions.count | 0 ± (0 - 0) | 0 ± (0 - 0) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 7.69 ± (7.68 - 7.70) MB | 7.68 ± (7.68 - 7.69) MB | -0.1% | ✅ |
| runtime.dotnet.threads.count | 10 ± (10 - 10) | 10 ± (10 - 10) | +0.0% | ✅ |
| .NET 8 - Bailout | ||||
| process.internal_duration_ms | 19.35 ± (19.31 - 19.38) ms | 19.13 ± (19.10 - 19.17) ms | -1.1% | ✅ |
| process.time_to_main_ms | 73.21 ± (73.05 - 73.37) ms | 72.31 ± (72.17 - 72.45) ms | -1.2% | ✅ |
| runtime.dotnet.exceptions.count | 0 ± (0 - 0) | 0 ± (0 - 0) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 7.75 ± (7.74 - 7.76) MB | 7.73 ± (7.72 - 7.73) MB | -0.3% | ✅ |
| runtime.dotnet.threads.count | 11 ± (11 - 11) | 11 ± (11 - 11) | +0.0% | ✅ |
| .NET 8 - CallTarget+Inlining+NGEN | ||||
| process.internal_duration_ms | 160.07 ± (159.25 - 160.89) ms | 308.18 ± (305.85 - 310.50) ms | +92.5% | ✅⬆️ |
| process.time_to_main_ms | 490.08 ± (489.03 - 491.13) ms | 490.98 ± (490.08 - 491.89) ms | +0.2% | ✅⬆️ |
| runtime.dotnet.exceptions.count | 0 ± (0 - 0) | 0 ± (0 - 0) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 36.93 ± (36.90 - 36.95) MB | 37.25 ± (37.23 - 37.27) MB | +0.9% | ✅⬆️ |
| runtime.dotnet.threads.count | 28 ± (28 - 28) | 27 ± (27 - 27) | -1.6% | ✅ |
HttpMessageHandler
| Metric | Master (Mean ± 95% CI) | Current (Mean ± 95% CI) | Change | Status |
|---|---|---|---|---|
| .NET Framework 4.8 - Baseline | ||||
| duration | 193.46 ± (193.25 - 194.07) ms | 220.36 ± (219.82 - 221.33) ms | +13.9% | ❌⬆️ |
| .NET Framework 4.8 - Bailout | ||||
| duration | 196.16 ± (196.05 - 196.84) ms | 232.81 ± (232.17 - 234.83) ms | +18.7% | ❌⬆️ |
| .NET Framework 4.8 - CallTarget+Inlining+NGEN | ||||
| duration | 1150.65 ± (1151.26 - 1157.73) ms | 1263.45 ± (1263.90 - 1273.02) ms | +9.8% | ❌⬆️ |
| .NET Core 3.1 - Baseline | ||||
| process.internal_duration_ms | 187.25 ± (186.89 - 187.62) ms | 213.03 ± (212.41 - 213.65) ms | +13.8% | ✅⬆️ |
| process.time_to_main_ms | 80.70 ± (80.47 - 80.93) ms | 91.28 ± (91.06 - 91.50) ms | +13.1% | ✅⬆️ |
| runtime.dotnet.exceptions.count | 3 ± (3 - 3) | 3 ± (3 - 3) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 16.14 ± (16.11 - 16.17) MB | 15.83 ± (15.82 - 15.85) MB | -1.9% | ✅ |
| runtime.dotnet.threads.count | 20 ± (19 - 20) | 20 ± (20 - 20) | +1.9% | ✅⬆️ |
| .NET Core 3.1 - Bailout | ||||
| process.internal_duration_ms | 186.52 ± (186.21 - 186.82) ms | 211.82 ± (211.16 - 212.47) ms | +13.6% | ✅⬆️ |
| process.time_to_main_ms | 82.09 ± (81.94 - 82.24) ms | 92.87 ± (92.60 - 93.13) ms | +13.1% | ✅⬆️ |
| runtime.dotnet.exceptions.count | 3 ± (3 - 3) | 3 ± (3 - 3) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 16.15 ± (16.13 - 16.18) MB | 15.91 ± (15.90 - 15.93) MB | -1.5% | ✅ |
| runtime.dotnet.threads.count | 21 ± (20 - 21) | 21 ± (21 - 21) | +1.9% | ✅⬆️ |
| .NET Core 3.1 - CallTarget+Inlining+NGEN | ||||
| process.internal_duration_ms | 395.92 ± (394.50 - 397.34) ms | 425.33 ± (423.76 - 426.91) ms | +7.4% | ✅⬆️ |
| process.time_to_main_ms | 522.87 ± (521.84 - 523.90) ms | 572.37 ± (571.09 - 573.66) ms | +9.5% | ✅⬆️ |
| runtime.dotnet.exceptions.count | 3 ± (3 - 3) | 3 ± (3 - 3) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 58.82 ± (58.67 - 58.98) MB | 59.16 ± (59.11 - 59.21) MB | +0.6% | ✅⬆️ |
| runtime.dotnet.threads.count | 30 ± (30 - 30) | 30 ± (30 - 30) | +0.5% | ✅⬆️ |
| .NET 6 - Baseline | ||||
| process.internal_duration_ms | 191.61 ± (191.24 - 191.97) ms | 219.07 ± (218.44 - 219.71) ms | +14.3% | ✅⬆️ |
| process.time_to_main_ms | 69.75 ± (69.61 - 69.89) ms | 79.87 ± (79.66 - 80.09) ms | +14.5% | ✅⬆️ |
| runtime.dotnet.exceptions.count | 4 ± (4 - 4) | 4 ± (4 - 4) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 16.12 ± (15.98 - 16.25) MB | 16.15 ± (16.13 - 16.17) MB | +0.2% | ✅⬆️ |
| runtime.dotnet.threads.count | 19 ± (18 - 19) | 20 ± (20 - 20) | +4.9% | ✅⬆️ |
| .NET 6 - Bailout | ||||
| process.internal_duration_ms | 191.37 ± (190.95 - 191.78) ms | 218.06 ± (217.44 - 218.67) ms | +13.9% | ✅⬆️ |
| process.time_to_main_ms | 71.00 ± (70.85 - 71.16) ms | 80.99 ± (80.80 - 81.18) ms | +14.1% | ✅⬆️ |
| runtime.dotnet.exceptions.count | 4 ± (4 - 4) | 4 ± (4 - 4) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 16.07 ± (15.92 - 16.22) MB | 16.20 ± (16.19 - 16.22) MB | +0.8% | ✅⬆️ |
| runtime.dotnet.threads.count | 20 ± (20 - 20) | 21 ± (20 - 21) | +4.1% | ✅⬆️ |
| .NET 6 - CallTarget+Inlining+NGEN | ||||
| process.internal_duration_ms | 413.66 ± (412.25 - 415.07) ms | 600.34 ± (597.36 - 603.32) ms | +45.1% | ✅⬆️ |
| process.time_to_main_ms | 523.82 ± (522.76 - 524.87) ms | 574.67 ± (573.35 - 575.99) ms | +9.7% | ✅⬆️ |
| runtime.dotnet.exceptions.count | 4 ± (4 - 4) | 4 ± (4 - 4) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 60.94 ± (60.90 - 60.99) MB | 61.72 ± (61.63 - 61.81) MB | +1.3% | ✅⬆️ |
| runtime.dotnet.threads.count | 31 ± (30 - 31) | 31 ± (31 - 31) | +1.1% | ✅⬆️ |
| .NET 8 - Baseline | ||||
| process.internal_duration_ms | 188.84 ± (188.46 - 189.21) ms | 214.99 ± (214.38 - 215.59) ms | +13.8% | ✅⬆️ |
| process.time_to_main_ms | 69.45 ± (69.24 - 69.65) ms | 78.23 ± (78.03 - 78.42) ms | +12.6% | ✅⬆️ |
| runtime.dotnet.exceptions.count | 4 ± (4 - 4) | 4 ± (4 - 4) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 11.79 ± (11.77 - 11.82) MB | 11.51 ± (11.49 - 11.52) MB | -2.4% | ✅ |
| runtime.dotnet.threads.count | 18 ± (18 - 18) | 19 ± (19 - 19) | +4.3% | ✅⬆️ |
| .NET 8 - Bailout | ||||
| process.internal_duration_ms | 188.15 ± (187.87 - 188.42) ms | 213.99 ± (213.42 - 214.56) ms | +13.7% | ✅⬆️ |
| process.time_to_main_ms | 70.31 ± (70.19 - 70.44) ms | 79.42 ± (79.24 - 79.60) ms | +13.0% | ✅⬆️ |
| runtime.dotnet.exceptions.count | 4 ± (4 - 4) | 4 ± (4 - 4) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 11.80 ± (11.73 - 11.86) MB | 11.56 ± (11.54 - 11.57) MB | -2.0% | ✅ |
| runtime.dotnet.threads.count | 19 ± (19 - 19) | 20 ± (20 - 20) | +5.0% | ✅⬆️ |
| .NET 8 - CallTarget+Inlining+NGEN | ||||
| process.internal_duration_ms | 340.99 ± (339.88 - 342.11) ms | 609.73 ± (603.75 - 615.72) ms | +78.8% | ✅⬆️ |
| process.time_to_main_ms | 480.85 ± (479.91 - 481.79) ms | 531.86 ± (530.90 - 532.82) ms | +10.6% | ✅⬆️ |
| runtime.dotnet.exceptions.count | 4 ± (4 - 4) | 4 ± (4 - 4) | +0.0% | ✅ |
| runtime.dotnet.mem.committed | 48.81 ± (48.77 - 48.85) MB | 51.83 ± (51.75 - 51.91) MB | +6.2% | ✅⬆️ |
| runtime.dotnet.threads.count | 30 ± (30 - 30) | 30 ± (30 - 30) | +0.1% | ✅⬆️ |
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 (8339) - mean (72ms) : 69, 75
master - mean (72ms) : 70, 75
section Bailout
This PR (8339) - mean (76ms) : 75, 78
master - mean (76ms) : 75, 78
section CallTarget+Inlining+NGEN
This PR (8339) - mean (1,082ms) : 1027, 1136
master - mean (1,075ms) : 1035, 1115
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 (8339) - mean (112ms) : 109, 116
master - mean (113ms) : 107, 119
section Bailout
This PR (8339) - mean (114ms) : 112, 117
master - mean (114ms) : 111, 117
section CallTarget+Inlining+NGEN
This PR (8339) - mean (795ms) : 768, 822
master - mean (796ms) : 773, 818
FakeDbCommand (.NET 6)
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8339) - mean (99ms) : 96, 103
master - mean (99ms) : 96, 103
section Bailout
This PR (8339) - mean (100ms) : 97, 103
master - mean (100ms) : 97, 103
section CallTarget+Inlining+NGEN
This PR (8339) - mean (947ms) : crit, 914, 980
master - mean (782ms) : 758, 807
FakeDbCommand (.NET 8)
gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8339) - mean (99ms) : 96, 102
master - mean (98ms) : 95, 101
section Bailout
This PR (8339) - mean (99ms) : 96, 102
master - mean (100ms) : 98, 103
section CallTarget+Inlining+NGEN
This PR (8339) - mean (830ms) : crit, 795, 865
master - mean (689ms) : 668, 710
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 (8339) - mean (221ms) : 209, 232
master - mean (194ms) : 189, 198
section Bailout
This PR (8339) - mean (234ms) : crit, 214, 253
master - mean (196ms) : 193, 200
section CallTarget+Inlining+NGEN
This PR (8339) - mean (1,268ms) : crit, 1198, 1339
master - mean (1,154ms) : 1108, 1201
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 (8339) - mean (315ms) : 304, 325
master - mean (276ms) : 270, 283
section Bailout
This PR (8339) - mean (314ms) : crit, 302, 326
master - mean (277ms) : 273, 281
section CallTarget+Inlining+NGEN
This PR (8339) - mean (1,040ms) : crit, 1006, 1073
master - mean (950ms) : 921, 979
HttpMessageHandler (.NET 6)
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8339) - mean (308ms) : 294, 322
master - mean (269ms) : 264, 274
section Bailout
This PR (8339) - mean (308ms) : crit, 297, 319
master - mean (270ms) : 265, 276
section CallTarget+Inlining+NGEN
This PR (8339) - mean (1,207ms) : crit, 1156, 1258
master - mean (968ms) : 945, 991
HttpMessageHandler (.NET 8)
gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8339) - mean (304ms) : 292, 316
master - mean (268ms) : 262, 274
section Bailout
This PR (8339) - mean (304ms) : crit, 294, 314
master - mean (268ms) : 264, 272
section CallTarget+Inlining+NGEN
This PR (8339) - mean (1,172ms) : crit, 1082, 1263
master - mean (852ms) : 833, 871
andrewlock
left a comment
There was a problem hiding this comment.
As discussed, we're missing a lot from the spec, but hey 🤷♂️
|
|
||
| if (hasServiceSource) | ||
| { | ||
| MessagePackBinary.WriteString(stream, "srv_src"); |
There was a problem hiding this comment.
Based on the stats computation spec, I think this should be service_source, but given that none of our keys match here, I don't know what's correct 😅
| MessagePackBinary.WriteString(stream, "srv_src"); | |
| MessagePackBinary.WriteString(stream, "service_source"); |
There was a problem hiding this comment.
We should wait for clarification. Thanks for finding that!
There was a problem hiding this comment.
Does this need to be service_source?
There was a problem hiding this comment.
No, its srv_src. We had a discussion. Also, the system tests PR confirms the field name: DataDog/system-tests#6565
There was a problem hiding this comment.
The spec Andrew posted above needs to be updated?
There was a problem hiding this comment.
Yes, it does, but they are already notified.
| [Key("TopLevelHits")] | ||
| public long TopLevelHits { get; set; } | ||
|
|
||
| [Key("srv_src")] |
There was a problem hiding this comment.
| [Key("srv_src")] | |
| [Key("service_source")] |
|
Note: System test was tested manually by chaging temprarily the pipeline: https://github.com/DataDog/system-tests/pull/6598/changes |
bouwkast
left a comment
There was a problem hiding this comment.
LGTM
The service_source vs svc_src question doesn't look answered yet which seems to be the only concern
|
|
||
| if (hasServiceSource) | ||
| { | ||
| MessagePackBinary.WriteString(stream, "srv_src"); |
There was a problem hiding this comment.
Does this need to be service_source?
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2afede67f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| span.ServiceName, | ||
| span.OperationName, | ||
| span.Type, | ||
| span.Context.ServiceNameSource, |
There was a problem hiding this comment.
Normalize service source before adding it to stats key
Use of span.Context.ServiceNameSource here bypasses the safety normalization used for trace serialization: when a span’s service equals the default service, non-opt.* sources are intentionally cleared before _dd.svc_src is emitted. In this path, those sources are still preserved, so spans can produce stats buckets with srv_src (and separate aggregation keys) even when the corresponding trace payload drops the source. A concrete case is setting ISpan.ServiceName to the default service, which sets source m; this now creates inconsistent trace-vs-stats attribution and extra timeseries cardinality.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
setting
ISpan.ServiceNameto the default service, which sets sourcem
That's a good question. Should this be considered "manual" or "default"? (in both the _dd.svc_src tag and the trace stats)
There was a problem hiding this comment.
Yes, we should apply the same logic for that corner case. If user sets the service name to Default, no tags or stats should be emitted. Fixed.
We had a discussion regarding the topic and the right name is srv_src. Also, the system tests PR confirms the field name: DataDog/system-tests#6565 |
| public readonly string Service; | ||
| public readonly string OperationName; | ||
| public readonly string Type; | ||
| public readonly string ServiceSource; |
There was a problem hiding this comment.
Not a blocker, but maybe add #nullable enabled while you're here and declare some of these as string?? 😅
There was a problem hiding this comment.
That's a good point. I can do that in a subsequent PR. We would need to check what fields can be null and check the callers. It's worth to do that, but probably in a new PR. Also, using scoped namespaces. I promise I will create a subsequent PR. :)
There was a problem hiding this comment.
That's a good point. I can do that in a subsequent PR
Please don't, it'll conflict with mine 😄
Summary of changes
Add
srv_srcfield to the client stats payload, propagating the service name source alongside trace stats.Reason for change
Per the service name source RFC, the source must be propagated in both trace and stats payloads. The trace payload (
_dd.svc_srcspan meta) was added in PR #8302. This completes the implementation by addingsrv_srcat the stats bucket level.Implementation details
StatsAggregationKey.cs: AddedServiceSourcefield. It participates in equality and hash code, so stats with different sources aggregate into distinct buckets.StatsAggregator.cs:BuildKey()now readsspan.Context.ServiceNameSourceand passes it to the aggregation key.StatsBuffer.cs:SerializeBucket()conditionally serializessrv_srcwhenServiceSourceis non-null (dynamic map size: 13 with source, 12 without).Test coverage
KeyEquality_WithServiceSource: Verifies that keys with different sources are not equal.Serialization_WithServiceSource: Verifiessrv_srcis serialized when present and absent when null.StatsBufferTestsandStatsAggregatorTestsupdated and passing.Other details
APMLP-1015
Related agent PR: DataDog/datadog-agent#45982
Reference Java implementation: DataDog/dd-trace-java#10653