feat(tracer): rollout local root span id pprof label in favor of trace id for appsec - #5114
feat(tracer): rollout local root span id pprof label in favor of trace id for appsec#5114eliottness wants to merge 8 commits into
local root span id pprof label in favor of trace id for appsec#5114Conversation
We don't actually use these anywhere in our backend or UI. Stop adding them (saving a bit of memory allocated per span) and remove the associated tests. (cherry picked from commit 3e671ce)
Emit the full 128-bit trace id (32-char lowercase hex) as a new "trace id" pprof label so profiler samples can be correlated with the whole trace, not just the local root span. The "trace id" and "local root span id" labels are now emitted for code hotspots and whenever AppSec is enabled, so AppSec can correlate security events with traces/profiles. "span id" stays gated behind code hotspots and "trace endpoint" behind endpoint profiling. (cherry picked from commit ae25f6e)
The new pprof "trace id" label added a per-span hex.EncodeToString allocation (+1 alloc/span on BenchmarkStartSpan and other span-creation benchmarks). Every span in a trace shares the same trace id, so compute the 32-char hex once and let child spans reuse the parent's cached string: hexEncodedCached memoizes the hex on the not-yet-shared SpanContext during StartSpan, and newSpanContext copies the parent's cached hex into the child (guarded by a full trace-id equality check). The read path (HexEncoded, used by Inject) still never writes, so the v2.8.0-rc.2 concurrent-Inject data race cannot reoccur. benchstat (count=10): allocs/op 16 -> 15, B/op -1.84% on BenchmarkStartSpan / BenchmarkStartSpanConcurrent. (cherry picked from commit 679beae)
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Config Audit |
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: c17535a | Docs | Datadog PR Page | Give us feedback! |
BenchmarksBenchmark execution time: 2026-08-04 14:12:17 Comparing candidate commit c17535a in PR branch Found 24 performance improvements and 4 performance regressions! Performance is the same for 298 metrics, 0 unstable metrics, 1 flaky benchmarks without significant changes.
|
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
eliottness
changed the title
|
keep in mind the benchmarks are like this because appsec and profiling is enabled, there is no changes for APM only customers and APM+appsec or APM+Profling will both only see half the performance impact |
|
@copilot review |
|
@codex review |
The changes look correct and well-structured. Here's my review:
CI: All workflow runs on this branch are passing (Code Freeze, Static Checks, CodeQL, Pull Request Tests, System Tests, etc.). No issues found — the implementation correctly gates the |
![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: 0053e3ee16
ℹ️ 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".
| func (t *traceID) hexEncodedCached() string { | ||
| if t.hexEncoded == "" { | ||
| t.cacheHex() |
There was a problem hiding this comment.
When AppSec is enabled and a custom Sampler.Sample publishes the supplied span or context to another goroutine, StartSpan has already invoked that callback at tracer.go:1024 before reaching this write via applyPPROFLabels at tracer.go:1031. A concurrent TraceID() or Inject() then reads hexEncoded while cacheHex mutates it, producing a reproducible data race under go test -race; populate the cache before invoking the sampler callback or keep this path read-only.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 84722c2 — good catch, this was a real race.
You are right about the ordering: StartSpan calls t.sample(span) (tracer.go:1024) before applyPPROFLabels (tracer.go:1031), so a custom Sampler that publishes the span makes any later write to its SpanContext racy.
Rather than caching earlier inside the label path, the write now happens in newSpanContext (tracer.go:949), which runs before both the sampler and the label path, while the context is still private to the starting goroutine. applyPPROFLabels is now a pure read via HexEncoded(), which returns the cache when set and otherwise encodes locally without storing. hexEncodedCached() is deleted.
Added TestApplyPPROFLabelsTraceIDNoCacheWriteAfterPublish, which reproduces exactly your scenario: a custom Sampler publishes every span to a reader goroutine that calls TraceID() while StartSpan is still running. It fails on the previous commit under -race:
WARNING: DATA RACE
...tracer.(*traceID).cacheHex()
...tracer.(*traceID).hexEncodedCached()
and passes now. Full go test -race ./ddtrace/tracer ./internal/traceprof/... is green.
| if t.hexEncoded == "" { | ||
| t.cacheHex() | ||
| } |
There was a problem hiding this comment.
There is a TOCTU race here; unless we can absolutely guarantee that hexEncodedCached() may only be called by one specific goroutine (which isn't enforced at the moment; and multiple spans may be started concurrently from the same extracted context, for example).
Apparently, there is a similar race around trace.root in similar circumstances.
There was a problem hiding this comment.
Both confirmed. The TOCTOU is fixed in 84722c2; the trace.root one is real but pre-existing, so I would like to do it separately.
1. hexEncodedCached() TOCTOU — fixed. You were right that nothing enforced single-goroutine access. hexEncodedCached() is deleted. The cache is now finalized in newSpanContext (called at tracer.go:949), which is before t.sample (:1024) and applyPPROFLabels (:1031), so the context is provably still private to the starting goroutine. The label path only reads, via HexEncoded(). Regression test TestApplyPPROFLabelsTraceIDNoCacheWriteAfterPublish fails on the parent commit under -race and passes now.
2. trace.root — confirmed, and it reproduces on main unchanged. newSpanContext does an unsynchronized check-then-set:
if context.trace.root == nil {
context.trace.root = span
}Concurrent children of one extracted context all race there, and span.Root() can then hand applyPPROFLabels a span another goroutine is still initializing from the pool. I reproduced it on a clean origin/main worktree (20addd7ce, no changes from this PR) with 32 concurrent children of one extracted context:
WARNING: DATA RACE
Write at ... by goroutine 51:
tracer.newSpanContext() spancontext.go:334
Previous read at ... by goroutine 56:
tracer.spanStart() tracer.go:937
Since it is orthogonal to the label change and sits on the span-start hot path, I would rather not fold it into this PR: the fix wants trace.root published once under the trace lock and read atomically, which touches every trace.root reader and needs its own Benchmarking Platform run. Happy to open that PR right after this one — or to fold it in here if you prefer it shipped together.
For that reason the test I added deliberately covers the hex path only (TestNewSpanContextTraceIDHexInheritsParentReadOnly, asserting the child copy never rewrites the shared parent). Any concurrent shared-parent test still trips the trace.root race regardless of profiler settings, so it would have been red for a reason unrelated to this PR.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
|
Note on the four
What does change is where the linker places the protobuf sizing code. Comparing the baseline and candidate test binaries, the sizing functions are identical instructions at shifted addresses ( Two supporting results:
The per-span delta (+18-28 ns, no allocation change) is consistent with the protobuf per-span sizing loop straddling instruction-cache lines differently. Suggested follow-up, outside this PR: run the Benchmarking Platform stability workflow for |
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
|
Ran a five-lane review over this branch (coverage, concurrency, memory, package consistency, dead-code/comment audit). Fixed everything in scope in Fixed
Benchmarks under the CI configuration (Go 1.25.9, Pre-existing, deliberately not in this PR
|
nsrip-dd
left a comment
nsrip-dd
left a comment
There was a problem hiding this comment.
I'm inclined to approve this if it unblocks an important feature for appsec, but I want to make sure we clearly document what this is for and what we're paying for it. So, I have a few clarifying points/questions, and perhaps you could update the PR description so we have a record of this?
- In the motivation, you say "AppSec needs a whole-trace correlation key in profiles". To be clear, it doesn't actually matter if the new label in the CPU/goroutine profile, right? The thing that's reading these labels is eBPF code that knows how to find goroutine labels. Nothing new is touching the profiles that the client library uploads.
- So we could, for example, strip them from the CPU profile before we upload it.
- I also want to call out that this makes CPU profiles bigger. We care about this because upload/intake bandwidth costs us and our customers money. I have about 600 CPU profiles at hand. I ran a quick test replacing the local root span ID labels with trace ID labels, taking the local root span ID and padding out to 128 bits, hex-encoded. I did random padding and all zeros. With random padding, CPU profiles on average were 10% bigger, with a p95 increase of 35%, after zstd compression. With zero padding the average and p95 are about half of that. I haven't yet done the math on overall payload size increase, but CPU profiles tend to be the largest things besides execution traces, so I would anticipate a modest but non-zero size increase.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
|
Thanks — both points are fair, and the second one changes the cost calculus. I investigated the strip idea; findings below, and I will fold the size cost into the PR description. 1. Who reads the label Your eBPF hypothesis holds up as far as I can verify from open sources, but I cannot fully close it from this repo.
Worth separating out: 2. Stripping is possible, but not free I checked whether we could piggyback on an existing decode. We cannot, for CPU specifically:
So the cheapest correct option is a sanitizer at collection time, right after Affected profile types are CPU, goroutine, and the experimental goroutine-leak profile. Heap, block, and mutex serialize samples without goroutine labels, and the execution trace is a separate 3. Alternatives
Recommendation: keep the label live for eBPF and strip it from CPU, goroutine, and goroutine-leak payloads at collection time — conditional on someone confirming no backend consumer. That is roughly 40-80 production lines using Happy to take that on. Do you want the strip before this merges, or as an immediate follow-up? |
This was a question for you @eliottness not an LLM. Is it right that this label is consumed by eBPF code running in/observing a process, and that nothing on the AppSec side is going to look at the aggregated CPU profiles that this client library uploads? The agent PR referenced there seems to confirm that, but I'd like to know definitively. Regarding stripping the label, yeah, I can do that as a followup if we need to. |
|
@nsrip-dd Help me I am loosing control of my AI I can confirm we only use pprof labels as a mean-to-an-end here to have custom data readable from an eBPF probe |
eliottness
changed the title
local root span id pprof label in favor of trace id for appsec
| if parent != nil && context.traceID.value == parent.traceID.value { | ||
| context.traceID.hexEncoded = parent.traceID.hexEncoded | ||
| } | ||
| // AppSec correlates security events with profiles through the "trace id" | ||
| // pprof label, which needs the hex form. Finalize the cache here, while the | ||
| // context is still private to this goroutine: StartSpan hands the span to | ||
| // the sampler (and any custom Sampler may publish it) before it applies the | ||
| // pprof labels, so a lazy write from that later point would race with | ||
| // readers such as TraceID and Inject. | ||
| if context.traceID.hexEncoded == "" && appsec.Enabled() { | ||
| context.traceID.cacheHex() | ||
| } |
There was a problem hiding this comment.
can these statements be combined? they both look at context.traceID.hexEncoded. It seems context.traceID.hexEncoded is always "" if the span doesn't have a parent, so you can probably do some if/else-ing here?
| // Disable AppSec so locally started spans keep a cold hex cache; with it | ||
| // enabled, newSpanContext finalizes the cache and the "cold cache" subtest | ||
| // below could no longer exercise the non-caching HexEncoded read path. |
There was a problem hiding this comment.
Perhaps I'm not the target audience for this, but I don't know what this comment means 😆 . Can you make it clearer/simpler?
| // Apply the pprof labels before t.sample: a custom Sampler receives the span | ||
| // and may publish it to another goroutine, after which writing span fields | ||
| // here would race with that goroutine (e.g. SetTag or Finish). | ||
| t.applyPPROFLabels(span.pprofCtxRestore, span, cSnap) |
There was a problem hiding this comment.
Is there a more robust way to ensure pprof labels are always added to the span before sampling?
| // +checklocksignore — Initialization time, called from StartSpan before the span | ||
| // is handed to the sampler, so it is not yet shared with other goroutines. | ||
| func (t *tracer) applyPPROFLabels(ctx gocontext.Context, span *Span, snap internalconfig.SpanStartSnapshot) { | ||
| // "trace id" is AppSec-only. Profiling features retain their own labels |
There was a problem hiding this comment.
"trace id" is AppSec-only
Is confusing to me. "trace id" to me means "ID of the current trace" but I assume you mean the "trace id" label.
What does this PR do?
Combines Nick Ripley's #5087 with the trace-ID behavior from #5064, scoped to AppSec:
local root span idpprof labeltrace idpprof label only when AppSec is enabledspan idand do not receivetrace idtrace endpointand does not receivetrace idMotivation
AppSec needs a whole-trace correlation key in profiles. Profiling alone does not need the trace ID and must not pay its cardinality or encoding cost. #5087 demonstrates that
local root span idis unused by the profiling backend/UI, so removing it preserves or improves profiling performance while AppSec selectively addstrace id.The previous Benchmarking Platform run was for head
819a3eb46, before the AppSec-only gate. Head0053e3ee1removes trace-ID work from profiling-only benchmarks; its new Benchmarking Platform result is the decision criterion.Validation:
go test -race -count=1 ./ddtrace/tracer ./internal/traceprof/...go test -count=1 ./ddtrace/tracer/...internal/traceprof/traceproftestTestEndpointsAndCodeHotspotsgo vet ./ddtrace/tracer ./internal/traceprof/...Reviewer's Checklist