feat(health): add endpoint TCP reachability telemetry - #4696
Open
jayzhudev wants to merge 1 commit into
Open
Conversation
Contributor
|
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 (13)
🚧 Files skipped from review as they are similar to previous changes (13)
Summary by CodeRabbit
WalkthroughThis change adds configurable TCP reachability telemetry. Discovery resolves eligible endpoint targets and reconciles periodic collectors. The collector emits metrics and optional logs. Reachability events bypass health-report processing. ChangesTCP reachability telemetry
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Discovery
participant Eligibility
participant ReachabilityCollector
participant Endpoint
participant Sink
Discovery->>Eligibility: evaluate endpoint and collector configuration
Discovery->>ReachabilityCollector: reconcile targets
ReachabilityCollector->>Endpoint: open TCP connection with timeout
Endpoint-->>ReachabilityCollector: success or failure
ReachabilityCollector->>Sink: emit metric and optional structured log
🚥 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/health/src/lib.rs (1)
192-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for
ReachabilityTracingFilter.The current test only covers
ReachabilityProcessorBypass. Add coverage that forwards reachabilityCollectorEvent::Logevents and suppresses reachability metric and lifecycle events. Also verify that non-reachability events still reach the wrapped sink. This protects thelog_modecontract independently of processor routing.As per path instructions, “Prefer findings about behavior, concurrency, resource lifetimes, and missing tests over style-only comments.”
🤖 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/lib.rs` around lines 192 - 216, Add focused tests for ReachabilityTracingFilter covering three cases: forward reachability CollectorEvent::Log events, suppress reachability metric and lifecycle events, and forward non-reachability events regardless of kind. Use a recording or mock DataSink to assert which events reach the wrapped sink, keeping coverage independent of ReachabilityProcessorBypass routing.Source: Path instructions
crates/health/src/config.rs (1)
4554-4573: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConvert the validation test to the
scenarios!table.This test invokes
validate()twice with different inputs. The neighbouring validation tests in this file (nmxc_collector_validation,logs_collector_validation,sse_log_config_validation) already express that shape withscenarios!andOutcome. Aligning keeps the failure output uniform and makes new cases cheap to add.♻️ Proposed table-driven form
#[test] - fn reachability_validation_rejects_non_positive_runtime_values() { - let mut config = ReachabilityCollectorConfig { - interval: Duration::ZERO, - ..ReachabilityCollectorConfig::default() - }; - - assert_eq!( - config.validate().expect_err("zero interval must fail"), - "[collectors.reachability].interval must be greater than 0" - ); - - config.interval = Duration::from_secs(1); - config.timeout = Duration::ZERO; - - assert_eq!( - config.validate().expect_err("zero timeout must fail"), - "[collectors.reachability].timeout must be greater than 0" - ); - } + fn reachability_collector_validation() { + scenarios!(run = |config: ReachabilityCollectorConfig| config.validate(); + "valid configuration" { + ReachabilityCollectorConfig::default() => Yields(()), + } + + "probe cadence" { + ReachabilityCollectorConfig { + interval: Duration::ZERO, + ..ReachabilityCollectorConfig::default() + } => FailsWith( + "[collectors.reachability].interval must be greater than 0".to_string() + ), + + ReachabilityCollectorConfig { + timeout: Duration::ZERO, + ..ReachabilityCollectorConfig::default() + } => FailsWith( + "[collectors.reachability].timeout must be greater than 0".to_string() + ), + } + ); + }🤖 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 4554 - 4573, Convert reachability_validation_rejects_non_positive_runtime_values into the scenarios! table-driven form, using the existing Outcome pattern from neighboring tests such as nmxc_collector_validation, logs_collector_validation, and sse_log_config_validation. Represent both zero-interval and zero-timeout configurations as scenarios with their expected validation errors, while preserving the current assertions and test coverage.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/config.rs`:
- Around line 4554-4573: Convert
reachability_validation_rejects_non_positive_runtime_values into the scenarios!
table-driven form, using the existing Outcome pattern from neighboring tests
such as nmxc_collector_validation, logs_collector_validation, and
sse_log_config_validation. Represent both zero-interval and zero-timeout
configurations as scenarios with their expected validation errors, while
preserving the current assertions and test coverage.
In `@crates/health/src/lib.rs`:
- Around line 192-216: Add focused tests for ReachabilityTracingFilter covering
three cases: forward reachability CollectorEvent::Log events, suppress
reachability metric and lifecycle events, and forward non-reachability events
regardless of kind. Use a recording or mock DataSink to assert which events
reach the wrapped sink, keeping coverage independent of
ReachabilityProcessorBypass routing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6f411b5e-ff23-4df7-8581-b265e4038801
📒 Files selected for processing (13)
crates/health/example/config.example.tomlcrates/health/src/collectors/mod.rscrates/health/src/collectors/nmxt.rscrates/health/src/collectors/reachability.rscrates/health/src/config.rscrates/health/src/discovery/context.rscrates/health/src/discovery/iteration.rscrates/health/src/discovery/mod.rscrates/health/src/discovery/reachability.rscrates/health/src/discovery/spawn.rscrates/health/src/endpoint/model.rscrates/health/src/lib.rscrates/health/src/sink/events.rs
Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
jayzhudev
force-pushed
the
health/connectivity-probing
branch
from
August 7, 2026 06:23
dabefa0 to
1ae8c68
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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 opt-in TCP reachability observations to
carbide-health. This allows metrics and logs consumers to distinguish transport failures from TLS, authentication, and protocol failures.The collector derives targets and ports from the existing eligibility rules and configuration for Redfish, NVUE REST, gNMI, NMX-T, and NMX-C. BMC probes connect directly to the discovered endpoint, even when a BMC proxy is configured.
Each completed probe emits an endpoint-correlated
tcp_port_reachable_stategauge. Structured logging is stateless:unreachablelogs every failed probe, whilealllogs every completed probe. Reachability observations bypass health processors and do not create health reports or persisted alerts.This feature is disabled by default. To enable, here's an example configuration:
Related issues
Resolves #4658
Type of Change
Breaking Changes
Testing
Manual Testing
Method:
healthcollection against real NVOS switch host endpoints.Coverage:
allandunreachablelog modes.Steps:
Probed each service port independently.
Observed results:
Enabled
log_mode = "all"and inspected/telemetryand JSONL output.Selected redacted metric:
Selected redacted log:
{"endpoint":"<endpoint-key>","collector":"reachability","component_type":"nvlink_switch","severity":"WARN","body":"TCP port is unreachable","attributes":[["message_id","CarbideHealth.1.0.TcpPortUnreachable"],["message_args","[\"nmxt@<endpoint-ip>:9352\"]"],["reachability.service","nmxt"],["reachability.address","<endpoint-ip>"],["reachability.port","9352"],["reachability.state","unreachable"],["reachability.error","Connection refused (os error 61)"]]}Repeated the fleet run with
log_mode = "unreachable".Observed results:
All 21 metric series remained available. The 84 records represent 14 failed targets across six cycles.
Configured two discovered endpoints with the same test key. The first enabled NVUE REST, while the later duplicate also enabled NMX-T.
Observed results:
Results:
Additional Notes
A successful probe means only that the TCP handshake completed. It does not validate TLS, authentication, HTTP, gRPC, gNMI, NMX-C subscription, or collector readiness.