Repository navigation
refactor(ipc)!: make the shared memory table generic over a slot schema - #2673
Merged
arjunguhaswe merged 11 commits intoOct 27, 2026
Merged
arjunguhaswe merged 11 commits into
arjunguhaswe merged 11 commits into
Conversation
Split the lock-free shared memory table out of `shm_stats` into `shm_table`, so that products other than APM client stats can store a different slot layout without duplicating the table. `ShmTable<S: Schema>` holds everything that does not depend on what a slot contains: the header, layout validation, slot claiming, the two buckets, the string pool, and reload/replacement. A `Schema` defines the slot's key and value and how an input is hashed, matched and recorded. APM client stats become the first schema; `ShmSpanConcentrator` is now an alias with the same API, so callers do not change. The header records the schema ID and version, and opening a segment written with another schema fails. This changes the header layout, so `SHM_VERSION` goes from 3 to 4. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
📚 Documentation Check Results📦
|
Contributor
🔒 Cargo Deny Results📦
|
BenchmarksComparisonBenchmark execution time: 2026-10-27 14:48:19 Comparing candidate commit 3473333 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 2 metrics, 0 unstable metrics.
|
| cpu_model | git_commit_sha | git_commit_date | git_branch |
|---|---|---|---|
| Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz | 3473333 | 1793111888 | arjun.guha/shm-table-generic |
| scenario | metric | min | mean ± sd | median ± mad | p75 | p95 | p99 | max | peak_to_median_ratio | skewness | kurtosis | cv | sem | runs | sample_size |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| two way interface | execution_time | 13.493µs | 13.915µs ± 0.191µs | 13.908µs ± 0.090µs | 13.992µs | 14.263µs | 14.581µs | 14.672µs | 5.49% | 0.966 | 2.083 | 1.37% | 0.013µs | 1 | 200 |
| scenario | metric | 95% CI mean | Shapiro-Wilk pvalue | Ljung-Box pvalue (lag=1) | Dip test pvalue |
|---|---|---|---|---|---|
| two way interface | execution_time | [13.889µs; 13.942µs] or [-0.190%; +0.190%] | None | None | None |
Group 2
| cpu_model | git_commit_sha | git_commit_date | git_branch |
|---|---|---|---|
| Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz | 3473333 | 1793111888 | arjun.guha/shm-table-generic |
| scenario | metric | min | mean ± sd | median ± mad | p75 | p95 | p99 | max | peak_to_median_ratio | skewness | kurtosis | cv | sem | runs | sample_size |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| write only interface | execution_time | 474.428ns | 510.599ns ± 8.100ns | 514.159ns ± 0.887ns | 514.728ns | 515.269ns | 516.074ns | 517.864ns | 0.72% | -2.815 | 8.194 | 1.58% | 0.573ns | 1 | 200 |
| scenario | metric | 95% CI mean | Shapiro-Wilk pvalue | Ljung-Box pvalue (lag=1) | Dip test pvalue |
|---|---|---|---|---|---|
| write only interface | execution_time | [509.477ns; 511.722ns] or [-0.220%; +0.220%] | None | None | None |
Baseline
Baseline benchmark details
Group 1
| cpu_model | git_commit_sha | git_commit_date | git_branch |
|---|---|---|---|
| Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz | 9c3464f | 1792756400 | feat/arjun.guha/dsm-php-integration |
| scenario | metric | min | mean ± sd | median ± mad | p75 | p95 | p99 | max | peak_to_median_ratio | skewness | kurtosis | cv | sem | runs | sample_size |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| two way interface | execution_time | 13.281µs | 13.611µs ± 0.218µs | 13.571µs ± 0.080µs | 13.671µs | 14.042µs | 14.438µs | 15.012µs | 10.62% | 2.449 | 10.170 | 1.60% | 0.015µs | 1 | 200 |
| scenario | metric | 95% CI mean | Shapiro-Wilk pvalue | Ljung-Box pvalue (lag=1) | Dip test pvalue |
|---|---|---|---|---|---|
| two way interface | execution_time | [13.581µs; 13.642µs] or [-0.222%; +0.222%] | None | None | None |
Group 2
| cpu_model | git_commit_sha | git_commit_date | git_branch |
|---|---|---|---|
| Intel(R) Xeon(R) Platinum 8259CL CPU @ 2.50GHz | 9c3464f | 1792756400 | feat/arjun.guha/dsm-php-integration |
| scenario | metric | min | mean ± sd | median ± mad | p75 | p95 | p99 | max | peak_to_median_ratio | skewness | kurtosis | cv | sem | runs | sample_size |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| write only interface | execution_time | 476.768ns | 508.109ns ± 13.624ns | 514.846ns ± 2.361ns | 516.459ns | 517.722ns | 523.802ns | 527.833ns | 2.52% | -1.369 | 0.444 | 2.67% | 0.963ns | 1 | 200 |
| scenario | metric | 95% CI mean | Shapiro-Wilk pvalue | Ljung-Box pvalue (lag=1) | Dip test pvalue |
|---|---|---|---|---|---|
| write only interface | execution_time | [506.221ns; 509.997ns] or [-0.372%; +0.372%] | None | None | None |
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 3473333 | Docs | View more details | Give us feedback! |
Author
|
Linking DSM2-464 |
2 tasks
bwoebi
reviewed
Oct 14, 2026
…s module Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
arjunguhaswe
added a commit
to DataDog/dd-trace-php
that referenced
this pull request
Oct 14, 2026
Picks up the review fix to the shared memory table refactor (DataDog/libdatadog#2673); no functional change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The sidecar only logs a warning when a bucket passes this ratio. It never recreates or grows the table, which the old comment implied. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
arjunguhaswe
marked this pull request as ready for review
October 16, 2026 04:15
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. |
Author
|
@bwoebi This looks good from my POV. Can't make a determination on "code style" since I don't have experience in rust, but the division of responsibility between [PHP callers] <----> [Product Specific Schema Definition] <-----> [Shared Table Code] is quite reasonable. Marking "Ready for Review" but feel free to raise any concerns on structure/approach. (I'll also find some time to cleanup the PR desc and add test links before merging and ask for re-review, but the code is good to look at as-is). |
arjunguhaswe
changed the base branch from
main
to
feat/arjun.guha/dsm-php-integration
October 19, 2026 14:54
yannham
approved these changes
Oct 21, 2026
yannham
left a comment
yannham
left a comment
Contributor
There was a problem hiding this comment.
No hard blocker, but some safety improvement suggestions.
| /// byte expanding to a three-byte replacement character. | ||
| pub(crate) fn pool_string(pool: &Pool, sr: StringRef, budget: &mut usize) -> String { | ||
| let Some(bytes) = pool_slice(pool, sr) else { | ||
| return String::new(); |
Contributor
There was a problem hiding this comment.
Returning an empty string on error is a bit fishy. Should this option return an Option<String> instead? They both have the same layout and same size thanks to the niche optimization.
Author
There was a problem hiding this comment.
I see no downside to this approach, but it looks like None can only be returned in situations where the writer thread exceeds the budget of the string pool -- which given it's sizing in respect to the slot limit can't happen unless there's some form of corruption.
Will make the change anyway since it appears to be more idiomatic.
| Some(bytes) if bytes.len() == expected.len() => { | ||
| // Deliberately use non-atomic byte equality for this fixed, checked range. | ||
| // A peer may change the match result; no byte is interpreted as metadata or UTF-8. | ||
| let bytes = |
Contributor
There was a problem hiding this comment.
This is technically UB to materialize a slice to memory that can be mutated by someone else. The naive method would be to compare byte per byte atomically but that is costly, or to use extern "C" memcmp to perform the comparison.
Note sure this is worth doing here. But I would still write an explicit safety disclaimer (we know this is technically UB, but there's no easy way to go around it while maintaining performance in Rust).
Contributor
There was a problem hiding this comment.
It's also UB to perform a libc::memcmp on something which gets concurrently modified. (just not Rusts UB.)
The SHM protocol expects that, once placed there, there's no writing access until a minute has passed and the sections are swapped and cleared.
Contributor
There was a problem hiding this comment.
Ah, I had the impression that recent C++ had an atomic memcpy version of some sort, and that Rust was lagging behind. But apparently not 🤔 so I guess it would either be atomic byte-per-byte copy or unportable assembly. Anyway I think you add useful information @bwoebi , I leave this comment unresolved because some of it might be useful to include in the code comment.
| if old.as_slice().len() >= size_of::<ShmHeader>() { | ||
| // SAFETY: page-aligned, in bounds, and all fields are integer atomics. | ||
| // Touch only `ready`; the old segment's layout is untrusted. | ||
| let hdr = unsafe { &*(old.as_slice().as_ptr() as *const ShmHeader) }; |
Contributor
There was a problem hiding this comment.
This still relies on ready being at the same place in the new and old layout, but ready could theoretically move in future versions, right? I would suggest to add a little compile-time test to ensure the offset of ready is fixed. Just slap const { assert!(std::mem::offset_of!(ShmHeader, ready) == 4) } somewhere in this module file, for example.
Also, instead of materializing &ShmHeader, which has some additional safety requirements, I think it would be safer to just use raw pointers all the way down, something like:
const READY_OFFSET: usize = std::mem::offset_of!(ShmHeader, ready);
// SAFETY: we always access the header through atomics, so there's no interleaved atomic and non-atomic accesses.
// The location is allocated and valid for read and writes.
AtomicU8::from_ptr(old.as_slice().as_ptr().cast_mut().cast::<u8>().add(READY_OFFSET))
.store(0, Relaxed);…ion' into arjun.guha/shm-table-generic
`Schema` was an unsafe trait whose safety contract was that `Key` and `Value` are made only of integer atomics. Encode that at the type level instead, with the `ShmSafe` trait from the rate limiter: `Key` and `Value` must now be `ShmSafe`, and `Schema` itself becomes a safe trait. Add `ShmSafe` impls for `AtomicU8`, `AtomicU32` and arrays of `ShmSafe` types, which the span stats key and value are built from. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
rustfmt wraps the import that adding ShmSafe made too long. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…m-table-generic Keeps this branch's shm_stats.rs, whose module doc no longer has the Untrusted contents section that #2694 edited on main. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ShmSpanConcentrator is now an alias of the shared table type, which lives in the private shm_table module, so rustdoc treats links to its methods, to ShmKeyHeader::scalars and to crate::shm_table as private and the new doc lint job (#2695) fails on them. Name them in code spans instead, as #2694 did for Layout. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
arjunguhaswe
added a commit
that referenced
this pull request
Oct 24, 2026
…s table key Merging arjun.guha/dsm-shm-table brought two changes the backlog code predates: - Schema is no longer an unsafe trait: its Key and Value must be ShmSafe instead (#2673). ShmBacklogKey and ShmBacklogValue now implement ShmSafe, and DataStreamsBacklogs implements Schema without unsafe, as DataStreams does. - DsmTableKey has no product any more (#2674), so add_dsm_backlog builds its key without one. Also name ShmDsmBacklogTable::add_backlog in code spans. It aliases the shared table type of the private shm_table module, so links to its methods fail the doc lint, as #2674 found for ShmDsmTable. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
arjunguhaswe
added a commit
to DataDog/dd-trace-php
that referenced
this pull request
Oct 26, 2026
…ration (#4314) * feat(dsm): add Data Streams Monitoring checkpoints to the Kafka integration Set a DSM checkpoint when rdkafka produces or consumes a message, propagate the pathway context in the `dd-pathway-ctx-base64` header, and record each checkpoint's stats point in the sidecar's shared-memory DSM table (falling back to IPC until it exists). - `DD_DATA_STREAMS_ENABLED` (default off) turns checkpoints and header injection on. Stats points are recorded once the agent's /info lists `/v0.1/pipeline_stats`. - components-rs/data_streams.rs: pathway hashing as in the Go tracer and as system-tests expects (FNV-1 64 over service, env, sorted edge tags, process tags and container tags hash, then over the parent hash), the Java/Go wire format, the request's current pathway, and recording. A consume takes its parent from the message; a produce from the request's pathway, with consecutive produces sharing one parent. - `DDTrace\Internal\dsm_set_checkpoint()` for integrations to call. - Kafka: checkpoints on `ProducerTopic::producev` (direction, topic, type) and on `KafkaConsumer::consume` / `Queue::consume` (direction, group, topic, type). `produce` is rerouted through `producev` when DSM is on, even with distributed tracing off. - Bump libdatadog to the DSM shared-memory table branch, and regenerate the C headers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * chore: bump libdatadog to the DSM branch head Picks up the review fix to the shared memory table refactor (DataDog/libdatadog#2673); no functional change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * chore: bump libdatadog to the DSM branch head Picks up a test fix in DataDog/libdatadog#2674; no functional change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * chore: bump libdatadog to the DSM branch head Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * chore: bump libdatadog to the DSM branch head Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(kafka): use the PHPUnit-version-aware regex assertion in the DSM test `assertMatchesRegularExpression` only exists from PHPUnit 9, so the Data Streams test errored on the PHP 7.0-7.2 jobs. Use BaseTestCase's `assertRegularExpression`, which falls back to `assertRegExp`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(dsm): group stats points by pathway hash Bump libdatadog to e9439c4b2, which drops the separate aggregation hash from DSM stats points: the pathway hash already folds in the parent's. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * chore: bump libdatadog to the DSM branch head Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(dsm): hash the process tags the sidecar reports, svc.* tag included The sidecar reports the process tags in the DSM payload followed by a service-source tag - svc.user:true when DD_SERVICE is set, svc.auto with the normalized default service name otherwise - while the pathway hash only covered the process tags. Append the same tag when hashing, under the same conditions: neither is sent while process tags are disabled. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Revert "fix(dsm): hash the process tags the sidecar reports, svc.* tag included" This reverts commit e9d87ed. The svc.* tag in the pathway hash moves to a stacked branch for now: under Kubernetes SSI the injector sets DD_SERVICE, so svc.user:true does not reliably mean the user chose the service. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(dsm): hash the DBM base hash instead of the process tags The process tags and the container tags hash that went into the node hash are already hashed together for DBM by datadog_process_tags_get_base_hash. Feed that base hash into the node hash in their place, and drop the container tags hash getter that only DSM used. With process tags off the base hash is empty, so those hashes do not change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(dsm): drop the product from the DSM table key Bump libdatadog to the DSM branch head, whose DsmTableKey no longer has a product field. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * feat(dsm): start the sidecar when DSM is enabled, and require it The sidecar owns the shared memory tables DSM writes its stats points to, and sends them, so DD_DATA_STREAMS_ENABLED now starts it, as the flagging provider and OTel metrics do. Checkpoints still require it (ddtrace_dsm_enabled), for when its setup fails or DSM is enabled for a single request only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * chore: bump libdatadog to the DSM branch head The DSM branch now includes libdatadog main up to #2717, so mirror its new workspace dependencies (httpdate, webpki-roots, webpki-root-certs) and its rustls-webpki 0.103.16 floor in the workspace manifest, and lock rustls-webpki 0.103.16, webpki-roots and webpki-root-certs 1.0.11 and the new libdd-trace-model crate. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(dsm): call DSM stats points checkpoints Follow libdatadog's rename of the DSM stats table to the checkpoint table (DataDog/libdatadog#2674): pin libdatadog to it, use its new names, and call what a checkpoint records a checkpoint rather than a stats point. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(dsm): refresh agent info before each checkpoint AGENT_SUPPORTS_DSM was only updated as a side effect of RINIT, root-span sampling or stats computation, so long-running workers whose agent /info arrived after RINIT could skip every DSM checkpoint. Re-read the agent info (a cheap no-op when unchanged) at the start of ddtrace_dsm_set_checkpoint so DSM support, env and the container tags hash are current. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * chore(dsm): remove unused DDTRACE_DSM_CONTEXT_KEY macro Nothing references it; the Kafka integration uses its own DSM_CONTEXT_HEADER constant for the pathway context header. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(kafka): give DSM produce and consume checkpoints matching methods Move the produce checkpoint out of setupKafkaProduceSpan into setDataStreamsProduceCheckpoint, which returns the pathway context, and move the DD_DATA_STREAMS_ENABLED and RD_KAFKA_RESP_ERR_NO_ERROR checks from the consume hook closure into setDataStreamsConsumeCheckpoint, so each method owns when its checkpoint is set. No behaviour change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * chore: bump libdatadog to shrink the DSM checkpoint table Pick up DataDog/libdatadog@fc0ebc3ba: 128 checkpoint slots per bucket and 1 KiB of edge tag text per checkpoint. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * chore: bump libdatadog to #2674's head Pick up DataDog/libdatadog@39a0c80af, #2674 with #2709 merged in, so the whole stack pins the same libdatadog commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * feat(dsm): report Kafka offsets and payload sizes to Data Streams Monitoring (#4352) * feat(dsm): report Kafka offsets and payload sizes to Data Streams Monitoring Completes the rdkafka DSM integration with the throughput and lag data (DSM2-450): - Checkpoints now carry the message size (value, key and headers), which fills the payload size sketch the sidecar already keeps. - Produce offsets are recorded from the delivery report callback, and committed offsets (automatic or not) from the offset commit callback, as DSM backlogs. The integration wraps the application's callbacks, and installs its own on Producer and KafkaConsumer where there are none. - New internal function DDTrace\Internal\dsm_track_backlog(), backed by ddog_dsm_track_backlog(), which goes into the sidecar's new backlog table (libdatadog) through shared memory, or IPC until that is open. Bumps libdatadog to the backlog table commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(dsm): only track backlogs when the sidecar is set up for DSM #4314 gates checkpoints on ddtrace_dsm_enabled(), which also requires the sidecar that aggregates and sends DSM data. Gate backlogs the same way, so an offset is never handed to a sidecar that is not there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * feat(dsm): add Data Streams Monitoring checkpoints to the AMQP integration (#4347) * feat(dsm): add Data Streams Monitoring checkpoints to the AMQP integration Set a DSM checkpoint when php-amqplib publishes or consumes a message (DSM2-449), with the Java tracer's RabbitMQ edge tags. - basic_publish: direction:out with topic:<routing key> for the default exchange, and exchange:<name> plus has_routing_key otherwise. The context goes in the dd-pathway-ctx-base64 header, set on a copy of the application's header table so its entries keep their AMQP types. It is sent whether or not distributed tracing is on. - basic_consume: a delivery names its consumer but not the queue, and basic_deliver reads the consumer only right before calling back. So the callback is wrapped: it knows its queue, and sets the direction:in, topic:<queue> checkpoint before the application's code runs. Each message runs on its own pathway, so what the callback publishes links to that message and nothing after it does. - basic_get: the checkpoint is set when the message is returned and stays the request's pathway, as for a Kafka consume. - Consumes are checkpointed with or without an incoming context. - New DDTrace\Internal\dsm_push_pathway() / dsm_pop_pathway() set the request's pathway aside and bring it back, for integrations that hand messages to callbacks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(amqp): don't call str_starts_with() in formatQueueName (PHP 7) str_starts_with() only exists from PHP 8.0. On PHP 7.x every AMQP hook that formats a queue name (basic_get, queue_declare, basic_consume, ...) threw "Call to undefined function", which the tracer swallows: those spans lost their tags, and basic_get's DSM consume checkpoint never ran, so a publish after basic_get started a new pathway instead of continuing the consumed one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * feat(dsm): report RabbitMQ payload sizes to Data Streams Monitoring Pass the message size with each AMQP checkpoint, which fills the payload size sketch the sidecar keeps for throughput (count is messages, sum is bytes), as the Kafka integration does since the previous commit. The size is the body plus the application headers, counted as the Node.js tracer counts them (the Java tracer reports none for RabbitMQ): header names and string values, nested tables and arrays included; numbers, booleans, timestamps and decimals count as 0. A publish is sized without the dd-pathway-ctx-base64 header the checkpoint computes, and a consume with it, as for Kafka. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(dsm): size RabbitMQ headers by their AMQP encoding Count each header entry's encoded bytes (name, type and value, with nested tables and arrays) with php-amqplib's own AMQPWriter, instead of only header names and string values. Numbers, booleans, timestamps and decimals now count at their wire width. The table's 4-byte length is left out, as is the context header on publish. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(dsm): note that pushed pathways aren't fiber safe Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> * test(dsm): use the PHPUnit-compatible string assertion in KafkaTest assertStringContainsString() is missing from PHPUnit 6, which PHP 7.0 runs, so testDataStreamsContextPropagation errored there. Use BaseTestCase::assertStringContains(), which falls back to assertContains(). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
arjunguhaswe
commented
Oct 27, 2026
| pub(crate) const SLOT_INIT: u64 = u64::MAX; | ||
|
|
||
| /// The sidecar should recreate the SHM when slot utilisation exceeds this ratio. | ||
| pub const RELOAD_FILL_RATIO: f64 = 0.80; |
Author
There was a problem hiding this comment.
I could see some of these values being product specific instead of base table properties.
bwoebi
approved these changes
Oct 27, 2026
bwoebi
left a comment
bwoebi
left a comment
Contributor
There was a problem hiding this comment.
This looks good to me, thanks!
A string reference that does not resolve, or does not fit the flush budget, now yields None instead of an empty string, so it can no longer be mistaken for a field that is really empty. The span stats schema drops the entry, as it already does for a key whose scalars fail to decode, rather than exporting the group under a blank field. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
pool_eq and pool_string read pool bytes through a plain slice while a peer may be writing them. Say that this is technically undefined behaviour, why the protocol makes it safe in practice, and why there is no cheap sound alternative. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…through it Retiring a replaced segment stored to `ready` through a `&ShmHeader` built from a shared slice over the old mapping. That relied on `ready` being where this version's layout puts it, and wrote through a pointer derived from a `&[u8]`, which may only read. Assert at compile time that `version` and `ready` keep their offsets, and store through an `AtomicU8` at that offset, using the mapping's own pointer so no reference over the shared bytes is made. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
arjunguhaswe
merged commit Oct 27, 2026
1dd3a27
into
feat/arjun.guha/dsm-php-integration
66 checks passed
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
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?
Splits the lock-free shared memory table out of
libdd-ipc/src/shm_stats.rsinto a newshm_tablemodule that is generic over what a slot holds.ShmTable<S: Schema>holds everything that doesn't depend on slot contents: the header, layout validation, slot claiming, the two buckets, the string pool, and reload/replacement. This code is moved, not rewritten.Schematrait defines a slot's key and value, and how an input is hashed, matched and recorded. It is anunsafe trait: keys and values must be#[repr(C)]integer atomics, because they live in memory every peer can write.ShmSpanConcentratoris now an alias forShmTable<SpanStats>with the same API, so the sidecar and the PHP tracer don't change.openrefuses a segment written with another schema. This changes the header layout, soSHM_VERSIONgoes from 3 to 4.Motivation
We're bringing Data Streams Monitoring to the PHP tracer. DSM needs the same shared memory table as client stats, but with a different slot: a pathway key and three histograms. Without this split we'd need a second copy of code that is both lock-free and hardened against untrusted memory. The DSM schema is in the stacked PR on top of this one.
Additional Notes
shm_statstests are byte-for-byte unchanged.!) because of theSHM_VERSIONbump, and becauseShmSpanConcentratoris now a type alias rather than a struct.start_nanosis only set when it is drained. So the first two flushes of a new concentrator reportstart = 0, and later buckets are dated one interval early. I reproduced this on the parent commit and can open a separate issue.curl_execdoes not appropriate taghttp.status_code, it appears asN/AHow to test the change?
New tests in
shm_tableuse a small test-only schema. They check that a segment only opens as the schema and version that created it, and that a schema can record into the table and drain it.Furthermore, this PR was tested in comparison with the current release branch 1.26.0, with several operations for APM client stats as noted in the table below. In all cases, number of stats points sent, their groups and their tags were all equivalent.
🤖 Generated with Claude Code