feat: add v1 isolated bricks for trace exporter - #2280
Open
anais-raison wants to merge 2 commits into
Open
Conversation
Contributor
📚 Documentation Check Results📦
|
Contributor
🔒 Cargo Deny Results📦
|
anais-raison
marked this pull request as ready for review
July 27, 2026 14:44
|
![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: 16f08876b2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+143
to
+147
| fn get_meta(&'a self, key: &str) -> Option<&'a str> { | ||
| match self.attributes.get(key) { | ||
| Some(AttributeValue::String(s)) => Some(s.borrow()), | ||
| _ => None, | ||
| } |
There was a problem hiding this comment.
When a V1 child span is eligible only because its dedicated span_kind is Client, Server, Producer, or Consumer, this implementation returns None for get_meta("span.kind") because that value is not stored in attributes. SpanConcentrator::is_span_eligible consequently excludes the span, and even otherwise-eligible spans are aggregated with an empty kind and lose peer-tag classification. Map the dedicated span_kind field to the corresponding lowercase metadata value.
Useful? React with 👍 / 👎.
Comment on lines
+119
to
+123
| fn get_meta(&'a self, key: &str) -> Option<&'a str> { | ||
| match self.attributes.get(key) { | ||
| Some(AttributeValue::String(s)) => Some(s.borrow()), | ||
| _ => None, | ||
| } |
There was a problem hiding this comment.
When agent filter configuration targets env, version, component, or span.kind, a normal V1 span stores these values in dedicated fields rather than attributes, so this lookup reports them as missing. Required filters then reject matching traces, while reject filters retain traces that should be dropped; the V1 downgrade encoder explicitly treats these names as promoted fields. Handle the dedicated fields before falling back to string attributes.
Useful? React with 👍 / 👎.
| let Ok(root_span_index) = trace_utils_v1::get_root_span_index(&chunk.spans) else { | ||
| return true; | ||
| }; | ||
| let should_drop = self.should_drop(&chunk.spans[root_span_index]); |
There was a problem hiding this comment.
When a filter tag is supplied through TraceChunk.attributes, which represents attributes common to all spans, passing only the root span to should_drop makes the tag invisible. A required filter will therefore discard a valid chunk and a reject filter will fail to discard a matching chunk. Evaluate the root through a view that falls back to chunk attributes while preserving span-level precedence.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| // PrioritySampler and NoPrioritySampler | ||
| if chunk.priority.is_none_or(|p| p > 0) { |
There was a problem hiding this comment.
When dropped_trace is true but priority is absent or positive, this early return retains every span even though the chunk explicitly says the trace was dropped by samplers. The V1-to-v0.4 encoder already gives this flag precedence by converting it to a negative sampling priority; without equivalent handling here, client-side stats mode forwards P0 spans and reports incorrect dropped counts. Derive an effective rejection state from dropped_trace before this priority check.
Useful? React with 👍 / 👎.
Comment on lines
+377
to
+379
| let spans = traces.iter().flat_map(|chunk| chunk.spans.iter()); | ||
| for span in spans { | ||
| stats_concentrator.add_span(span); |
There was a problem hiding this comment.
When common peer tags are stored in TraceChunk.attributes or origin is stored in TraceChunk.origin, flattening directly to bare spans discards that context before SpanConcentrator builds aggregation keys. Such stats lose configured peer tags and _dd.origin-based synthetics classification even though these values apply to every span in the chunk. Pass a chunk-aware span view to the concentrator with span attributes taking precedence.
Useful? React with 👍 / 👎.
BenchmarksComparisonBenchmark execution time: 2026-07-27 14:46:59 Comparing candidate commit 8b1e810 in PR branch Found 0 performance improvements and 25 performance regressions! Performance is the same for 114 metrics, 0 unstable metrics.
|
Contributor
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
|
ajgajg1134
requested changes
Jul 27, 2026
| { | ||
| let dropped_by_trace_filter = trace_filterer.filter_traces_v1(traces); | ||
| #[cfg(not(all(not(target_arch = "wasm32"), feature = "telemetry")))] | ||
| let _ = dropped_by_trace_filter; |
Contributor
There was a problem hiding this comment.
Hmm, what's the point of this line here? 🤔
| // Not yet called from the live pipeline; will be wired in once the exporter's public API is | ||
| // swapped to v1 native structs. | ||
| #[allow(dead_code)] | ||
| pub(crate) fn process_traces_for_stats_v1<T: libdd_trace_utils::span::TraceData>( |
Contributor
There was a problem hiding this comment.
I know these functions are dead code but should there be any unit tests for these or do you want to include that in the next steps?
| v1_trace_utils::has_top_level(self) | ||
| } | ||
|
|
||
| fn get_meta(&'a self, key: &str) -> Option<&'a str> { |
Contributor
There was a problem hiding this comment.
Are these get meta and metrics methods expected to go away once we move to v1 internally? I wouldn't expect to continue to see references to meta nor metrics once we have attributes
| use std::collections::HashSet; | ||
| use tracing::debug; | ||
|
|
||
| /// Span metric the mini agent must set for the backend to recognize top level span |
| /// - OR its parent belongs to another service (in that case it's a "local root" being the highest | ||
| /// ancestor of other spans belonging to this service and attached to it). | ||
| pub fn compute_top_level_span<T: TraceData>(trace: &mut [Span<T>]) { | ||
| let mut span_id_idx: std::collections::HashMap<u64, usize> = std::collections::HashMap::new(); |
Contributor
There was a problem hiding this comment.
perf nit: Can we initialize this hash map with capacity of len(trace) to avoid re-sizing?
Contributor
There was a problem hiding this comment.
Or maybe instead it might read a little nicer to use something like collect?
let span_id_idx: HashMap<u64, usize> = trace
.iter()
.enumerate()
.map(|(i, span)| (span.span_id, i))
.collect();
But maybe a rust person should double check me here
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Adds v1-native equivalents of existing v0.4 pipeline helpers, as groundwork for making
v1::Span/v1::TraceChunkthe exporter's canonical internal type:trace_utils_v1::drop_chunksandTraceFilterer::filter_traces_v1(libdd-trace-utils)stats::add_spans_to_stats_v1/stats::process_traces_for_stats_v1(libdd-data-pipeline)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.
State of the ticket
Done so far (in this PR)
Remaining (in a follow up PR)
Swap PR (with breaking changes)