feat(health): add NVLink domain UUID to switch telemetry - #4476
Merged
Conversation
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (16)
🚧 Files skipped from review as they are similar to previous changes (16)
Summary by CodeRabbit
WalkthroughThe health service now accepts optional NVLink domain UUIDs for switch endpoints, tracks domain changes during discovery, restarts affected collectors, and exports the metadata through events, JSONL, Prometheus, and OTLP sinks. ChangesNVLink domain metadata
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DiscoveryIteration
participant CollectorState
participant stop_stale_switch_collectors
participant SwitchEndpointCollectors
DiscoveryIteration->>CollectorState: observe_switch_domain
CollectorState-->>DiscoveryIteration: report domain change
DiscoveryIteration->>stop_stale_switch_collectors: remove stale collectors
stop_stale_switch_collectors->>SwitchEndpointCollectors: await collector shutdown
DiscoveryIteration->>SwitchEndpointCollectors: respawn collectors with current metadata
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4476.docs.buildwithfern.com/infra-controller |
![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: 3
🤖 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 `@crates/health/src/discovery/cleanup.rs`:
- Around line 51-106: Add a bounded shutdown helper around Collector::stop so
each stale collector is given a deadline, then its task is aborted and joined if
it does not finish; update the final join_all in stop_stale_switch_collectors to
use this behavior. Ensure shutdown does not detach old collectors before
replacements are spawned, and preserve cleanup completion for every collector.
In `@crates/health/src/sink/events.rs`:
- Line 673: Update the switch accessor fixtures at the changed
`nvlink_domain_uuid` assignments to use a fixed non-nil UUID instead of
`nvlink_domain_id()`, so they verify propagation of a valid switch UUID.
Preserve nil UUID omission coverage in a separate test case.
In `@docs/architecture/health_aggregation.md`:
- Line 284: Update the OTLP resource-attribute documentation in the
`[sinks.otlp]` section to include machine.serial, driver.version,
component.type, switch.serial_number, switch.endpoint_role, and
switch.is_primary, matching the attributes emitted by the conversion logic. Keep
the existing attribute names and type annotations accurate, and ensure the list
is presented as complete rather than omitting emitted attributes.
🪄 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: CHILL
Plan: Enterprise
Run ID: 608e4956-418f-408e-8c87-e8a7f38e0c29
📒 Files selected for processing (16)
crates/health/example/config.example.tomlcrates/health/src/api_client.rscrates/health/src/collectors/nvue/gnmi/on_change_processor.rscrates/health/src/collectors/nvue/gnmi/sample_processor.rscrates/health/src/config.rscrates/health/src/discovery/cleanup.rscrates/health/src/discovery/context.rscrates/health/src/discovery/iteration.rscrates/health/src/discovery/spawn.rscrates/health/src/endpoint/model.rscrates/health/src/endpoint/sources.rscrates/health/src/otlp/convert.rscrates/health/src/sink/events.rscrates/health/src/sink/log_file.rscrates/health/src/sink/prometheus.rsdocs/architecture/health_aggregation.md
yoks
reviewed
Aug 3, 2026
yoks
reviewed
Aug 3, 2026
yoks
reviewed
Aug 3, 2026
yoks
approved these changes
Aug 3, 2026
jayzhudev
force-pushed
the
health/nvl-domain-uuid
branch
from
August 4, 2026 00:02
9a1b40b to
dac5a96
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/health/src/discovery/cleanup.rs (1)
309-397: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover UUID rotation and UUID removal transitions.
The tests cover
None -> Some(uuid)but notSome(uuid_a) -> Some(uuid_b)orSome(uuid) -> None.Add table-driven cases for both transitions. Assert that each transition removes the collector. This verifies that a rotated UUID replaces stale metadata and that an unavailable UUID removes the old telemetry label.
🤖 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/health/src/discovery/cleanup.rs` around lines 309 - 397, Extend the tests around stop_stale_switch_collectors to cover both Some(uuid_a) to Some(uuid_b) rotation and Some(uuid) to None removal, using table-driven cases. For each transition, initialize the endpoint with the first UUID, register its NvueRest collector, apply the second UUID, invoke stop_stale_switch_collectors, and assert that the collector for the endpoint key is removed.Source: Coding guidelines
🤖 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 `@crates/health/src/discovery/cleanup.rs`:
- Around line 309-397: Extend the tests around stop_stale_switch_collectors to
cover both Some(uuid_a) to Some(uuid_b) rotation and Some(uuid) to None removal,
using table-driven cases. For each transition, initialize the endpoint with the
first UUID, register its NvueRest collector, apply the second UUID, invoke
stop_stale_switch_collectors, and assert that the collector for the endpoint key
is removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3ae693fa-4786-410b-9918-ff5f17580164
📒 Files selected for processing (16)
crates/health/example/config.example.tomlcrates/health/src/api_client.rscrates/health/src/collectors/nvue/gnmi/on_change_processor.rscrates/health/src/collectors/nvue/gnmi/sample_processor.rscrates/health/src/config.rscrates/health/src/discovery/cleanup.rscrates/health/src/discovery/context.rscrates/health/src/discovery/iteration.rscrates/health/src/discovery/spawn.rscrates/health/src/endpoint/model.rscrates/health/src/endpoint/sources.rscrates/health/src/otlp/convert.rscrates/health/src/sink/events.rscrates/health/src/sink/log_file.rscrates/health/src/sink/prometheus.rsdocs/architecture/health_aggregation.md
🚧 Files skipped from review as they are similar to previous changes (15)
- docs/architecture/health_aggregation.md
- crates/health/src/endpoint/model.rs
- crates/health/src/collectors/nvue/gnmi/sample_processor.rs
- crates/health/src/collectors/nvue/gnmi/on_change_processor.rs
- crates/health/src/sink/log_file.rs
- crates/health/example/config.example.toml
- crates/health/src/discovery/iteration.rs
- crates/health/src/otlp/convert.rs
- crates/health/src/api_client.rs
- crates/health/src/sink/events.rs
- crates/health/src/discovery/spawn.rs
- crates/health/src/sink/prometheus.rs
- crates/health/src/endpoint/sources.rs
- crates/health/src/discovery/context.rs
- crates/health/src/config.rs
jayzhudev
force-pushed
the
health/nvl-domain-uuid
branch
from
August 4, 2026 00:22
dac5a96 to
3224623
Compare
Read switch domain UUIDs from Carbide API and propagate them to metrics, OTLP attributes, events, and structured logs. Restart collectors when a switch changes domains so cached metadata and registered metric labels are replaced before collection resumes. Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
Allow static switch endpoints to provide NVLink domain UUID metadata. Ignore invalid and nil values so telemetry never publishes unusable labels. Preserve first-source precedence when duplicate endpoint keys are discovered, preventing false domain changes and unnecessary collector restarts. Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
jayzhudev
force-pushed
the
health/nvl-domain-uuid
branch
from
August 4, 2026 02:49
3224623 to
076c5e1
Compare
10 tasks
yoks
approved these changes
Aug 4, 2026
spydaNVIDIA
approved these changes
Aug 4, 2026
jayzhudev
enabled auto-merge (squash)
August 4, 2026 16:35
polarweasel
approved these changes
Aug 4, 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
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.
Switch health metrics and logs need the NVLink domain UUID to correlate failures. This PR reads the UUID from NICo API switch inventory or static endpoint configuration and includes it in metric and log sink output.
Collectors restart when the UUID changes so telemetry does not retain stale domain metadata. Missing, invalid, and nil UUIDs are omitted.
Related issues
Supports #4397
Type of Change
Breaking Changes
Testing