feat(trace-utils): add v1-native agentless JSON encoder brick - #2370
feat(trace-utils): add v1-native agentless JSON encoder brick#2370anais-raison wants to merge 3 commits into
Conversation
Isolated brick for APMSP-2812: adds encode_payload_from_v1 and its v1::Span-native helpers (collect_attrs_v1, flatten_attr_into_v1, encode_span_link_v1, encode_span_event_v1) alongside the existing v0.4 agentless encoder. Not wired into any live send path yet.
📚 Documentation Check Results📦
|
🔒 Cargo Deny Results📦
|
|
BenchmarksComparisonBenchmark execution time: 2026-08-20 12:03:08 Comparing candidate commit 1a854a5 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 152 metrics, 0 unstable metrics.
|
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
|
| type CollectedAttrsV1<'a, T> = ( | ||
| Vec<(String, String)>, | ||
| Vec<(String, f64)>, | ||
| Vec<(&'a <T as TraceData>::Text, &'a <T as TraceData>::Bytes)>, | ||
| ); |
There was a problem hiding this comment.
it's inferable from the typing, but I would prefer and actual struct with named field rather than an aliased tuple here
| .filter(|(k, _)| !PROMOTED_ATTR_KEYS_V1.contains(&(*k).borrow())) | ||
| .chain(chunk_attrs_dd.iter().filter(|(k, _)| { | ||
| !PROMOTED_ATTR_KEYS_V1.contains(&(*k).borrow()) | ||
| && !span_attrs_dd.iter().any(|(k2, _)| k2 == *k) | ||
| })); |
There was a problem hiding this comment.
Is this really necessary?
We perfom len(PROMOTED_ATTR_KEYS_V1) * len(span_attr) + (len(PROMOTED_ATTR_KEYS_V1) + len(span_attr) ) * len(chunk_attr) string comparison 🤔
Span level attributes should not collide with chunk level attribute, and if they do they should be promoted before we reach the encoder IMO.
| AttributeValueV1::Bytes(_) => { | ||
| // Callers filter `Bytes` out before recursing; unreachable in practice. | ||
| } |
There was a problem hiding this comment.
This is not true
If one of the nested AttributeValueV1::List item is a Bytes attribute it is not filteredt
| let mut meta_leaves: Vec<(String, String)> = Vec::new(); | ||
| let mut metrics_leaves: Vec<(String, f64)> = Vec::new(); | ||
| let mut bytes_attrs: Vec<(&T::Text, &T::Bytes)> = Vec::new(); | ||
| let mut key_buf = String::new(); | ||
| for (k, v) in merged_attrs { | ||
| match v { | ||
| AttributeValueV1::Bytes(b) => bytes_attrs.push((k, b)), | ||
| _ => { | ||
| key_buf.clear(); | ||
| key_buf.push_str(k.borrow()); | ||
| flatten_attr_into_v1(&mut key_buf, v, &mut meta_leaves, &mut metrics_leaves); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I think we can assume that most of of attributes are scalars.
This means that the attribute key will be a single segment, that does not need flattening and we can borrow it as is from the merged_attrs.
So in order to not copy keys in the non-nested case we can use a Vec<(Cow<'a, str>, Cow<'a, str>)> and borrow the key (and values if the scalar is a string, or a value we can turn in a &'static like booleans)
Something like this I guess
| let mut meta_leaves: Vec<(String, String)> = Vec::new(); | |
| let mut metrics_leaves: Vec<(String, f64)> = Vec::new(); | |
| let mut bytes_attrs: Vec<(&T::Text, &T::Bytes)> = Vec::new(); | |
| let mut key_buf = String::new(); | |
| for (k, v) in merged_attrs { | |
| match v { | |
| AttributeValueV1::Bytes(b) => bytes_attrs.push((k, b)), | |
| _ => { | |
| key_buf.clear(); | |
| key_buf.push_str(k.borrow()); | |
| flatten_attr_into_v1(&mut key_buf, v, &mut meta_leaves, &mut metrics_leaves); | |
| } | |
| } | |
| } | |
| let mut meta_leaves: Vec<(Cow<'a, str>, Cow<'a, str>)> = Vec::new(); | |
| let mut metrics_leaves: Vec<(Cow<'a, str>, f64)> = Vec::new(); | |
| let mut bytes_attrs: Vec<(&T::Text, &T::Bytes)> = Vec::new(); | |
| let mut key_buf = String::new(); | |
| for (k, v) in merged_attrs { | |
| match v { | |
| AttributeValueV1::Bytes(b) => bytes_attrs.push((k, b)), | |
| AttributeValueV1::String(s) => meta_leaves.push(Cow::Borrowed(k.as_str(), Cow::Borrowed(s)), | |
| // same for metrics/ bools/ints/floats | |
| _ => { | |
| key_buf.clear(); | |
| key_buf.push_str(k.borrow()); | |
| flatten_attr_into_v1(&mut key_buf, v, &mut meta_leaves, &mut metrics_leaves); | |
| } | |
| } | |
| } |
| dedup_first_wins_v1(meta_leaves), | ||
| dedup_first_wins_v1(metrics_leaves), |
There was a problem hiding this comment.
why is the dedup here necessary if we already filtered out trace chunks and span attrs with the same paths?
| } | ||
| _ => {} | ||
| } | ||
| metrics.serialize_entry(key, val)?; |
There was a problem hiding this comment.
We should skip NaN and Inf metric values as is done for the v04 encoder
2 participants
What does this PR do?
Adds a v1-native equivalent of the existing v0.4 agentless JSON encoder, as groundwork for making
v1::Span/v1::TraceChunkthe exporter's canonical internal type:agentless_encoder::encode_payload_from_v1and its helpers (collect_attrs_v1,flatten_attr_into_v1,encode_span_link_v1,encode_span_event_v1) (libdd-trace-utils)Nothing is wired into the live pipeline yet.
Motivation
Part of APMSP-2812: migration of the exporter from v0.4 to v1 with isolated bricks first before one final breaking PR that will handle the actual swap.
Additional Notes
Pure addition, no behavior change — these functions aren't called anywhere yet, so there's no regression risk.