Add srv_src field to client stats payload#8339
Conversation
BenchmarksBenchmark execution time: 2026-04-06 11:42:56 Comparing candidate commit 80fdb8f in PR branch Found 6 performance improvements and 9 performance regressions! Performance is the same for 262 metrics, 11 unstable metrics.
|
NachoEchevarria
added
the
area:tracer
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8339) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-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:
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 chartsFakeDbCommand (.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 (75ms) : 70, 79
master - mean (72ms) : 70, 74
section Bailout
This PR (8339) - mean (79ms) : 76, 82
master - mean (76ms) : 74, 78
section CallTarget+Inlining+NGEN
This PR (8339) - mean (1,094ms) : 1054, 1135
master - mean (1,063ms) : 1023, 1103
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 (116ms) : 111, 120
master - mean (113ms) : 109, 116
section Bailout
This PR (8339) - mean (117ms) : 112, 121
master - mean (114ms) : 111, 118
section CallTarget+Inlining+NGEN
This PR (8339) - mean (804ms) : 781, 828
master - mean (783ms) : 766, 800
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8339) - mean (103ms) : 99, 107
master - mean (100ms) : 97, 103
section Bailout
This PR (8339) - mean (103ms) : 100, 107
master - mean (100ms) : 98, 102
section CallTarget+Inlining+NGEN
This PR (8339) - mean (949ms) : 916, 981
master - mean (936ms) : 897, 975
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8339) - mean (102ms) : 98, 106
master - mean (98ms) : 95, 101
section Bailout
This PR (8339) - mean (103ms) : 100, 106
master - mean (99ms) : 97, 102
section CallTarget+Inlining+NGEN
This PR (8339) - mean (833ms) : 792, 873
master - mean (818ms) : 780, 856
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 (189ms) : 186, 192
master - mean (199ms) : 173, 226
section Bailout
This PR (8339) - mean (192ms) : 190, 194
master - mean (193ms) : 192, 195
section CallTarget+Inlining+NGEN
This PR (8339) - mean (1,137ms) : 1085, 1188
master - mean (1,131ms) : 1086, 1175
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 (272ms) : 268, 275
master - mean (271ms) : 267, 275
section Bailout
This PR (8339) - mean (272ms) : 269, 276
master - mean (272ms) : 269, 274
section CallTarget+Inlining+NGEN
This PR (8339) - mean (920ms) : 895, 945
master - mean (922ms) : 901, 943
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8339) - mean (266ms) : 262, 270
master - mean (265ms) : 262, 269
section Bailout
This PR (8339) - mean (266ms) : 264, 268
master - mean (265ms) : 263, 268
section CallTarget+Inlining+NGEN
This PR (8339) - mean (1,136ms) : 1103, 1170
master - mean (1,132ms) : 1083, 1181
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8339) - mean (263ms) : 259, 267
master - mean (263ms) : 260, 266
section Bailout
This PR (8339) - mean (263ms) : 261, 266
master - mean (263ms) : 261, 265
section CallTarget+Inlining+NGEN
This PR (8339) - mean (1,016ms) : 976, 1056
master - mean (1,014ms) : 973, 1055
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
NachoEchevarria
changed the title
andrewlock
left a comment
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
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
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.
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 😄
|
Thanks for the feedback and reviews! |
4 participants
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