feat(health): add NMX-C streaming collector#3121
Open
jayzhudev wants to merge 1 commit into
Open
Conversation
Contributor
Summary by CodeRabbit
WalkthroughAdds NMX-C streaming collection for eligible switch-host endpoints, including runtime contract updates, collector implementation, config and discovery wiring, endpoint metadata propagation, cleanup handling, and documentation. ChangesNMX-C streaming collector feature
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Discovery as Discovery iteration
participant Spawn as Collector spawn
participant Collector as NmxcCollector
participant Gateway as NMX-C gRPC gateway
participant Sink as Log sink
Discovery->>Spawn: eligible endpoint set
Spawn->>Spawn: switch_supports_nmxc_subscription()
Spawn->>Collector: start_streaming()
Collector->>Gateway: Hello
Gateway-->>Collector: server header
Collector->>Gateway: Subscribe
Gateway-->>Collector: initial acknowledgement
alt Rejected acknowledgement
Collector->>Sink: emit rejection log
Collector-->>Spawn: NmxcStatus error
else Accepted acknowledgement
Collector-->>Spawn: connected stream
loop notifications
Gateway-->>Collector: notification
Collector->>Sink: emit structured log
end
end
Discovery->>Discovery: stop ineligible collectors
Related issues: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3121.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: 2
🧹 Nitpick comments (1)
crates/health/src/config.rs (1)
1856-1891: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
value_scenarios!for this total-operation test.
test_sinks_config_includes_log_eventshand-rolls a case array and loop, while the siblingtest_nmxc_transport_config_validationadded in this same PR correctly usesvalue_scenarios!for an equivalent total-operation table. Sinceincludes_log_events()is a total (non-fallible) operation,value_scenarios!would give per-case failure labeling for free and keep style consistent within the diff.As per coding guidelines, "Prefer table-driven tests using the
carbide-test-supportcrate withscenarios!for fallible operations andvalue_scenarios!for total operations."♻️ Proposed refactor using value_scenarios!
- fn test_sinks_config_includes_log_events() { - let cases = [ - ("default", SinksConfig::default(), false), - ( - "tracing", - SinksConfig { - tracing: Configurable::Enabled(TracingSinkConfig::default()), - ..SinksConfig::default() - }, - true, - ), - ... - ]; - - for (name, sinks, expected) in cases { - assert_eq!(sinks.includes_log_events(), expected, "{name}"); - } + fn test_sinks_config_includes_log_events() { + value_scenarios!( + run = |sinks: SinksConfig| sinks.includes_log_events(); + + "sinks includes_log_events" { + SinksConfig::default() => false, + SinksConfig { tracing: Configurable::Enabled(TracingSinkConfig::default()), ..SinksConfig::default() } => true, + SinksConfig { log_file: Configurable::Enabled(LogFileSinkConfig::default()), ..SinksConfig::default() } => true, + SinksConfig { otlp: Configurable::Enabled(OtlpSinkConfig::default()), ..SinksConfig::default() } => true, + } + ); }🤖 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/config.rs` around lines 1856 - 1891, Refactor test_sinks_config_includes_log_events to use value_scenarios! instead of the manual cases array and for-loop, since includes_log_events() is a total operation. Keep the same scenario coverage for default, tracing, log-file, and otlp by expressing them as value scenarios, and preserve the per-case assertion labeling style used by test_nmxc_transport_config_validation for consistency.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.
Inline comments:
In `@crates/health/src/collectors/nmxc.rs`:
- Around line 227-241: Add client-side HTTP/2 keepalive to the nmxc_client
Endpoint configuration so dead NMX-C peers are detected even after the initial
connect. Update the Endpoint builder in nmxc_client to set keepalive settings
before connect(), using the existing connect_timeout flow and preserving the
current error mapping via nmxc_transport_error. This should ensure long-lived
streams fail as transport errors instead of leaving stream.next() pending
indefinitely.
In `@docs/operations/monitoring-health.md`:
- Around line 146-148: The NMX-C collection description uses imprecise transport
wording; update the monitoring-health docs text to match the gRPC transport used
by the collector. In the relevant prose section, replace “plaintext HTTP only”
with wording that clearly says it uses plaintext gRPC over HTTP/2, and keep the
note that TLS, certificate bypass, custom certificate loading, and mTLS are out
of scope and not represented by NICo API SPIFFE certificate fields.
---
Nitpick comments:
In `@crates/health/src/config.rs`:
- Around line 1856-1891: Refactor test_sinks_config_includes_log_events to use
value_scenarios! instead of the manual cases array and for-loop, since
includes_log_events() is a total operation. Keep the same scenario coverage for
default, tracing, log-file, and otlp by expressing them as value scenarios, and
preserve the per-case assertion labeling style used by
test_nmxc_transport_config_validation for consistency.
🪄 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: 2e4dfb14-4224-4918-8be0-3d365b830e5d
📒 Files selected for processing (20)
crates/health/example/config.example.tomlcrates/health/src/api_client.rscrates/health/src/collectors/logs/sse.rscrates/health/src/collectors/mod.rscrates/health/src/collectors/nmxc.rscrates/health/src/collectors/nvue/gnmi/on_change_processor.rscrates/health/src/collectors/nvue/gnmi/sample_processor.rscrates/health/src/collectors/runtime.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/lib.rscrates/health/src/otlp/convert.rscrates/health/src/sink/events.rscrates/health/src/sink/prometheus.rsdocs/operations/monitoring-health.md
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
jayzhudev
force-pushed
the
health/nmx-c-streaming
branch
from
8217076 to
44aecdc
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/health/src/collectors/runtime.rs (1)
473-505: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate failure-handling logic between
Err(e)andFailed{...}arms.Both arms increment
metrics.reconnections_total, emit the sametracing::error!message, and callon_connect_result(Err(&error))with identical control flow — only the event-forwarding step differs. Consider extracting the common tail (metric bump, log, callback+early-return) into a small helper so the two failure paths can't drift (e.g. one arm's log message changing without the other).♻️ Suggested consolidation
- match stream_result { - Err(e) => { - metrics.reconnections_total.inc(); - tracing::error!( - error = ?e, - collector_type, - endpoint = ?endpoint.addr, - "streaming collector connection failed" - ); - if !on_connect_result(Err(&e)) { - return; - } - } - Ok(StreamingConnectResult::Failed { events, error }) => { - metrics.reconnections_total.inc(); - - for event in events { - metrics.items_processed_total.inc(); - data_sink.handle_event(&event_context, &event); - } - - tracing::error!( - error = ?error, - collector_type, - endpoint = ?endpoint.addr, - "streaming collector connection failed" - ); - - if !on_connect_result(Err(&error)) { - return; - } - } + match stream_result { + Err(error) | Ok(StreamingConnectResult::Failed { error, events: _ }) => { + // NOTE: illustrative only — a real fix needs a helper fn since match + // arms with differing bindings can't be merged this simply; extract + // a `handle_connect_failure(&metrics, &data_sink, &event_context, ...)` helper instead. + } Ok(StreamingConnectResult::Connected(mut stream)) => {🤖 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/collectors/runtime.rs` around lines 473 - 505, The `Err(e)` and `StreamingConnectResult::Failed { .. }` branches in `runtime.rs` duplicate the same reconnection metric, error logging, and `on_connect_result` early-return flow. Extract that shared failure tail into a small helper near the streaming connection handling logic, and have both `match` arms call it after their branch-specific work (including the `events` forwarding in `Failed`). Keep the helper aligned with the existing `metrics.reconnections_total`, `tracing::error!`, and `on_connect_result` behavior so future changes only need to happen in one place.
🤖 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/collectors/runtime.rs`:
- Around line 473-505: The `Err(e)` and `StreamingConnectResult::Failed { .. }`
branches in `runtime.rs` duplicate the same reconnection metric, error logging,
and `on_connect_result` early-return flow. Extract that shared failure tail into
a small helper near the streaming connection handling logic, and have both
`match` arms call it after their branch-specific work (including the `events`
forwarding in `Failed`). Keep the helper aligned with the existing
`metrics.reconnections_total`, `tracing::error!`, and `on_connect_result`
behavior so future changes only need to happen in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ca6a65ba-1023-49f2-ae12-daa4c3dbffc8
📒 Files selected for processing (20)
crates/health/example/config.example.tomlcrates/health/src/api_client.rscrates/health/src/collectors/logs/sse.rscrates/health/src/collectors/mod.rscrates/health/src/collectors/nmxc.rscrates/health/src/collectors/nvue/gnmi/on_change_processor.rscrates/health/src/collectors/nvue/gnmi/sample_processor.rscrates/health/src/collectors/runtime.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/lib.rscrates/health/src/otlp/convert.rscrates/health/src/sink/events.rscrates/health/src/sink/prometheus.rsdocs/operations/monitoring-health.md
✅ Files skipped from review due to trivial changes (4)
- crates/health/src/sink/events.rs
- crates/health/src/collectors/nvue/gnmi/sample_processor.rs
- crates/health/src/sink/prometheus.rs
- crates/health/src/otlp/convert.rs
🚧 Files skipped from review as they are similar to previous changes (15)
- crates/health/src/lib.rs
- crates/health/src/collectors/logs/sse.rs
- docs/operations/monitoring-health.md
- crates/health/src/collectors/nvue/gnmi/on_change_processor.rs
- crates/health/src/api_client.rs
- crates/health/src/endpoint/sources.rs
- crates/health/src/discovery/cleanup.rs
- crates/health/src/endpoint/model.rs
- crates/health/example/config.example.toml
- crates/health/src/collectors/mod.rs
- crates/health/src/collectors/nmxc.rs
- crates/health/src/discovery/iteration.rs
- crates/health/src/config.rs
- crates/health/src/discovery/context.rs
- crates/health/src/discovery/spawn.rs
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
1 participant
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.
Add NMX-C
Subscribestreaming support to carbide-health.This collector connects to eligible primary switch-host NMX-C gRPC endpoints, validates the Subscribe acknowledgement, and emits NMX-C notifications as structured log events through tracing, log-file, and OTLP sinks.
Note: Security and mTLS support are required for all switch host collectors, but are outside of the scope of this PR. They are tracked by #3122 and will be addressed in separate PRs.
Related issues
Closes #3120
Type of Change
Breaking Changes
Testing
Additional Notes
Manual test logs from updating a primary switch's topology and restarting the cluster: