feat(nvtx): app-integrated in-process NVTX capture - #402
Conversation
9prady9
changed the title
📝 WalkthroughWalkthroughAdds NVTX bridge and example crates to the workspace, exposes observer senders, ensures static injection linkage, and documents and tests in-process NVTX capture. ChangesNVTX capture integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 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.
🧹 Nitpick comments (1)
integrations/nvtx/instrumentation/build.rs (1)
41-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve
cc’s configured compiler args here.
compiler.path()drops the flags and wrappersccalready inferred fromCXX/CXXFLAGSand cross-target setup. Usecompiler.to_command()so the sample build keeps those settings and still appends the local flags.🤖 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 `@integrations/nvtx/instrumentation/build.rs` around lines 41 - 53, Update the sample compilation command in the build script to start from cc’s configured command via compiler.to_command() instead of creating a new Command from compiler.path(). Preserve the existing local arguments for C++17, optimization, pthreads, includes, source, output, and dl, while retaining all compiler flags, wrappers, and cross-target settings inferred by cc.
🤖 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.
Nitpick comments:
In `@integrations/nvtx/instrumentation/build.rs`:
- Around line 41-53: Update the sample compilation command in the build script
to start from cc’s configured command via compiler.to_command() instead of
creating a new Command from compiler.path(). Preserve the existing local
arguments for C++17, optimization, pthreads, includes, source, output, and dl,
while retaining all compiler flags, wrappers, and cross-target settings inferred
by cc.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 72b20491-801a-45b9-a721-48d83f179b89
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (7)
Cargo.tomlintegrations/nvtx/README.mdintegrations/nvtx/instrumentation/Cargo.tomlintegrations/nvtx/instrumentation/build.rsintegrations/nvtx/instrumentation/c/sample_app.cppintegrations/nvtx/instrumentation/src/lib.rsintegrations/nvtx/instrumentation/tests/capture_e2e.rs
|
Had a quick call with @johanpel about the comment threads #402 (comment) and #402 (comment) I had different use case that I was trying to cover as well - app and/or library without any code changes which complicated the bridge design. I am addressing all of the above that simplifies the use case - quent instrumentation with additional nvtx range annotations. |
9prady9
changed the title
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (3)
integrations/nvtx/README.md-66-71 (1)
66-71: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEnd the range before flushing the observer.
_rangeis dropped afterdrop(observer), so itsRangeEndis discarded. Explicitly drop the guard first.Proposed fix
-nvtx::mark!("startup"); -let _range = nvtx::range!("phase-1"); +nvtx::mark!("startup"); +let range = nvtx::range!("phase-1"); +drop(range); // 4. Flush by dropping the observer. drop(observer);🤖 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 `@integrations/nvtx/README.md` around lines 66 - 71, Update the NVTX example so the range guard created by nvtx::range!("phase-1") is explicitly dropped before drop(observer), ensuring the range ends before the observer is flushed.integrations/nvtx/README.md-39-42 (1)
39-42: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winName the actual overridden symbol.
integrations/nvtx/injection/build.rsLine 89 identifies the strong symbol asInitializeInjectionNvtx2_fnptr, notInitializeInjectionNvtx2. Keep the mechanism documentation consistent.- feature, publishing a *strong* `InitializeInjectionNvtx2` that overrides the + feature, publishing a *strong* `InitializeInjectionNvtx2_fnptr` that overrides the🤖 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 `@integrations/nvtx/README.md` around lines 39 - 42, Update the mechanism description in the README to name the actual strong overridden symbol, InitializeInjectionNvtx2_fnptr, instead of InitializeInjectionNvtx2; keep the surrounding explanation of static injection and in-process initialization unchanged.integrations/nvtx/example/src/main.rs-31-36 (1)
31-36: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject invalid explicit session IDs.
A supplied non-UTF-8 or invalid UUID silently becomes a random session, so a harness cannot locate its output. Return an argument error instead.
Proposed fix
- let session = std::env::args_os() - .nth(2) - .and_then(|arg| arg.to_str().and_then(|s| Uuid::parse_str(s).ok())) - .unwrap_or_else(Uuid::now_v7); + let session = match std::env::args_os().nth(2) { + Some(arg) => Uuid::parse_str(arg.to_str().ok_or_else(|| { + std::io::Error::new( + std::io::ErrorKind::InvalidInput, + "session must be a UTF-8 UUID", + ) + })?)?, + None => Uuid::now_v7(), + };🤖 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 `@integrations/nvtx/example/src/main.rs` around lines 31 - 36, Update the session parsing flow in main around std::env::args_os and Uuid::parse_str so an explicitly supplied second argument must be valid UTF-8 and a valid UUID; return an argument error for non-UTF-8 or invalid values instead of falling back to Uuid::now_v7. Retain fresh UUID generation only when no session argument is provided.
🤖 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.
Other comments:
In `@integrations/nvtx/example/src/main.rs`:
- Around line 31-36: Update the session parsing flow in main around
std::env::args_os and Uuid::parse_str so an explicitly supplied second argument
must be valid UTF-8 and a valid UUID; return an argument error for non-UTF-8 or
invalid values instead of falling back to Uuid::now_v7. Retain fresh UUID
generation only when no session argument is provided.
In `@integrations/nvtx/README.md`:
- Around line 66-71: Update the NVTX example so the range guard created by
nvtx::range!("phase-1") is explicitly dropped before drop(observer), ensuring
the range ends before the observer is flushed.
- Around line 39-42: Update the mechanism description in the README to name the
actual strong overridden symbol, InitializeInjectionNvtx2_fnptr, instead of
InitializeInjectionNvtx2; keep the surrounding explanation of static injection
and in-process initialization unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 973bdf18-5fc2-4077-aae3-e8eb9f60af9b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (9)
Cargo.tomlcrates/instrumentation/src/observer.rsintegrations/nvtx/README.mdintegrations/nvtx/bridge/Cargo.tomlintegrations/nvtx/bridge/src/lib.rsintegrations/nvtx/example/Cargo.tomlintegrations/nvtx/example/src/main.rsintegrations/nvtx/example/tests/capture.rsintegrations/nvtx/injection/build.rs
Signed-off-by: Pradeep Garigipati <pgarigipati@nvidia.com>
- example: split into a lib + bin. The demo prints captured events through a callback exporter; the test reuses the same `run_capture` routine in-process with a collecting callback exporter and asserts coverage — no subprocess and no temp files. - nvtx-injection: document the `static-injection` feature (manifest comment + crate "Attach modes" docs) rather than explaining it in the example manifest. - instrumentation: `Observer::sender()` docs no longer call post-drop sends "silent" — the first is logged via `tracing`. Signed-off-by: Pradeep Garigipati <pgarigipati@nvidia.com>
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (2)
integrations/nvtx/README.md-111-113 (1)
111-113: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace “no-oping” with standard wording.
Use “silently dropping the call” or “silently doing nothing” instead of “silently no-oping” at Line 113.
🤖 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 `@integrations/nvtx/README.md` around lines 111 - 113, Update the wide-char variants description in the README to replace “silently no-oping” with the standard wording “silently dropping the call” or “silently doing nothing,” without changing the surrounding behavior description.Source: Linters/SAST tools
integrations/nvtx/injection/Cargo.toml-27-32 (1)
27-32: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the pointer symbol name consistently in the static-injection docs.
integrations/nvtx/injection/Cargo.tomlandintegrations/nvtx/injection/src/lib.rsmixInitializeInjectionNvtx2with the strong symbol actually linked bystatic-injection,InitializeInjectionNvtx2_fnptr. ReserveInitializeInjectionNvtx2for the exported entry point and useInitializeInjectionNvtx2_fnptrfor the linked symbol.🤖 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 `@integrations/nvtx/injection/Cargo.toml` around lines 27 - 32, The static-injection documentation in integrations/nvtx/injection/Cargo.toml (lines 27-32) and integrations/nvtx/injection/src/lib.rs (lines 17-20) uses the wrong symbol name for the linked pointer. Update both sites to refer to InitializeInjectionNvtx2_fnptr, reserving InitializeInjectionNvtx2 for the exported entry point.
🧹 Nitpick comments (1)
crates/instrumentation/src/observer.rs (1)
121-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep
Observer::senderdocumentation contract-focused.Lines 121-126 lead with the concrete
EventSenderclone/pipeline mechanism and restate the type. Retain the ownership, flush, and post-drop behavior, but make that observable contract the opening. As per path instructions: “Docstrings state the contract, not the mechanism; do not restate types.”🤖 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 `@crates/instrumentation/src/observer.rs` around lines 121 - 126, Update the documentation for Observer::sender to lead with its observable ownership, flush-on-drop, and post-drop send behavior. Remove the implementation-focused explanation about cloning, the pipeline, and restating EventSender, while preserving the documented behavior that sends after observer drop are discarded and only the first logs an error.Source: Path instructions
🤖 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.
Other comments:
In `@integrations/nvtx/injection/Cargo.toml`:
- Around line 27-32: The static-injection documentation in
integrations/nvtx/injection/Cargo.toml (lines 27-32) and
integrations/nvtx/injection/src/lib.rs (lines 17-20) uses the wrong symbol name
for the linked pointer. Update both sites to refer to
InitializeInjectionNvtx2_fnptr, reserving InitializeInjectionNvtx2 for the
exported entry point.
In `@integrations/nvtx/README.md`:
- Around line 111-113: Update the wide-char variants description in the README
to replace “silently no-oping” with the standard wording “silently dropping the
call” or “silently doing nothing,” without changing the surrounding behavior
description.
---
Nitpick comments:
In `@crates/instrumentation/src/observer.rs`:
- Around line 121-126: Update the documentation for Observer::sender to lead
with its observable ownership, flush-on-drop, and post-drop send behavior.
Remove the implementation-focused explanation about cloning, the pipeline, and
restating EventSender, while preserving the documented behavior that sends after
observer drop are discarded and only the first logs an error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 989fdc6b-b08a-4bad-a13c-7c586518fd78
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (12)
Cargo.tomlcrates/instrumentation/src/observer.rsintegrations/nvtx/README.mdintegrations/nvtx/bridge/Cargo.tomlintegrations/nvtx/bridge/src/lib.rsintegrations/nvtx/example/Cargo.tomlintegrations/nvtx/example/src/lib.rsintegrations/nvtx/example/src/main.rsintegrations/nvtx/example/tests/capture.rsintegrations/nvtx/injection/Cargo.tomlintegrations/nvtx/injection/build.rsintegrations/nvtx/injection/src/lib.rs
johanpel
left a comment
johanpel
left a comment
There was a problem hiding this comment.
Nice, happy to see this integration come alive!
a few more things here and there I think. |
|
/merge |
Full Phase-1 (NVTX capture foundation) planning and execution record: discuss/research/plan artifacts, per-plan SUMMARYs (events vocabulary, injection cdylib, bridge + capture e2e, full core coverage), tracking updates, code review and security verification, and reconciliation with the upstream merge (PR rapidsai#402). Signed-off-by: Pradeep Garigipati <pgarigipati@nvidia.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
Wires captured NVTX events into a Quent pipeline, completing the foundational NVTX stack (#386
quent-nvtx-events, #391quent-nvtx-injection).What it is
The application drives capture: it owns its Quent
Contextand exporter, annotates its code with the NVTX Rust API, and linksquent-nvtx-injectionwith itsstatic-injectionfeature so NVTX initializes injection in-process at the first NVTX call — no cdylib, noNVTX_INJECTION64_PATH.quent-nvtx-bridge—NvtxEventEntity, a#[serde(transparent)]newtype overNvtxEventimplementing Quent'sEntityEvent(the orphan-rule adapter; the only crate depending on Quent internals).quent-instrumentation— addsObserver::sender(), a clonedEventSenderso the'staticinjection hook can emit into an app-owned observer that still flushes on drop.quent-nvtx-injection—static-injectionlinks the strong-symbol shim with+whole-archive, so the strongInitializeInjectionNvtx2overrides NVTX's weak no-op.quent-nvtx-example— runnable wiring plus its test.Using it
Tests
cargo test -p quent-nvtx-exampleruns the example against a temp dir and asserts every core NVTX kind round-trips through ndjson. No GPU. Linux 64-bit only.