fix(trace-utils)!: serialize v0.5 span links and events into meta#980
Conversation
BenchmarksComparisonBenchmark execution time: 2026-07-10 17:44:31 Comparing candidate commit b2dda5c in PR branch Found 1 performance improvements and 6 performance regressions! Performance is the same for 68 metrics, 10 unstable metrics.
|
paullegranddc
changed the title
| if !span.span_links.is_empty() { | ||
| let serialized_span_links = serde_json::to_string(&span.span_links)?; |
There was a problem hiding this comment.
I think we should check here for the 25kb limit on the tag value, right?
There was a problem hiding this comment.
Yes, anything beyond 25KB limits is dropped. the implementation should try to stuff as many as links possible under the limit and drop the others.
There used to be dropped_attributes_count to count the dropped attributes in case of oversized span links which i didn't see our implementation.
There was a problem hiding this comment.
Good question — I looked into it and concluded we shouldn't add a guard here. Reasoning:
The 25 KB cap (traceutil.MaxMetaValLen = 25000) lives agent-side in truncator.go, applied uniformly to every meta value as the final normalization step (Truncate(span) in agent/agent.go), regardless of which key or source produced it. The agent is the arbiter of this limit — it normalizes whatever it receives.
Given that, a check here:
- couldn't improve the outcome — truncating a JSON string to fit 25 KB produces invalid JSON (exactly what the agent's truncator does anyway), and dropping the key would instead diverge from what the agent does with an oversized value (keep it, truncated). Either way the agent re-applies its own 25 KB cap as its last step, so a client-side guard can't change the result;
- would be inconsistent — libdatadog caps no other meta value's length anywhere in the v0.4/v0.5 conversion; it defers meta-value length normalization to the agent. Guarding only these two keys would be the lone exception.
So I've left it to the agent's existing uniform truncation. Happy to revisit if you'd rather we drop oversized link/event blobs client-side (a deliberate divergence from the agent's normalization).
There was a problem hiding this comment.
Agreed on not doing truncation for now. That could be followup PR, with valid JSON truncation or we could just ignore that since v05 is going to be deprecated in the not so far future
ganeshnj
left a comment
ganeshnj
left a comment
There was a problem hiding this comment.
a few minor comments around tests and one major around tag size limit
overall looks good.
| if !span.span_links.is_empty() { | ||
| let serialized_span_links = serde_json::to_string(&span.span_links)?; |
There was a problem hiding this comment.
Yes, anything beyond 25KB limits is dropped. the implementation should try to stuff as many as links possible under the limit and drop the others.
There used to be dropped_attributes_count to count the dropped attributes in case of oversized span links which i didn't see our implementation.
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: b2dda5c | Docs | Datadog PR Page | Give us feedback! |
The 25KB tag-value truncation concern is being handled separately in #2161. The other points from this March 2025 review round are addressed in the updated PR, so dismissing to unblock re-review.
b19c492 to
a88e3a7
Compare
Clippy Allow Annotation ReportComparing clippy allow annotations between branches:
Summary by Rule
Annotation Counts by File
Annotation Stats by Crate
About This ReportThis report tracks Clippy allow annotations for specific rules, showing how they've changed in this PR. Decreasing the number of these annotations generally improves code quality. |
b7d6356 to
bf07b8c
Compare
bengl
changed the title
The v0.5 trace format is a fixed 12-element positional span array with no slots for span links, span events, or meta_struct. Previously the v0.5 conversion dropped span links and span events entirely. The agent and backend read them back from `meta` as JSON strings, so serialize them there: - span links under the "_dd.span_links" key, matching the agent's OTLP `transform.MarshalLinks`: a hex-encoded 128-bit trace_id (high then low 64 bits), hex-encoded 64-bit span_id, and optional tracestate and attributes. The v0.4 flags field is intentionally omitted to match the agent, the canonical producer of this key. - span events under the "events" key, matching the agent's `MarshalEvents` (time_unix_nano, name, attributes as natural JSON). Both keys are only emitted when non-empty. Event and link attributes are emitted with keys in sorted order, matching Go's encoding/json (used by the agent), so the output is deterministic despite the HashMap source. The v0.5 exporter snapshot is updated to carry the new events / _dd.span_links meta keys. Carrying these requires interning dynamically-built JSON strings, which a borrowed text dictionary cannot own. The v0.5 shared dictionary therefore always owns its strings (SharedDictBytes) via a new SpanText::to_bytes_string conversion: borrowed input text is copied, owned text is reference-counted. meta_struct still has no v0.5 representation (arbitrary binary blobs, no agent-side meta-key convention) and is documented as dropped; callers that need it must use the v0.4 output format. Co-authored-by: paullegranddc <[email protected]> BREAKING CHANGE: the v0.5 shared dictionary is now always owned (`SharedDictBytes`). `v05::from_v04_span` takes `&mut SharedDictBytes` instead of `&mut SharedDict<T::Text>`, and the `TraceChunks::V05` variant holds `SharedDictBytes` instead of `SharedDict<T::Text>`.
bf07b8c to
f82a230
Compare
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
|
Approved, but I cannot approve since this is technically my PR |
|
There is some redundant code with the agentless endpoint here https://github.com/DataDog/libdatadog/blob/main/libdd-trace-utils/src/agentless_encoder/mod.rs#L322 but we can do the unification in a followup PR |
Aaalibaba42
left a comment
Aaalibaba42
left a comment
There was a problem hiding this comment.
Idk the protocol well, but rust side seems reasonable o/
|
|
||
| // Intern fields in the same order as the base conversion to keep dictionary indices | ||
| // stable; the span links / events keys are appended to `meta` afterwards. | ||
| let service = dict.get_or_insert(span.service.to_bytes_string())?; |
There was a problem hiding this comment.
I'm ok with doing this in a follow-up PR (let's make sure to create a Jira ticket if we do) - But should we have something like SharedDIctBytes::get_or_insert_with() that takes a &str instead so we don't do the ByteString allocation so frequently for what is most often going to be a "hit" and not an "insert" and defer the to_bytes_string() to only inserts?
There was a problem hiding this comment.
This is a topic we have been discussing with @VianneyRuhlmann for a while. Taking references to the v04 span string might not be possible in all cases as it requires keeping the v04 spans alive and pinned to the stack.
The solution would probably to add a from_string function to the SpanText trait, which would allow both:
- Create new dynamic SpanText strings for cases like this where we modify meta attributes in the exporter
- not having to convert ByteString and instead taking ownership of existing Spantext objects
but that requires non trivial changes to how we implement the trait in python (because we cannot python string in rust because no GIL).
I think a Jira ticket with refactoring of this after we implement dynamic SpanText creation in the exporter situation is the best course of action
| /// low 64 bits). | ||
| /// - `span_id` is the 64-bit id hex-encoded as 16 lowercase chars. | ||
| /// - `tracestate` and `attributes` are only emitted when non-empty. | ||
| /// - The v0.4 `flags` field is intentionally not emitted: it is not part of the `_dd.span_links` |
There was a problem hiding this comment.
I'm slightly confused by this comment and behavior. The RFC states that flags should only be omitted if not set. Is the RFC wrong?
There was a problem hiding this comment.
I'm not sure what my original intention was.
It looks like the agent does not emit flags but that it's a bug in the agent, not an actual thing 🤔
There was a problem hiding this comment.
added flags serialization
ekump
left a comment
ekump
left a comment
There was a problem hiding this comment.
I have a question around dropping flags, but LGTM
|
/merge |
|
View all feedbacks in Devflow UI.
This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
The expected merge time in
Tests failed on this commit ccebacd: What to do next?
|
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
7 participants
What does this PR do?
The v0.5 trace format is a fixed 12-element positional span array with no dedicated slots for span links, span events, or
meta_struct. The v0.5 conversion previously dropped span links and span events. The Datadog agent/backend read them back from themetafield as JSON strings, so this PR serializes them there:meta["_dd.span_links"], matching the agent's OTLPtransform.MarshalLinks: a hex-encoded 128-bittrace_id(high 64 bits then low), hex-encoded 64-bitspan_id, and optionaltracestate/attributes. The v0.4flagsfield is intentionally omitted to match the agent, the canonical producer of this key.meta["events"], matching the agent'sMarshalEvents({time_unix_nano, name, attributes}, attributes rendered as natural JSON rather than the v0.4 msgpack tagged form).Both keys are only emitted when non-empty.
span_links fallback format spec
event fallback format spec
Motivation
Ensure the v0.5 format carries the same span data as v0.4. Without these fallbacks span links and events are silently dropped on the v0.5 path.
Additional Notes
Reworked on top of current
main, which had refactored the span types to be generic over aTraceData/SpanTextabstraction with a zero-copy borrowed variant.Carrying links/events requires interning dynamically-built JSON strings, which a borrowed (
&str) dictionary cannot own. The v0.5 shared dictionary therefore always owns its strings (SharedDictBytes), via a newSpanText::to_bytes_stringconversion: borrowed input text is copied, owned text is reference-counted. Tradeoff: the zero-copy decode path now copies span text into the v0.5 dictionary; owned-BytesStringcallers are unaffected.meta_structstill has no v0.5 representation (arbitrary binary blobs, no agent-side meta-key convention) and is documented as dropped; callers needing it must use the v0.4 output format.How to test the change?
Unit tests cover: the
_dd.span_links/eventsJSON shapes; 128-bit hextrace_idencoding; conditional key emission;flagsomission; all scalar attribute types plus arrays and multi-attribute events; themeta_structdrop; multi-link and partial-field link serialization; and preserved dictionary interning order.Breaking changes
This is an API-breaking change (Rust API; libdatadog callers pin versions). The v0.5 shared dictionary is now always owned (
SharedDictBytes) so it can intern dynamically-built JSON:v05::from_v04_spannow takes&mut SharedDictBytes(was&mut SharedDict<T::Text>).TraceChunks::V05now holdsSharedDictBytes(wasSharedDict<T::Text>).