telemetry: add full-pipeline stress tests - #1951
Conversation
Add test-only stress coverage for the observable telemetry pipeline (concurrent producers -> metric mapping/activation -> staging queue -> periodic flush -> exporter -> observable output), complementing the direct staging-queue unit tests. The suite is written to pass unchanged on both the mutex-backed staging queue and a future low-lock implementation. - test/gtest/telemetry_stress_test.cpp: mixed concurrent value parity under capacity (per-type sent-vs-received value multiset equality), addXferStats batch atomicity, repeated queue/periodic-task lifecycle shake-out, and safe shutdown with an in-flight periodic flush -- all end-to-end through BUFFER. - test/gtest/telemetry_prometheus_stress_test.cpp: concurrent staging-overflow conservation and concurrent mixed-workload aggregation, sharing the fixture and scrape helpers extracted into telemetry_prometheus_test_util.h. - test/doca-telemetry/telemetry_doca_stress_test.cpp: high-volume exporter counter/gauge/drop accumulation, as its own doca_nixl_stress_test binary. No product, queue, exporter, schema, or public-API changes. Signed-off-by: Efraim Eygin <eeygin@nvidia.com>
|
👋 Hi e-eygin! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds high-volume and concurrent telemetry stress tests for Prometheus aggregation, event-ring integrity, batch atomicity, lifecycle shutdown, and DOCA exporter counters. Meson builds and registers the new DOCA stress executable and compiles the additional gtest sources. ChangesTelemetry stress coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/doca-telemetry/meson.build`:
- Around line 69-85: Review the doca_nixl_stress_test Meson test registration
and explicitly set a timeout that safely exceeds scrapeUntilValue’s 15-second
polling window plus setup and flush overhead, particularly for slower CI and
sanitizer builds.
In `@test/doca-telemetry/telemetry_doca_stress_test.cpp`:
- Line 82: Update the telemetry metric assertions in the stress test to use
telemetryMetricDescriptor.gaugeName instead of the hardcoded
"agent_tx_last_bytes" value, including the later assertion around lines 119–120.
Keep the existing assertion behavior while deriving the expected gauge name from
the descriptor.
In `@test/gtest/telemetry_prometheus_stress_test.cpp`:
- Around line 149-160: Update the producer setup in the telemetry stress test,
including the analogous section around the other producer case, to wait on a
shared latch or barrier before calling telemetry.addXferStats. Initialize the
gate before creating threads, have each producer await it, and release the gate
only after all producer threads have been created so both stress cases execute
concurrently.
- Around line 260-265: Update the convergence condition in the stress test
around metrics.latestValue to also read and compare both labeled error counters
against their expected totals before declaring convergence. Preserve the
existing checks for tx, rx, tx_req, and mem, ensuring the post-join scrape waits
until all six metric families are current.
In `@test/gtest/telemetry_stress_test.cpp`:
- Around line 392-460: Strengthen the atomicity assertions in the telemetry
stress test around the producer workload and RingTally validation: do not rely
only on equal aggregate counts with excess capacity. Configure a boundary where
partial batch insertion is observable, or retain event ordering/call identifiers
while draining the ring and verify each addXferStats call’s four records is
published as an indivisible batch.
- Around line 545-567: Update the telemetry stress test around nixlTelemetry
destruction so it deterministically waits for the periodic flush to enter before
calling telemetry.reset(), using a blocking test exporter or flush-entry hook.
Replace the in-process EXPECT_LT-only timing check with an external timeout
mechanism that still fails when reset() deadlocks, while preserving the
shutdown_budget assertion for prompt completion.
- Around line 69-74: Rename the C++ type aliases EventCounts and ValueHistograms
to lower-case _t-suffixed names such as event_counts_t and value_histograms_t,
and rename the related event type alias to event_type_t. Update every reference
in the telemetry stress test, including the additionally flagged locations,
without changing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: fa4f1e9a-e13f-4157-addf-93f9ab22100d
📒 Files selected for processing (5)
test/doca-telemetry/meson.buildtest/doca-telemetry/telemetry_doca_stress_test.cpptest/gtest/meson.buildtest/gtest/telemetry_prometheus_stress_test.cpptest/gtest/telemetry_stress_test.cpp
- Rename test type aliases to the repo's lower_case _t convention (event_counts_t, value_histograms_t, event_type_t). - Prove addXferStats atomicity by identity + contiguity: each call carries a unique per-call id and the drained ring is walked to verify every call's four events form one intact, ordered [post, transfer, bytes, requests] group with a consistent id, rejecting a torn or interleaved batch (equal aggregate counts under capacity did not prove this). - Bound the safe-shutdown destructor with an external timeout so a teardown deadlock fails as an assertion instead of hanging the test binary. - Release the concurrent Prometheus producers through a shared start gate so the workload runs concurrently rather than largely serially. - Wait for both labeled error counters before declaring convergence in the concurrent Prometheus aggregation test, avoiding a stale-scrape race. - Derive the DOCA last-op gauge name from telemetryMetricDescriptor.gaugeName instead of hardcoding it. - Give the doca_nixl_stress_test an explicit Meson timeout above its scrape-poll window. Signed-off-by: Efraim Eygin <eeygin@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/gtest/telemetry_stress_test.cpp`:
- Around line 465-513: Validate every event type’s range before using
eventIndex() to access counts in the manual ring walk. Apply the same guarded
behavior to the initial type and the xfer, bytes, and reqs members: report an
ADD_FAILURE for out-of-range values, avoid indexing counts, and preserve the
existing batch validation flow for in-range events.
- Around line 630-647: Update the shutdown task around telemetry destruction to
capture ownership by moving the telemetry unique_ptr into the callable, rather
than capturing the outer telemetry variable by reference. Ensure the outer
pointer is moved-from before the worker starts, so timeout-and-detach cannot
leave the thread referencing stack state while preserving exception propagation
on successful completion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: a1239c0b-2f9b-4dec-9bf3-397f0f7d795a
📒 Files selected for processing (4)
test/doca-telemetry/meson.buildtest/doca-telemetry/telemetry_doca_stress_test.cpptest/gtest/telemetry_prometheus_stress_test.cpptest/gtest/telemetry_stress_test.cpp
- Range-guard the batch-atomicity ring walk so corrupted ring content fails via ADD_FAILURE instead of indexing counts[] out of bounds (mirrors drainRing). - Fix a dangling-reference/data-race in the safe-shutdown test: move the telemetry unique_ptr into the destroy-worker task so the object lives in the worker's closure, not on the stack; on a timeout the abandoned deleter thread no longer references a destroyed local. Signed-off-by: Efraim Eygin <eeygin@nvidia.com>
|
/build |
|
/ok to test 9d85903 |
|
🤖 CI Triage Agent — TL;DR: The Docker image build failed because the aarch64 CI node could not reach Full analysisSummary: Stage "Compiling NIXL Docker Image for DL" (and the earlier build_helper attempts, which later passed on retry) failed at Dockerfile STEP 15/17 during Root cause: Implicated commit: None — infrastructure failure, unrelated to the PR commit [REDACTED:Hex High Entropy String]. File: Suggested fix:
Related: none
|
|
🤖 CI Triage Agent — TL;DR: The Docker image build (stages 150 & 125) failed because Full analysisSummary: Root cause: The aarch64 build agent could not connect to Implicated commit: unknown — not caused by commit 8c72cdd; the Dockerfile apt step is pre-existing and the error is a network outage. File: Suggested fix: Re-run the build once network connectivity to Related: none found. |
|
🤖 CI Triage Agent — TL;DR: The GPU CI node's UCX library was loaded without CUDA support ( Full analysisSummary: The "Run CPP tests" stage (node 203) failed with exit code 42; the sole genuine test failure was Root cause: On node Implicated commit: unknown — not a source regression; environment/UCX-image issue (related to the in-flight UCX version bump work). File: Suggested fix: Rebuild/repair the CI container so UCX is compiled and linked with working CUDA (and IB/RDMA) support, and verify the CUDA driver is accessible in the enroot/pyxis container on Related: PR #1868 (bump UCX to v1.22.x), PR #1743 (gtest single-process runner); build_helper stages 124/125 ( |
Replace the hand-rolled StartGate spin latches with std::latch, fold the DOCA volume case into the existing doca_nixl_test binary so the exporter tests keep a single invocation, gate that case's scrape on the last-written drop counter, and bound the staging-overflow drop assertion by the queue's drain capacity instead of relying on timing. Signed-off-by: Efraim Eygin <eeygin@nvidia.com>
|
/build |
|
/ok to test 09390e3 |
|
🤖 CI Triage Agent — TL;DR: The Jenkins build failed not in NIXL code but in the "Allocate DL EP Environment" stage: a Slurm Full analysisSummary: Slurm node allocation on the Root cause: The GPU test cluster ( Implicated commit: None — commit a65c9a2 (PR #1951) is not implicated; the build and Docker image steps for that commit passed. The failure is environmental. File: Not a source file. The relevant CI config is the Jenkins pipeline Suggested fix: This is a retryable infrastructure failure, not a code defect — re-run the job when GB200 capacity is available. To make CI more robust: (1) add automatic retry/backoff on Related: none |
The NIX-1205 stress tests (ai-dynamo#1951) landed while this branch was in review, using the scrape helpers from their old home: namespace nixl::doca_test in test/doca-telemetry/. This branch moves them to test/metrics/ as nixl::metrics_test, so the two files no longer compile together. Convert the two new stress sources, and drop the local scrapeMetrics copy in favour of the shared one, which additionally retries an endpoint that is not serving yet instead of parsing an empty body. Signed-off-by: Efraim Eygin <eeygin@nvidia.com>
The stress suite (ai-dynamo#1951) landed its own copy of the overflow-conservation harness while this branch was extracting the same logic into telemetry_core_scrape.h. Keep one, taking its arguments as a spec struct so the two knobs the stress case needs -- a 50 ms flush interval to keep drops guaranteed under concurrent producers, and a sanitizer-scaled settle timeout -- are named at the call site instead of being a second implementation. Signed-off-by: Efraim Eygin <eeygin@nvidia.com>
3 participants
What?
Test-only, full-pipeline stress coverage for telemetry: concurrent producers through the staging queue → periodic flush → the BUFFER / Prometheus / DOCA exporters. No product, queue, exporter, schema, or public-API changes.
test/gtest/telemetry_stress_test.cpp— value parity,addXferStatsbatch atomicity, lifecycle shake-out, safe shutdown (BUFFER, end-to-end).test/gtest/telemetry_prometheus_stress_test.cpp— concurrent staging-overflow conservation and mixed-workload aggregation.test/doca-telemetry/telemetry_doca_stress_test.cpp— high-volume DOCA exporter accumulation (standalonedoca_nixl_stress_test).Why?
Closes NIX-1205. Existing tests cover queue mechanics and single-threaded exporters, but not the concurrent observable pipeline. This pins down that contract — conservation, all-or-none batching, per-value fidelity, shutdown safety — so a regression anywhere in the path fails an assertion. It builds only on already-merged behavior (NIX-454 #1887, NIX-453 #1919), so it is independent of NIX-1542 and mergeable on its own, while serving as the validation gate for the NIX-1542/NIX-1544 queue changes.
How?
Deterministic and timing-independent: conservation identities and value multisets rather than fixed accepted/dropped splits, bounded polling instead of sleeps, and sanitizer-scaled iteration counts.
Design & validation details
accepted + dropped == producedis exact and hardware-independent.addXferStatsatomicity is proven by per-call identity + contiguity in the drained ring (a torn or interleaved batch fails), not just aggregate counts.Summary by CodeRabbit
Tests