refactor(span)!: use VecMap for meta, metrics and meta_struct for v04 spans#2043
Open
yannham wants to merge 14 commits into
Open
refactor(span)!: use VecMap for meta, metrics and meta_struct for v04 spans#2043yannham wants to merge 14 commits into
meta, metrics and meta_struct for v04 spans#2043yannham wants to merge 14 commits into
Conversation
|
Contributor
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. |
Contributor
📚 Documentation Check Results📦
|
Contributor
🔒 Cargo Deny Results📦
|
yannham
changed the title
meta, metrics and meta_struct
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## yannham/vecmap-as-dedup #2043 +/- ##
===========================================================
- Coverage 72.92% 72.91% -0.01%
===========================================================
Files 460 460
Lines 76481 76558 +77
===========================================================
+ Hits 55774 55824 +50
- Misses 20707 20734 +27
🚀 New features to boost your workflow:
|
yannham
force-pushed
the
yannham/span-vec-fields-integration
branch
from
1c727ec to
a9e644d
Compare
yannham
commented
May 26, 2026
| size += k.as_ref().len() + v.as_ref().len(); | ||
| } | ||
| for k in self.metrics.keys() { | ||
| for (k, _) in &self.metrics { |
Contributor
Author
There was a problem hiding this comment.
TODO: this is wrong, because keys can be duplicated.
Solution 1: don't dedup. The size will be over-estimated.
Solution 2: deduplicate here. Costly (2 times with serialization).
Solution 3: deduplicate upfront (and eg keep a dirty flag in the vecmap to avoid double dedup).
Contributor
There was a problem hiding this comment.
Now that we have a dirty flag maybe this should be changed to solution 3?
Contributor
Author
There was a problem hiding this comment.
As explained in the PR description, I initially thought we shouldn't explicitly dedup here and just hope for the best (still try to dedup before estimation as much as possible, but if not, it's ok to overestimate).
That being said, now that we have defensive_dedup(), if we expect spans to be almost always deduped, maybe it's also reasonable to use it here...
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
|
yannham
force-pushed
the
yannham/span-vec-fields-integration
branch
2 times, most recently
from
681d36f to
95d3d03
Compare
yannham
changed the title
meta, metrics and meta_structmeta, metrics and meta_struct
yannham
changed the title
meta, metrics and meta_structmeta, metrics and meta_struct for v04 spans
![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: 95d3d03685
ℹ️ 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".
yannham
force-pushed
the
yannham/span-vec-fields
branch
from
74358f8 to
7e38b23
Compare
yannham
force-pushed
the
yannham/span-vec-fields-integration
branch
3 times, most recently
from
2d45a7f to
8f43f82
Compare
yannham
force-pushed
the
yannham/span-vec-fields-integration
branch
from
d3b289b to
3dc15ea
Compare
The trace_buffer benchmark was still using HashMap for Span meta, metrics, and meta_struct fields after the VecMap migration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The decoder functions were returning Vec<(K, V)> which callers immediately converted to VecMap via .into(). Return VecMap directly to avoid the unnecessary intermediate type. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
yannham
force-pushed
the
yannham/span-vec-fields-integration
branch
from
3dc15ea to
57b3ec4
Compare
yannham
commented
May 28, 2026
| /// from a static str and check if the string is empty. | ||
| pub trait SpanText: Debug + Eq + Hash + Borrow<str> + Serialize + Default + From<String> { | ||
| pub trait SpanText: | ||
| Debug + Eq + Hash + Clone + Borrow<str> + Serialize + Default + From<String> |
Contributor
Author
There was a problem hiding this comment.
I think it's ok to add Clone, since it can always be derived as From<String> + Borrow<str> (although in a potentially unoptimal way) ?
Contributor
There was a problem hiding this comment.
Why do you need it? SpanText used to be Clone until I removed it for the python span text (because cloning without allocation requires holding the GIL)
I think we don't want people to use this API in general, even if it's derivable as it makes it harder to make performance issues
Contributor
There was a problem hiding this comment.
Ok I think I found why...
VecMap::dedup does self.data.retain(|(k, _)| seen.insert(k.clone())); reuiring the Clone trait bound on keys, which I don't think is actually required. You could just insert a ref (I think?)
Contributor
Author
There was a problem hiding this comment.
It's not possible, because the reference to the keys retain gives to your closure you is short-lived. And for a good reason: since retain (or anything else that removes duplicates while keeping the vec contiguous) mutates the vector and move stuff around, inner references are not preserved. One solution is to do it in two passes (remembering in the first which indices to keep or not, e.g in a Vec<bool>). I believe it's also possible to implement a custom (probably unsafe) retain to do it in one go (you store the reference to the key after you've shifted the first occurrence found, so you know it won't move anymore). Though with the second version we lose some stdlib niceties around panic safety, or we would have to re-implement that part ourselves as well.
Contributor
Author
There was a problem hiding this comment.
I'm tempted to say let's go with the double pass for now. It's safe, it does allocate a Vec<bool> but we need to allocate a HashSet already, so it's not like it makes a big difference. Leaving a one-pass, unsafe function for later performance work.
Contributor
Author
There was a problem hiding this comment.
Implemented in #2069. Meanwhile, clone bound removed from this PR.
yannham
commented
May 28, 2026
| const PARTIAL_VERSION_KEY: &str = "_dd.partial_version"; | ||
|
|
||
| fn set_top_level_span<T>(span: &mut Span<T>, is_top_level: bool) | ||
| fn set_top_level_span<T>(span: &mut Span<T>) |
Contributor
Author
There was a problem hiding this comment.
unrelated change, but is_top_level is always used with false, so... it does get rid of a remove call.
yannham
force-pushed
the
yannham/span-vec-fields-integration
branch
from
ce67621 to
7328f51
Compare
| size += k.as_ref().len() + v.as_ref().len(); | ||
| } | ||
| for k in self.metrics.keys() { | ||
| for (k, _) in &self.metrics { |
Contributor
There was a problem hiding this comment.
Now that we have a dirty flag maybe this should be changed to solution 3?
| /// from a static str and check if the string is empty. | ||
| pub trait SpanText: Debug + Eq + Hash + Borrow<str> + Serialize + Default + From<String> { | ||
| pub trait SpanText: | ||
| Debug + Eq + Hash + Clone + Borrow<str> + Serialize + Default + From<String> |
Contributor
There was a problem hiding this comment.
Why do you need it? SpanText used to be Clone until I removed it for the python span text (because cloning without allocation requires holding the GIL)
I think we don't want people to use this API in general, even if it's derivable as it makes it harder to make performance issues
| /// from a static str and check if the string is empty. | ||
| pub trait SpanText: Debug + Eq + Hash + Borrow<str> + Serialize + Default + From<String> { | ||
| pub trait SpanText: | ||
| Debug + Eq + Hash + Clone + Borrow<str> + Serialize + Default + From<String> |
Contributor
There was a problem hiding this comment.
Ok I think I found why...
VecMap::dedup does self.data.retain(|(k, _)| seen.insert(k.clone())); reuiring the Clone trait bound on keys, which I don't think is actually required. You could just insert a ref (I think?)
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
3 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.
Depends on #2022 and #2049.
What does this PR do?
This PR continues the native span performance work required to land native spans in dd-trace-js. Follow-up of #2022. Actually use
VecMapin theSpandata structure.Motivation
Performance improvement following dd-trace-js native span experiments. See #2022.
Additional considerations
There are some deduplication design choices. We deduplicate before serializing to the agent, because while the current agent implementation would support duplicate keys in a map (with the same semantics of "last one wins"), this is not part of the spec/API, so we can't rely on it. We also deduplicate defensively in the msgpack encoder, but it should already be deduped at this stage.
For byte estimate (
byte_size()), we make the choice of not deduplicating. This means potentially overestimating the size of a chunk (and reaching the limit sooner), in exchange of delaying the deduplication to serialization time.How to test the change?
Run tests.