Conversation
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
|
BenchmarksComparisonBenchmark execution time: 2026-09-11 14:33:43 Comparing candidate commit 025655e in PR branch Found 35 performance improvements and 3 performance regressions! Performance is the same for 128 metrics, 10 unstable metrics.
|
cfa88f9 to
025655e
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 025655ea2e
ℹ️ 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".
| let mut normalized = <S as Borrow<str>>::borrow(field).to_owned(); | ||
| normalizer(&mut normalized); |
There was a problem hiding this comment.
For every v0.4 span—even when all fields are already normalized—this helper copies each service, name, type, and optional env value into a new String; the resource path performs another unconditional copy. Since normalization runs per span on the trace ingestion hot path, this adds four or more transient heap allocations per span and defeats the zero-copy benefit of the generic span representation. Add borrowed fast paths or lazily allocate only after detecting that a field needs normalization.
AGENTS.md reference: AGENTS.md:L76-L76
Useful? React with 👍 / 👎.
| let first_trace_id = first_span.trace_id; | ||
| anyhow::ensure!( | ||
| first_trace_id != 0, | ||
| "TraceID is zero (reason:trace_id_zero)" |
There was a problem hiding this comment.
When a v0.4 span has a nonzero upper half but a zero lower half, such as trace_id == 1 << 64, this full-width check accepts it. Both the native v0.4 MessagePack encoder and the agentless v0.4 encoder serialize only trace_id as u64, however, so the normalized trace is emitted with trace ID zero and is rejected downstream. Validate that the low 64-bit v0.4 wire ID is nonzero before accepting the trace.
Useful? React with 👍 / 👎.
| // Normalize parent_id using u128 trace_id comparison for Zipkin root span detection | ||
| let parent_id_as_u128 = u128::from(span.parent_id); | ||
| if parent_id_as_u128 == trace_id && trace_id == u128::from(span.span_id) { | ||
| span.parent_id = 0; |
There was a problem hiding this comment.
For a 128-bit Zipkin trace where low64(trace_id) == span_id == parent_id, the v0.4 wire representation carries the low half as its trace ID and the high half separately in _dd.p.tid, so the existing u64 normalizer would recognize this as the special Zipkin root. Comparing against the full u128 value leaves the self-parent intact, causing root-dependent processing such as stats and _trace_root injection to miss the span. Apply the existing normalize_parent_id logic to the low 64 bits.
Useful? React with 👍 / 👎.
VianneyRuhlmann
left a comment
VianneyRuhlmann
left a comment
There was a problem hiding this comment.
Can you add more context about why you need this ?
| // Normalize resource using helper: borrow span.name, allocate one String for resource, compare, | ||
| // replace if different | ||
| let name_ref = <T::Text as Borrow<str>>::borrow(&span.name); | ||
| let mut resource_str = <T::Text as Borrow<str>>::borrow(&span.resource).to_owned(); |
There was a problem hiding this comment.
Maybe it's worth implementing Into\<String> on SpanText to make it simpler
|
|
||
| // Normalize resource using helper: borrow span.name, allocate one String for resource, compare, | ||
| // replace if different | ||
| let name_ref = <T::Text as Borrow<str>>::borrow(&span.name); |
There was a problem hiding this comment.
| let name_ref = <T::Text as Borrow<str>>::borrow(&span.name); | |
| let name_ref: &str = &span.name.borrow(); |
|
Closing for now after discussing about the tradeoffs IRL |
2 participants
What does this PR do?
Add a
normalize_trace_v04function, that performs normalization on our internal span representation.Motivation
This will be used to normalize traces entering the data-pipeline, before stats computation happens, in the goal of having client side stats equivalent to agent side stats
Additional Notes
How to test the change?
Describe here in detail how the change can be validated.
[generated using Codex]