test: drive prometheus_mp through nixlTelemetry - #2054
Merged
ovidiusm merged 10 commits intoAug 18, 2026
Merged
Conversation
|
👋 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. 🚀 |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change moves telemetry test utilities to ChangesTelemetry test consolidation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_mp_e2e_test.cpp`:
- Around line 50-60: Update seriesByAgent to include the histogram bucket’s le
label when selecting or keying series, and adjust its callers/assertions to
validate the intended bucket (for example, le="+Inf"). Ensure distinct bucket
series no longer overwrite one another and that the test verifies bucket values
rather than only series presence.
🪄 Autofix
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: 6663a5fd-59c7-41de-ae5a-238830b122e3
📒 Files selected for processing (19)
test/doca-telemetry/histogram_parity.htest/doca-telemetry/meson.buildtest/doca-telemetry/telemetry_doca_nixl_test.cpptest/doca-telemetry/telemetry_doca_test.cpptest/doca-telemetry/telemetry_histogram_parity_test.cpptest/gtest/meson.buildtest/gtest/mp_telemetry_fixture.htest/gtest/telemetry_core_scrape.htest/gtest/telemetry_histogram_test.cpptest/gtest/telemetry_mp_collector_test.cpptest/gtest/telemetry_mp_core_test.cpptest/gtest/telemetry_mp_e2e_test.cpptest/gtest/telemetry_mp_exporter_test.cpptest/gtest/telemetry_mp_store_test.cpptest/gtest/telemetry_prometheus_test.cpptest/metrics/loopback_connection.htest/metrics/open_metrics_text_parser.htest/metrics/scrape_util.htest/metrics/timeseries.h
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/gtest/telemetry_mp_e2e_test.cpp (1)
135-137: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAcquire RAII ownership before the next fallible operation.
The
scopedFdmembers take ownership only after all threepipe()calls succeed. If the second or thirdpipe()fails, earlier raw descriptors leak. Iffork()fails, the raw read ends also remain open because the close block after the loop is skipped. Wrap both ends immediately after each successfulpipe()or add an equivalent cleanup guard.🤖 Prompt for 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. In `@test/gtest/telemetry_mp_e2e_test.cpp` around lines 135 - 137, Update the pipe setup in the telemetry test to transfer each successful pipe descriptor into RAII ownership immediately after its pipe() call, before attempting subsequent pipes or fork(). Ensure both read and write ends are owned or otherwise cleaned up on any pipe or fork failure, including the read ends currently closed only after the child-process loop.
🤖 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_mp_e2e_test.cpp`:
- Around line 205-207: Update the zero-observation assertions in the telemetry
test to iterate over all entries in kChildren rather than checking only agent-0.
For each child writer, assert hist_count and hist_sum are 0.0 and the +Inf
bucket is optional<double>(0.0), reusing the existing bucket helper and
writer-name format.
---
Outside diff comments:
In `@test/gtest/telemetry_mp_e2e_test.cpp`:
- Around line 135-137: Update the pipe setup in the telemetry test to transfer
each successful pipe descriptor into RAII ownership immediately after its pipe()
call, before attempting subsequent pipes or fork(). Ensure both read and write
ends are owned or otherwise cleaned up on any pipe or fork failure, including
the read ends currently closed only after the child-process loop.
🪄 Autofix
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: d852d46a-f468-4c0b-898d-b6f78fa2484b
📒 Files selected for processing (1)
test/gtest/telemetry_mp_e2e_test.cpp
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/gtest/telemetry_mp_e2e_test.cpp (1)
135-137: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winOwn all pipe descriptors on every setup path.
If a fatal
pipe()orfork()assertion exits before Lines 157-159,TearDown()resets onlygoWrite_,readyRead_, andquitWrite_. The other three descriptors remain raw and leak into later tests.Create scoped owners for all six pipe ends immediately after each successful
pipe(). Move or reset only the parent-owned ends after setup succeeds.🤖 Prompt for 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. In `@test/gtest/telemetry_mp_e2e_test.cpp` around lines 135 - 137, Update the pipe setup in the test fixture so all six descriptors returned by each successful pipe() are immediately wrapped in scopedFd owners. In the fork/setup flow around the existing goWrite_, readyRead_, and quitWrite_ assignments, retain or reset only the parent-owned ends after setup succeeds, ensuring TearDown() also owns and closes descriptors when pipe() or fork() assertions abort early.
🤖 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.
Outside diff comments:
In `@test/gtest/telemetry_mp_e2e_test.cpp`:
- Around line 135-137: Update the pipe setup in the test fixture so all six
descriptors returned by each successful pipe() are immediately wrapped in
scopedFd owners. In the fork/setup flow around the existing goWrite_,
readyRead_, and quitWrite_ assignments, retain or reset only the parent-owned
ends after setup succeeds, ensuring TearDown() also owns and closes descriptors
when pipe() or fork() assertions abort early.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 4f0508bc-c02e-4289-881d-49cf5bf75873
📒 Files selected for processing (1)
test/gtest/telemetry_mp_e2e_test.cpp
Contributor
Author
|
Addressed the outside-diff finding from the last CodeRabbit review ("Own all pipe descriptors on every setup path") in 4c649dc. It was valid. Only the three parent-retained ends were owned, and only after all three Each end is now wrapped in a
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/gtest/telemetry_mp_e2e_test.cpp (1)
211-217: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRetain
deaduntil it is reaped.
children_.erase(children_.begin())runs beforekill()andwaitpid(). If either fatal assertion fails, GoogleTest leaves the test body andTearDown()no longer containsdead, so the child can remain unreaped. Move the erase after a successfulwaitpid().Proposed cleanup ordering
const pid_t dead = children_.front(); -children_.erase(children_.begin()); const std::string dead_prefix = std::string(nixl::telemetry::mp::MP_STORE_FILE_PREFIX) + std::to_string(dead) + "."; ASSERT_EQ(countStores(dead_prefix), 1u); ASSERT_EQ(::kill(dead, SIGKILL), 0); ASSERT_EQ(::waitpid(dead, nullptr, 0), dead); +children_.erase(children_.begin());🤖 Prompt for 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. In `@test/gtest/telemetry_mp_e2e_test.cpp` around lines 211 - 217, In the child cleanup sequence using dead and children_, keep dead tracked until it has been successfully terminated and reaped. Move children_.erase(children_.begin()) to after the successful kill and waitpid assertions, preserving the existing store-count validation and cleanup order.
🤖 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.
Outside diff comments:
In `@test/gtest/telemetry_mp_e2e_test.cpp`:
- Around line 211-217: In the child cleanup sequence using dead and children_,
keep dead tracked until it has been successfully terminated and reaped. Move
children_.erase(children_.begin()) to after the successful kill and waitpid
assertions, preserving the existing store-count validation and cleanup order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 0a165dc5-681e-47a0-8bb7-1e3519192e57
📒 Files selected for processing (1)
test/gtest/telemetry_mp_e2e_test.cpp
Contributor
Author
|
Addressed the outside-diff finding ("Retain Erasing only after
|
Contributor
Author
Contributor
Author
|
/build |
Contributor
Author
|
/ok to test 5fac215 |
The gtest Prometheus, histogram and multi-process tests each carried their own HTTP client and hand-rolled exposition-format parsing, while the DOCA tests already had a tested loopback client, OpenMetrics parser and timeSeries query layer. Move those four headers to test/metrics/, rename their namespace to nixl::metrics_test, and convert the gtest telemetry tests to them, deleting the duplicated helpers. scrape_util.h grows a generic scrapeUntil() poll-until-predicate that scrapeUntilValue() and scrapeMetrics() are now expressed in terms of, and telemetry_core_scrape.h adds scrapeCoreOverflow() for driving nixlTelemetry with a small staging buffer and polling until accepted + dropped accounts for every produced event. Replacing fixed sleeps with polling also cuts suite runtime. Signed-off-by: Efraim Eygin <eeygin@nvidia.com>
Everything prometheus_mp had constructed the exporter directly and called exportEvent, which skips the core: the staging queue, the drain task, per-metric gating and the drop accounting were never exercised for this exporter. That gap blocks making it the configured exporter by default. Lift the MpExporterTest fixture into mp_telemetry_fixture.h so a second file can share it, and add telemetry_mp_core_test.cpp mirroring the single-process coverage: the allowlist keeping a deactivated metric out (asserted on the store as well as the scrape, which proves the event never reached the exporter rather than merely not being rendered), the dropped counter summing every delta, and conservation of accepted + dropped over a flooded 256-slot queue for both the updateData and the all-or-none addXferStats batch path. Signed-off-by: Efraim Eygin <eeygin@nvidia.com>
…ture Four suites (store, collector, exporter, e2e) each opened their own per-test temp directory and removed it again, two of them also repeating the mode-0700 chmod the exporter needs to stay quiet, and the exporter fixture extracted in the previous commit made that a fifth copy. Give them a common mpTempDirTest base and derive MpExporterTest (directory plus port and env) and MpE2ETest (plus child reaping) from it, so the 0700 rule and the naming live in one place. idx() had four identical copies and initParams() two; both move next to the fixture. Signed-off-by: Efraim Eygin <eeygin@nvidia.com>
The e2e fixture kept three pipe ends as raw ints with its own closeFd(), and loopbackConnection hand-rolled the same ownership in a constructor error path, a destructor and findFreePort(). nixl::scopedFd already does exactly this and is what the prometheus and collector tests use, so adopt it: loopbackConnection loses its destructor, and the fixture's fds close themselves. Also mark the remaining loopbackConnection queries [[nodiscard]] -- the rest of test/metrics already does -- and give the standalone doca_test target the utils include path scoped_fd.h needs. Signed-off-by: Efraim Eygin <eeygin@nvidia.com>
…kets exist seriesByAgent() keys on agent_name alone, so the bucket series of one agent -- which differ only by their le label -- collapsed onto a single entry. Counting those entries proved every agent exposes some bucket series and nothing about the distribution, so a sample in the wrong bucket passed. Query the cumulative buckets by le instead: the parent's single 1234us sample must leave le="1000" at zero and le="2500" and le="+Inf" at one, and a writer that observed nothing must read zero at +Inf. Signed-off-by: Efraim Eygin <eeygin@nvidia.com>
The transfer-byte assertions already name each child, but the histogram ones stopped at agent-0, so a wrong histogram for agent-1 or agent-2 passed. Loop over the writers and assert count, sum and the +Inf bucket are all zero. Signed-off-by: Efraim Eygin <eeygin@nvidia.com>
Only the three parent-retained ends were owned, and only once all three pipes had been created: a failing pipe() leaked the pipes opened before it, and a failing fork() leaked the three ends the parent hands to the children. Wrap each end in a scopedFd as soon as its pipe() succeeds, so the fixture no longer needs to hold any descriptor -- the locals close on every exit path, including a fatal assertion mid-fork -- and TearDown is left with just the child reaping. Signed-off-by: Efraim Eygin <eeygin@nvidia.com>
The child was dropped from children_ before it was killed, so a failing assertion between the two left the test body with nobody responsible for reaping it. Erase only after waitpid() succeeds: TearDown() finishes the job on an early exit, and stops tracking the pid once reaped so a recycled one is never signalled. Signed-off-by: Efraim Eygin <eeygin@nvidia.com>
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>
e-eygin
force-pushed
the
nix-1682-prometheus-mp-core-tests
branch
from
August 13, 2026 12:43
5fac215 to
85a2ce1
Compare
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>
Contributor
Author
|
/build |
Contributor
Author
|
/ok to test 7880fea |
ovidiusm
pushed a commit
that referenced
this pull request
Aug 18, 2026
…2092) ## What? `Tracing.RequestedBackendWithoutPluginIsInert` asked `makeTracer` for the `nvtx` backend and expected a null tracer. It now asks for a backend name no plugin can provide, matching what the sibling test `Tracing.MakeTracerUnknownBackendReturnsNull` already did; the name lives in one `kUnloadableBackend` constant used by both. ## Why? The old assertion rested on a property of the *environment*, not of the code: its own comment said "no `libtrace_backend_*.so` is registered in this unit binary". Whenever a real `libtrace_backend_nvtx.so` is discoverable, the plugin loads, `makeTracer` correctly returns a live tracer, and the test fails. That is the case in the dev container, where `NIXL_PLUGIN_DIR` points at an install tree containing the NVTX trace plugin, and it would be the case in any CI leg that installs NIXL before running the `unit` suite. The test has been permanently red locally for weeks and was written off as "pre-existing, unrelated" in the validation notes of three telemetry PRs (#1952, #2054, #2086). Beyond the recurring explanation, a genuine regression in the null-tracer / inert-`Span` path would have been indistinguishable from the known-red state. Coverage is unchanged: this test exists to prove that a null tracer leaves call sites on the safe default-constructed `Span` path, which never required NVTX specifically. Real NVTX behaviour stays covered by the e2e `TestTransferTracing` tests, which load the actual plugin. Tracking: NIX-1710. <details> <summary>Rejected alternative, and verification</summary> Clearing `NIXL_PLUGIN_DIR` inside the test via the existing `gtest::ScopedEnv` helper does not work reliably: `getPluginDir()` is read once in the `nixlPluginManager` constructor and cached in `plugin_dirs_` for the process lifetime, so the outcome would depend on whether an earlier test in the binary already touched the plugin manager. Verified both directions with the container's default `NIXL_PLUGIN_DIR`, i.e. with no workaround applied: - Baseline (this change stashed, rebuilt): exactly one failure, `RequestedBackendWithoutPluginIsInert`, reporting a live tracer pointer where `nullptr` was expected. - With the change: all 17 `Tracing.*` tests pass, and the full `unit` suite is 157 passed / 2 skipped / 3 failed — the three failures being `objCrtTestFixture.TransferBelowThreshold` and the two `ObjClientTests/objParamTestFixture.ReadTransfer` params, which need an object-storage endpoint this container does not provide. </details> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Improved tracing plugin-loading tests to consistently cover environments where the requested backend is unavailable. * Updated inert-tracer coverage to validate behavior when no tracing plugin can be loaded. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Efraim Eygin <eeygin@nvidia.com>
ovidiusm
approved these changes
Aug 18, 2026
This was referenced Sep 10, 2026
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
2 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.
What?
Covers the
prometheus_mptelemetry exporter's core path in gtest by driving it throughnixlTelemetryinstead of callingexportEventdirectly: the per-metric allowlist keeping adeactivated metric out, the dropped-events counter accumulating every flush delta, and
conservation of accepted + dropped events when a small staging queue overflows (both the
single-event path and the all-or-none 4-event
addXferStatsbatch).Tests only; no production code changes.
Why?
Everything this exporter had constructed it directly and called
exportEvent, which skips thecore: the staging queue, the drain task, metric gating and the drop accounting were never
exercised for it. NIX-1685 makes
prometheus_mpthe configured exporter everywhere, and anexporter should not become a default while its core path is untested.
The allowlist is the case that matters most. Per-metric activation gates producer-side, before
the staging queue, so it should apply identically to any exporter — but nothing proved a
deactivated metric stays out of an mp store, and NIX-1685 would make that a default-path
behaviour. Closes NIX-1682.
How?
These tests need an HTTP scrape and a way to query the parsed result. Three implementations of
that already existed: two hand-rolled ones in gtest (string matching over the raw body) and a
proper loopback client, OpenMetrics parser and series-query layer in the DOCA tests. Rather
than add a fourth, the DOCA one moved to
test/metrics/as the shared toolkit and the gtesttelemetry tests were converted onto it, which deleted more code than the new tests add.
Waiting on an expected end state rather than sleeping a fixed interval is what keeps the new
tests timing-independent, and it also cut suite runtime.
The mp fixtures got the same treatment: four suites each opened their own per-test temp
directory, two of them separately repeating the mode-0700 chmod the exporter needs in order
not to warn (a warning fails these tests), so that rule now lives in one place. Descriptors
are owned by
nixl::scopedFdrather than hand-rolled closers.The allowlist test asserts on the store snapshot as well as on the scrape: an untouched slot
proves the event never reached the exporter, rather than merely not being rendered.
Verified with
./build/test/gtest/gtest --gtest_filter='Mp*:*Prometheus*:*Telemetry*'(88/88,stable over
--gtest_repeat=5) and the DOCA telemetry targets, which exercise the movedscrape toolkit.
Summary by CodeRabbit