refactor(change-buffer)!: replace slot index with span_id, fix segment isolation#2105
Merged
gh-worker-dd-mergequeue-cf854d[bot] merged 9 commits intoJun 18, 2026
Merged
Conversation
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 953dc2e | Docs | Datadog PR Page | Give us feedback! |
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. |
![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: 29b0d7d5bd
ℹ️ 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".
rochdev
changed the title
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
|
danielsn
reviewed
Jun 11, 2026
| let (mut span, segment_id) = self | ||
| .spans | ||
| .remove(span_id) | ||
| .ok_or(ChangeBufferError::SpanNotFound(*span_id))?; |
Contributor
There was a problem hiding this comment.
If this fails, we stop removing the other spans. Is that what we want?
Comment on lines
+871
to
+877
| // Segment 1 — span_id=1: origin → "origin-A" (string id 0) | ||
| w.op(OP_CREATE, 1); // span_id=1 in header | ||
| w.u128(TRACE_ID); | ||
| w.u64(1); // segment_id=1 | ||
| w.u64(0); // parent_id | ||
| w.op(OP_SET_TRACE_ORIGIN, 1); | ||
| w.u32(0); // string_id → "origin-A" |
Contributor
There was a problem hiding this comment.
Should this have a helper function?
| deferred_meta_at(&mut self.deferred_meta, slot)?.insert(key_id, val_id); | ||
| if !cache.deferred_meta.is_null() { | ||
| // SAFETY: pointer into self.deferred_meta, valid per function contract. | ||
| unsafe { &mut *cache.deferred_meta }.insert(key_id, val_id); |
Contributor
There was a problem hiding this comment.
Are we guaranteed to enforce stacked-borrows here?
Contributor
There was a problem hiding this comment.
Yes, because nothing else is touching self.spans in any way (creating mutable or immutable references) while we materialize a mutable reference to some span. Basically we could use HashMap::get_mut for the lifetime of the materialized mutable span reference instead, but of course the whole point of the pointer is to avoid having to make a hashmap fetch.
gh-worker-dd-mergequeue-cf854d
Bot
requested a review
from a team
as a code owner
June 12, 2026 14:00
yannham
force-pushed
the
rochdev/change-buffer-cleanup
branch
from
June 15, 2026 13:51
a5fe934 to
d278eae
Compare
… isolation Segment isolation bug When service A calls B which calls back to A, both of A's segments share the same trace_id. The segment-level state map (origin, meta, metrics) was keyed by trace_id, so SetTraceOrigin/SetTraceMetaAttr/ SetTraceMetricsAttr written for segment 2 would overwrite segment 1's values before it was flushed. Fix: add segment_id (u64) to the Create op so each segment gets its own isolated entry in SmallSegmentMap. Slot index removal Operations previously carried a u32 slot_index — a JS-managed array index into parallel Vecs (spans, deferred_meta, deferred_metrics, segment_ids). JS had to allocate and free slot numbers, silently reusing a slot would drop a live span, and ensure_slot had to grow all four Vecs in lockstep on every Create. Replaced with span_id (u64) as the natural key into HashMaps, removing the slot allocation contract entirely. Opcode size The opcode was encoded as u64 because JS was writing via setBigUint64. With only 17 opcodes this wastes 6 bytes per op. Reduced to u16. SpanHeader removal materialize_header was a direct-write path where JS wrote span fields into a repr(C) struct in WASM memory, bypassing the change buffer protocol. It had no segment_id field and would have reproduced the isolation bug. It was not in use, so deleted along with span_header.rs. Rename Trace→Segment SmallTraceMap/Trace renamed to SmallSegmentMap/Segment since the map holds per-segment state, not per-trace state. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…buffer corruption Co-locate segment_id with its span in spans: HashMap<u64, (Span<T>, u64)>, removing the separate segment_ids: HashMap<u64, u64>. This eliminates a second HashMap lookup on every op that needs the segment_id. Group the 5 cache state variables in flush_change_buffer into a SpanCache<T> struct, fixing the too_many_arguments clippy lint on interpret_operation_cached. Fix a buffer-corruption bug (P1) where BatchSetMeta/BatchSetMetric in the cached path read the count but skipped the payload loop when cached_deferred_meta/cached_deferred_metrics was null (e.g. after materialize_span was called between flushes). The unread bytes were then decoded as the next op's opcode, corrupting all subsequent ops in the buffer. The fix moves the null-check inside the loop so bytes are always consumed. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
yannham
force-pushed
the
rochdev/change-buffer-cleanup
branch
from
June 15, 2026 15:12
f18ba51 to
e85b3fd
Compare
yannham
approved these changes
Jun 15, 2026
yannham
left a comment
yannham
left a comment
Contributor
There was a problem hiding this comment.
Reviewed with Roch, but since I also rebased/amended the PR, it'd better to have another pair of eyes (and a stamp).
paullegranddc
approved these changes
Jun 17, 2026
paullegranddc
left a comment
paullegranddc
left a comment
Contributor
There was a problem hiding this comment.
No objection on principle, but did you benchmark this on dd-trace-js vs the previous implementation?
Member
Author
The previous implementation was incorrect, so it doesn't matter if it was faster. Also, we shouldn't be relying on people benchmarking on their machines, they should be automated in this repo. |
Contributor
Hey, this is precisely what I'm trying to do this week 🙂 |
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: queued
and removed
mergequeue-status: queued
labels
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
gh-worker-dd-mergequeue-cf854d
Bot
deleted the
rochdev/change-buffer-cleanup
branch
June 18, 2026 10:21
This was referenced Jul 3, 2026
iunanua
added a commit
that referenced
this pull request
Jul 7, 2026
…ibdd-data-pipeline, libdd-li... (#2201) # Release proposal for libdd-capabilities-impl, libdd-common, libdd-data-pipeline, libdd-library-config, libdd-remote-config, libdd-sampling, libdd-telemetry, libdd-tinybytes, libdd-trace-utils and their dependencies This PR contains version bumps based on public API changes and commits since last release. ## libdd-capabilities **Next version:** `2.1.0` **Semver bump:** `minor` **Tag:** `libdd-capabilities-v2.1.0` ### Commits - feat(data-pipeline)!: add stdout log trace exporter (#2074) ## libdd-common **Next version:** `5.1.0` **Semver bump:** `minor` **Tag:** `libdd-common-v5.1.0` ### Commits - refactor(clippy): prefer core and alloc imports (#2196) - fix: update rustls-webpki to 0.103.13 (#2187) - fix: update anyhow for unsoundness (#2186) - feat(machine id): Add helpers in ddcommon to fetch the machine UUID l… (#2163) ## libdd-ddsketch **Next version:** `1.1.0` **Semver bump:** `minor` **Tag:** `libdd-ddsketch-v1.1.0` ### Commits - feat(data-pipeline)!: export client-computed span stats as OTLP trace metrics (#2067) - test(ddsketch): add microbenchmarks for add/encode/collapse (#2125) ## libdd-trace-protobuf **Next version:** `4.0.0` **Semver bump:** `major` **Tag:** `libdd-trace-protobuf-v4.0.0` ### Commits - chore!: update protobufs to be in sync with datadog-agent (#2180) - feat(stats)!: add whole key cardinality limit (#2158) - feat(remote-config)!: use the proto file from the agent (#2165) - feat(data-pipeline): OTLP HTTP/protobuf trace export (#2115) ## libdd-capabilities-impl **Next version:** `3.0.0` **Semver bump:** `major` **Tag:** `libdd-capabilities-impl-v3.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.1.0 → ^5.1.0 ### Commits - feat(data-pipeline)!: add stdout log trace exporter (#2074) ## libdd-library-config **Next version:** `3.0.0` **Semver bump:** `major` **Tag:** `libdd-library-config-v3.0.0` ###⚠️ major bump forced due to: - `libdd-trace-protobuf`: ^3.0.2 → ^4.0.0 ### Commits - refactor(clippy): prefer core and alloc imports (#2196) - feat(library-config)!: caller-supplied threadlocal schema and extra process-context attributes (#2162) - fix(otel-thread-ctx): put the threadlocal attributes at the right place in the context (#2167) ## libdd-remote-config **Next version:** `2.0.0` **Semver bump:** `major` **Tag:** `libdd-remote-config-v2.0.0` ###⚠️ major bump forced due to: - `libdd-trace-protobuf`: ^3.0.2 → ^4.0.0 ### Commits - refactor(libdd-remote-config)!: hide Target inner properties so they are not leaked (#2182) - feat(remote-config)!: use the proto file from the agent (#2165) - refactor(rc): reexport Endpoint and Tag common types (#2147) ## libdd-trace-normalization **Next version:** `3.0.0` **Semver bump:** `major` **Tag:** `libdd-trace-normalization-v3.0.0` ###⚠️ major bump forced due to: - `libdd-trace-protobuf`: ^3.0.1 → ^4.0.0 ### Commits - feat(data-pipeline)!: CSS Trace Filters (#1985) ## libdd-shared-runtime **Next version:** `2.0.0` **Semver bump:** `major` **Tag:** `libdd-shared-runtime-v2.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.1.0 → ^5.1.0 ### Commits - feat(shared-runtime)!: SharedRuntime Borrowed & Owned mode (#2061) - feat(shared-runtime)!: use weak waker in trigger [APMSP-3371] (#2050) ## libdd-trace-utils **Next version:** `9.0.0` **Semver bump:** `major` **Tag:** `libdd-trace-utils-v9.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.2.0 → ^5.1.0 - `libdd-trace-protobuf`: ^3.0.2 → ^4.0.0 ### Commits - ci(miri): skip slow miri tests (#2188) - chore!: update protobufs to be in sync with datadog-agent (#2180) - feat(data-pipeline): add agentless export (#2081) - feat(data-pipeline)!: add stdout log trace exporter (#2074) - feat(data-pipeline): OTLP HTTP/protobuf trace export (#2115) - feat(otlp)!: Export OTLP spans with attribute-level OTel compatibility (#2091) - test(trace-utils): add V05 msgpack decode microbenchmark (#2127) - feat(data-pipeline)!: export client-computed span stats as OTLP trace metrics (#2067) - test(trace-utils): add VecMap microbenchmarks (#2126) - chore(stats)!: submit p0 telemetry in stats (#2130) - refactor(change-buffer)!: replace slot index with span_id, fix segment isolation (#2105) - feat(data-pipeline)!: CSS Trace Filters (#1985) - feat(trace-exporter): add v1 span and its encoder (#2039) - fix(trace-utils): mark decoded span maps as deduped (#2110) - feat(trace-utils)!: change buffer implementation (#2055) - feat(native-spans)!: change buffer foundation (#2046) - refactor(span)!: use VecMap for `meta`, `metrics` and `meta_struct` for v04 spans (#2043) - test: fix timeouts on heavily contended scenarios (#2093) ## libdd-telemetry **Next version:** `6.0.0` **Semver bump:** `major` **Tag:** `libdd-telemetry-v6.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.2.0 → ^5.1.0 - `libdd-shared-runtime`: ^1.0.0 → ^2.0.0 ### Commits - ci(miri): skip slow miri tests (#2188) - refactor(libdd-telemetry)!: avoid leaking libdd-common types in the public API (#2152) - feat(shared-runtime)!: SharedRuntime Borrowed & Owned mode (#2061) ## libdd-trace-obfuscation **Next version:** `5.0.0` **Semver bump:** `major` **Tag:** `libdd-trace-obfuscation-v5.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.2.0 → ^5.1.0 - `libdd-trace-protobuf`: ^3.0.2 → ^4.0.0 - `libdd-trace-utils`: ^8.0.0 → ^9.0.0 ### Commits - refactor(clippy): prefer core and alloc imports (#2196) - ci(miri): skip slow miri tests (#2188) - fix: update anyhow for unsoundness (#2186) ## libdd-trace-stats **Next version:** `6.0.0` **Semver bump:** `major` **Tag:** `libdd-trace-stats-v6.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.2.0 → ^5.1.0 - `libdd-shared-runtime`: ^1.0.0 → ^2.0.0 - `libdd-trace-protobuf`: ^3.0.2 → ^4.0.0 - `libdd-trace-utils`: ^8.0.0 → ^9.0.0 ### Commits - chore!: update protobufs to be in sync with datadog-agent (#2180) - feat(stats)!: send telemetry for cardinality limits (#2159) - feat(stats)!: add whole key cardinality limit (#2158) - fix(trace-stats)!: add grpc_method to aggregation key (#2151) - feat(shared-runtime)!: SharedRuntime Borrowed & Owned mode (#2061) - feat(data-pipeline)!: export client-computed span stats as OTLP trace metrics (#2067) - refactor(span)!: use VecMap for `meta`, `metrics` and `meta_struct` for v04 spans (#2043) ## libdd-data-pipeline **Next version:** `7.0.0` **Semver bump:** `major` **Tag:** `libdd-data-pipeline-v7.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.2.0 → ^5.1.0 - `libdd-shared-runtime`: ^1.0.0 → ^2.0.0 - `libdd-telemetry`: ^5.0.1 → ^6.0.0 - `libdd-trace-protobuf`: ^3.0.2 → ^4.0.0 - `libdd-trace-stats`: ^5.0.0 → ^6.0.0 - `libdd-trace-utils`: ^8.0.0 → ^9.0.0 ### Commits - feat(trace_exporter): enable telemetry in stats exporter (#2160) - refactor(libdd-telemetry)!: avoid leaking libdd-common types in the public API (#2152) - feat(stats): emit canonical gRPC status name for OTLP rpc.response.status_code (#2183) - feat(data-pipeline): add agentless export (#2081) - feat(stats)!: send telemetry for cardinality limits (#2159) - feat(stats)!: add whole key cardinality limit (#2158) - fix(trace-stats)!: add grpc_method to aggregation key (#2151) - feat(data-pipeline)!: add stdout log trace exporter (#2074) - feat(shared-runtime)!: SharedRuntime Borrowed & Owned mode (#2061) - feat(data-pipeline): OTLP HTTP/protobuf trace export (#2115) - feat(otlp)!: Export OTLP spans with attribute-level OTel compatibility (#2091) - feat(data-pipeline)!: export client-computed span stats as OTLP trace metrics (#2067) - chore(stats)!: submit p0 telemetry in stats (#2130) - feat(data-pipeline)!: CSS Trace Filters (#1985) - feat(shared-runtime)!: use weak waker in trigger [APMSP-3371] (#2050) - refactor(span)!: use VecMap for `meta`, `metrics` and `meta_struct` for v04 spans (#2043) - feat(stats)!: add endpoint gating to client-side stats [APMSP-3361] (#2040) ## libdd-dogstatsd-client **Next version:** `4.0.0` **Semver bump:** `major` **Tag:** `libdd-dogstatsd-client-v4.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.1.0 → ^5.1.0 ## libdd-sampling **Next version:** `5.0.0` **Semver bump:** `major` **Tag:** `libdd-sampling-v5.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.2.0 → ^5.1.0 - `libdd-trace-utils`: ^8.0.0 → ^9.0.0 [APMSP-3371]: https://datadoghq.atlassian.net/browse/APMSP-3371?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: iunanua <[email protected]>
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
4 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.
Summary
trace_id, trace-level state (SetTraceOrigin,SetTraceMetaAttr,SetTraceMetricsAttr) was keyed bytrace_id, so segment 2's writes overwrote segment 1's values before it was flushed. Fixed by addingsegment_id(u64) to Create ops and keyingSmallSegmentMapby segment_id instead.u32 slot_index— a JS-managed array index into four parallelVecs. JS had to allocate/free slot numbers; reusing a slot prematurely would silently drop a live span;ensure_slothad to grow all four Vecs in lockstep. Replaced withspan_id(u64) as the natural key intoHashMaps, removing the slot allocation contract entirely.u64because JS wrote viasetBigUint64, wasting 6 bytes per op with only 17 opcodes. Reduced tou16.materialize_headerwas an unused direct-write path where JS wrote span fields into a#[repr(C)]struct in WASM memory, bypassing the change buffer protocol. It had nosegment_idfield and would have reproduced the isolation bug. Deleted along withspan_header.rs.Trace→Segment:SmallTraceMap/Tracerenamed toSmallSegmentMap/Segmentsince the map holds per-segment state, not per-trace state.Test plan
cargo test -p libdd-trace-utils --lib --features change-buffer -- change_bufferpasses (16 tests including 3 new segment isolation tests)cargo check -p libdd-trace-utils --features change-bufferclean