feat(ddtrace/tracer): promote span fields out of meta map into a typed SpanAttributes struct #4538
feat(ddtrace/tracer): promote span fields out of meta map into a typed SpanAttributes struct #4538
Conversation
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: 423355d | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback! |
Codecov Report❌ Patch coverage is Additional details and impacted files
🚀 New features to boost your workflow:
|
BenchmarksBenchmark execution time: 2026-03-27 18:59:17 Comparing candidate commit 423355d in PR branch Found 3 performance improvements and 11 performance regressions! Performance is the same for 202 metrics, 8 unstable metrics.
|
I wasn't expecting this. |
Okay this is because of the dual write. |
kakkoyun
left a comment
kakkoyun
left a comment
There was a problem hiding this comment.
This is on great trajectory.
However, I think we should meditate on the API a little more. spanMeta types is good place to hide all the implementation details. We shouldn't allow accessing spanMeta.m and spanMeta.attrs directly.
|
|
||
| //go:linkname spanStart github.com/DataDog/dd-trace-go/v2/ddtrace/tracer.spanStart | ||
| func spanStart(operationName string, options ...tracer.StartSpanOption) *tracer.Span | ||
| func spanStart(operationName string, sharedAttrs unsafe.Pointer, options ...tracer.StartSpanOption) *tracer.Span |
There was a problem hiding this comment.
I think we should go level higher here if it is possible. sharedAttrs should be an implementation detail.
| func spanStart(operationName string, sharedAttrs unsafe.Pointer, options ...tracer.StartSpanOption) *tracer.Span | |
| func spanStart(operationName string, meta spanMeta, options ...tracer.StartSpanOption) *tracer.Span |
There was a problem hiding this comment.
@kakkoyun mocktracer doesn't have visibility of the unexported spanMeta type. How should this be done?
There was a problem hiding this comment.
😭 mocktracer strikes again. We should find a better solution for it.
There was a problem hiding this comment.
I agree with Kemal; would #4512 unblock this for us?
darccio
force-pushed
the
dario.castane/apmlp-856/promote-redundant-span-fields
branch
from
08a2e11 to
1ce48fe
Compare
kakkoyun
left a comment
kakkoyun
left a comment
There was a problem hiding this comment.
The API surface improved quite a bit. We are adding a lot of complexity for this optimization but I'm glad we containing it in an internal package.
That being said, I still see some room for improvement. There are several methods that kind of access the internal workings of the SpanMeta and SpanAttributes.
Methods in particula: ReplaceSharedAttrs, IsPromotedKeyLen
It would be amazing to make Inline transparent as well.
Ideally, tracer/span.go shouldn't now about interworking of the SpanMeta the logic around promoting and inlining.
They are dual-stored values to avoid breaking v0.4 encoding.
…ementation details
…ssing promoted fields with non-string values
… avoid race condition
Add Put() as an inlineable flat-map write for paths wehre promoted keys are already handled. Rename Merge() to Map() and make it transparently inline promoted attrs into sm.m on first call.
darccio
force-pushed
the
dario.castane/apmlp-856/promote-redundant-span-fields
branch
from
f692e05 to
d80aad0
Compare
…mic.Bool in SpanMeta
mtoffl01
left a comment
mtoffl01
left a comment
There was a problem hiding this comment.
component and span.kind are never set on the tracer's shared attrs — they are not "process-level shared attributes," rather they're purely per-span fields set by integrations. Including them in SpanAttributes means every integration span triggers a COW clone, even though the "shared" part of the struct (env, version, language) is never what changed. Would it make sense to only promote the truly process-level fields into SpanAttributes and leave component/span.kind in the flat map? This approach might not be fully optimal for the V1 encoder, but it's still better than before, and v1 can still read those dedicated fields from the map.
Alternatively, I could see this being more optimal if it was structured as one copy per integration.
ddtrace/tracer/tracer.go
Outdated
| dataStreams: dataStreamsProcessor, | ||
| logFile: logFile, | ||
| } | ||
| // Build the shared SpanAttributes that every span will start from. |
There was a problem hiding this comment.
it would be nice to put some of this in a separate helper function, dedicated to building sharedAttrs.
|
|
||
| //go:linkname spanStart github.com/DataDog/dd-trace-go/v2/ddtrace/tracer.spanStart | ||
| func spanStart(operationName string, options ...tracer.StartSpanOption) *tracer.Span | ||
| func spanStart(operationName string, sharedAttrs unsafe.Pointer, options ...tracer.StartSpanOption) *tracer.Span |
There was a problem hiding this comment.
I agree with Kemal; would #4512 unblock this for us?
| // SpanMeta replaces a plain map[string]string for the Span.meta field. | ||
| // Promoted attributes (env, version, component, span.kind, language) live in | ||
| // attrs and are excluded from the map m. The msgp codec merges both sources | ||
| // transparently so the wire format is unchanged. | ||
| // | ||
| // Set routes promoted keys to attrs (with copy-on-write) and others to the | ||
| // flat map. Promoted keys never appear in sm.m until Map() is called. | ||
| // | ||
| // Map() inlines promoted attrs into sm.m under mu, then returns sm.m directly | ||
| // (zero allocation on the hot stats path). EncodeMsg, Msgsize, Range, | ||
| // SerializableCount, and IsZero also acquire mu so they see a consistent view | ||
| // of sm.m during concurrent serialization. | ||
| type SpanMeta struct { | ||
| m map[string]string | ||
| attrs *SpanAttributes | ||
| mu locking.Mutex | ||
| inlined atomic.Bool | ||
| } |
There was a problem hiding this comment.
The distinction between attrs and m isn't very clear from reading SpanMeta alone. I think we can do a better job of defining their roles...
- rename attrs to either
sharedAttrs(shows commonality with the tracer'ssharedAttrsfield) orpromotedAttrs(makes the "promoted" behavior clear) , or - comment could mention that attrs starts as a "borrowed" pointer from tracer's process-level SpanAttributes and is cloned into a span-specific copy only on write
…void race conditions
You are right, I tried to max the promoted fields. Let me demote them.
I see your point but it's difficult. Let try this in a future PR.
Good idea!
That PR will deprecate
Applying it right now. Great feedback, thanks! |
…naming for shared attributes, improve internal naming
darccio
force-pushed
the
dario.castane/apmlp-856/promote-redundant-span-fields
branch
from
161ea73 to
78586fe
Compare
I tried really hard to avoid it, so I renamed it to |
darccio
force-pushed
the
dario.castane/apmlp-856/promote-redundant-span-fields
branch
from
aedc9f7 to
9020723
Compare
3 participants
For reviewers, a walkthrough doc: https://docs.google.com/document/d/1kWp1ZxC5s9laqbva9cqrY0cixQYQm3b8m5Y3VA8oX6A/edit?usp=sharing
What does this PR do?
Introduces two new types in
ddtrace/tracer/internaland wires them into the span lifecycle:SpanAttributes— a compact, fixed-size struct that stores the four V1-protocol promoted span fields (env,version,component,span.kind) in a typed[4]stringarray indexed by integerAttrKeyconstants, backed by a 1-byte presence bitmask (setMask). The bitmask distinguishes "field never set" from "field explicitly set to empty string", which a plainmap[string]stringcannot express. Total size: 72 bytes. Read and write operations are bitmask tests and array accesses — no string comparison, no hash, no allocation.SpanMeta— replacesspan.meta map[string]stringas the span's metadata field. Internally it holds a flatmap[string]string(m) for arbitrary tags and a*SpanAttributes(attrs) for the four promoted keys. The custom msgp codec merges both sources transparently so the V0.4 wire format is unchanged. ASetMap/DeleteMapfast path writes directly tomfor non-promoted keys (used by the init hot-path); theSet/Deletepath routes promoted keys toattrsvia copy-on-write.Copy-on-write shared attrs — the tracer holds two process-level
SpanAttributesinstances (sharedAttrsandsharedAttrsForMainSvc, the latter pre-populated withversionfor main-service spans underuniversalVersion=false). Every new span starts by sharing one of these pointers viaNewSpanMeta(sharedAttrs). A COW clone is triggered only when a span sets a per-span promoted field (component,span.kind, or a differingenv/version), meaning spans that never override promoted fields allocate noSpanAttributesof their own.Inline()— zero-allocationMerge()at finish time — atspan.finish(), aftercontext.finish()completes all trace-level tag propagation,s.meta.Inline()copies all setattrsvalues intom(at most 4 writes). After this pointattrsis still authoritative forGet(), butMerge()can detect thatattrsNotInM() == 0and returnsm.mdirectly — eliminating themake(map[string]string)that would be required to return a read-only copy of the merged view of promoted fields andm.EncodeMsgandMsgsizesimilarly skip the attrs loop entirely when all attrs are already inm.V1 payload —
payload_v1.goskips promoted keys when iterating the meta map for the attributes array (AttrKeyForTagguard), then readsenv,version,component,span.kindviameta.Get()for the dedicated fields 13–16. The size calculation usesCount() - AttrCount()to count only the flat-map entries.span_msgp.go— the generated codec is replaced by a hand-maintained equivalent.EncodeMsgdelegates toSpanMeta.EncodeMsg;DecodeMsgreads directly intoSpanMeta.DecodeMsgwhich stores all keys (including promoted ones on the decode path) in the flat map, avoiding anySpanAttributesallocation at decode time. A helper script (scripts/msgp_span_meta_omitempty.go) patches theomitemptyguard that themsgpgenerator cannot produce for types implementingmsgp.Encodable.Compile-time assertions (
[1]byte{}[AttrKey-N]) guard the numeric values of theAttrKeyconstants, turning any future reordering into a build error.Motivation
Previously
span.metawas a plainmap[string]string. The four promoted fields (env,version,component,span.kind) were stored in that map alongside arbitrary tags and had to be looked up by string key throughout the hot path: V1 encoding, sampler, peer-service detection, stats concentrator, and debug logging each paid a map lookup and a string hash. In addition:Merge()(used by the stats concentrator and V0.4 payload builder) unconditionally allocated a freshmap[string]stringeven for the common case where all promoted attrs are set, costing ~200–400 bytes per finished span.""", which is relevant for the V1 encoder's field-presence semantics.This change eliminates the per-span allocation for the promoted fields on the read path (replaced by array index + bitmask), eliminates the
Merge()allocation on the finish path (replaced by directsm.mreturn afterInline()), and reduces allocation pressure during span construction by sharing process-level attrs across all spans via COW.Reviewer's Checklist
make lintlocally.make testlocally.make generatelocally.make fix-moduleslocally.Unsure? Have a question? Request a review!