feat: health nvidia oem metrics - #4730
Merged
Merged
Conversation
Signed-off-by: ianisimov <ianisimov@nvidia.com>
Signed-off-by: ianisimov <ianisimov@nvidia.com>
Signed-off-by: ianisimov <ianisimov@nvidia.com>
Signed-off-by: ianisimov <ianisimov@nvidia.com>
Contributor
Summary by CodeRabbit
WalkthroughThe mock Redfish service now supports memory resources, HBM metrics, NVIDIA OEM processor metrics, and telemetry reports. The health crate adds a configurable telemetry collector with metric-definition lookup, stale-report filtering, numeric sample publishing, and discovery integration. ChangesRedfish mock resources
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Discovery
participant TelemetryCollector
participant RedfishTelemetryService
participant DataSink
Discovery->>TelemetryCollector: start with fetch interval and data sink
TelemetryCollector->>RedfishTelemetryService: discover telemetry service
TelemetryCollector->>RedfishTelemetryService: fetch metric definitions
TelemetryCollector->>RedfishTelemetryService: fetch metric reports
TelemetryCollector->>DataSink: publish valid numeric samples
Possibly related PRs
Suggested labels: 🚥 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: 2
🧹 Nitpick comments (1)
crates/health/src/collectors/entity_metrics.rs (1)
643-708: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider aligning the
bank_count!parameter order with the resolution order.The macro body reads
$correctedfirst, but every call site passes the misspelled identifier first. The two identifiers differ by one character. An inverted call would silently prefer the legacy spelling and would not fail any current test, because the fixtures never send both spellings for the same counter.Naming the parameters in resolution order removes that failure mode at no runtime cost.
♻️ Proposed parameter reordering
macro_rules! bank_count { - ($misspelled:ident, $corrected:ident, $mt:literal) => { + ($corrected:ident, $legacy:ident, $mt:literal) => { - if let Some(value) = rr.$corrected.flatten().or(rr.$misspelled.flatten()) { + if let Some(value) = rr.$corrected.flatten().or(rr.$legacy.flatten()) { out.push(MetricField { metric_type: Cow::Borrowed($mt), unit: "count", value: value as f64, }); } }; } bank_count!( - max_availablity_bank_count, max_availability_bank_count, + max_availablity_bank_count, "nvidia_max_availability_banks" );Apply the same swap to the remaining four
bank_count!invocations.🤖 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/entity_metrics.rs` around lines 643 - 708, Reorder the `bank_count!` macro parameters to match its resolution order, placing the corrected identifier before the misspelled legacy identifier, and swap the arguments in all five invocations within `nvidia_memory_metric_fields` so corrected fields remain preferred.
🤖 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/telemetry.rs`:
- Around line 145-147: Update load_units and the initialization flow in the
telemetry collector so units is cached only when metric_definitions returns
Ok(None) or successful definitions; leave self.units as None when the request
errors, allowing the next iteration to retry. Add a test covering a failed first
definitions request followed by a successful second request and verifying units
are refreshed.
- Around line 266-268: Update the metric-reading logic in the telemetry
collector to reject parsed f64 values that are not finite, including NaN and
infinities, before emitting samples. Add a regression test covering a non-finite
MetricValue and verify it is not sent to metric sinks.
---
Nitpick comments:
In `@crates/health/src/collectors/entity_metrics.rs`:
- Around line 643-708: Reorder the `bank_count!` macro parameters to match its
resolution order, placing the corrected identifier before the misspelled legacy
identifier, and swap the arguments in all five invocations within
`nvidia_memory_metric_fields` so corrected fields remain preferred.
🪄 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: CHILL
Plan: Enterprise
Run ID: 01155e51-bb3d-4641-b75b-a326ba2e8029
📒 Files selected for processing (31)
crates/bmc-mock/src/hw/bluefield3.rscrates/bmc-mock/src/hw/bluefield4.rscrates/bmc-mock/src/hw/dell_poweredge_r750.rscrates/bmc-mock/src/hw/dell_poweredge_r760_bf4.rscrates/bmc-mock/src/hw/dgx_gb300_nvl.rscrates/bmc-mock/src/hw/dgx_vr_nvl.rscrates/bmc-mock/src/hw/generic_ami.rscrates/bmc-mock/src/hw/hpe_proliant_dl380a_gen11.rscrates/bmc-mock/src/hw/lenovo_gb300_nvl.rscrates/bmc-mock/src/hw/liteon_power_shelf.rscrates/bmc-mock/src/hw/nvidia_dgx_h100.rscrates/bmc-mock/src/hw/nvidia_gb200.rscrates/bmc-mock/src/hw/nvidia_switch_n5700_ld.rscrates/bmc-mock/src/hw/nvidia_switch_nd5200_ld.rscrates/bmc-mock/src/hw/supermicro_gb300_nvl.rscrates/bmc-mock/src/hw/wiwynn_gb200_nvl.rscrates/bmc-mock/src/redfish/computer_system.rscrates/bmc-mock/src/redfish/memory.rscrates/bmc-mock/src/redfish/mod.rscrates/bmc-mock/src/redfish/processor.rscrates/bmc-mock/src/redfish/telemetry_service.rscrates/health/Cargo.tomlcrates/health/example/config.bmc-mock.tomlcrates/health/example/config.example.tomlcrates/health/src/collectors/entity_metrics.rscrates/health/src/collectors/mod.rscrates/health/src/collectors/projection_test_support.rscrates/health/src/collectors/telemetry.rscrates/health/src/config.rscrates/health/src/discovery/context.rscrates/health/src/discovery/spawn.rs
Comment on lines
+145
to
+147
| if self.units.is_none() { | ||
| self.units = Some(self.load_units(&telemetry_service).await); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Retry metric-definition loading after a fetch failure.
Line 146 caches an empty map after metric_definitions() returns Err. The collector then emits unknown units until restart, even when the BMC recovers on the next iteration.
Cache an empty map only for Ok(None). Keep units as None after an error so the next iteration retries the request. Add a test where the first definitions request fails and the second succeeds.
Also applies to: 212-234
🤖 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/telemetry.rs` around lines 145 - 147, Update
load_units and the initialization flow in the telemetry collector so units is
cached only when metric_definitions returns Ok(None) or successful definitions;
leave self.units as None when the request errors, allowing the next iteration to
retry. Add a test covering a failed first definitions request followed by a
successful second request and verifying units are refreshed.
Comment on lines
+266
to
+268
| let Ok(reading) = reading.trim().parse::<f64>() else { | ||
| continue; | ||
| }; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject non-finite metric values.
parse::<f64>() accepts values such as NaN and infinity. These values can reach metric sinks as invalid measurements.
Check reading.is_finite() before emitting the sample. Add a regression case for a non-finite MetricValue.
Proposed fix
let Ok(reading) = reading.trim().parse::<f64>() else {
continue;
};
+ if !reading.is_finite() {
+ continue;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let Ok(reading) = reading.trim().parse::<f64>() else { | |
| continue; | |
| }; | |
| let Ok(reading) = reading.trim().parse::<f64>() else { | |
| continue; | |
| }; | |
| if !reading.is_finite() { | |
| continue; | |
| } |
🤖 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/telemetry.rs` around lines 266 - 268, Update the
metric-reading logic in the telemetry collector to reject parsed f64 values that
are not finite, including NaN and infinities, before emitting samples. Add a
regression test covering a non-finite MetricValue and verify it is not sent to
metric sinks.
10 tasks
poroh
approved these changes
Aug 7, 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.
With nv-redfish support of OEM Nvidia CSDL, this PR adds support of this new OEM fields into metrics emite by hw-health.
Also expands bmc-mock to support some of the OEM fields.
Related issues
#4731
Type of Change
Breaking Changes
Testing
Additional Notes