test(health): own sensor metric projection#4086
Open
chet wants to merge 1 commit into
Open
Conversation
Contributor
WalkthroughThe sensor collector now derives sweep behavior from ChangesSensor collection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CollectorSweep
participant SensorSweepPlan
participant SensorCollector
participant project_sensor
participant MetricSink
CollectorSweep->>SensorSweepPlan: map sweep mode
SensorSweepPlan->>SensorCollector: provide fetch limit and metric flags
SensorCollector->>SensorCollector: fetch sensors within limit
SensorCollector->>project_sensor: validate and project sensor reading
project_sensor-->>SensorCollector: return primary and optional range metrics
SensorCollector->>MetricSink: emit projected metrics
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
🐇🔍 ✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/health/src/collectors/sensors.rs (1)
323-340: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify the option-flattening idiom and the misleading
readingbinding.Two readability refinements in this extraction chain:
s.health.and_then(std::convert::identity)is simplyOption::flatten; the explicit form obscures intent.- The
.filter(|(_, reading)| ...)closure binds the unit string asreading. It is correct today (only aStringexposesis_empty()), but the inverted name is a latent trap for future edits.As per coding guidelines: "Prefer simple, explicit, readable Rust over clever or unnecessarily abstract code."
♻️ Proposed clarity fix
let Some(bmc_health) = sensor .status .as_ref() - .and_then(|s| s.health.and_then(std::convert::identity)) + .and_then(|s| s.health.flatten()) else { return Err(SensorProjectionError::NoHealth); }; let Some((reading, reading_type, unit)) = sensor .reading .flatten() .zip(sensor.reading_type.flatten()) .zip(sensor.reading_units.clone().flatten()) - .filter(|(_, reading)| !reading.is_empty()) + .filter(|(_, unit)| !unit.is_empty()) .map(|((r, rt), u)| (r, rt, u)) else { return Err(SensorProjectionError::IncompleteReading); };🤖 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/sensors.rs` around lines 323 - 340, In the health extraction, replace the explicit identity-based flattening of s.health with Option::flatten. In the reading extraction chain, rename the filter closure’s second binding to reflect that it is the unit value, while preserving the existing non-empty check and tuple behavior.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/collectors/sensors.rs`:
- Around line 323-340: In the health extraction, replace the explicit
identity-based flattening of s.health with Option::flatten. In the reading
extraction chain, rename the filter closure’s second binding to reflect that it
is the unit value, while preserving the existing non-empty check and tuple
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f2dae110-ae31-400e-a36d-d0a2f07ea959
📒 Files selected for processing (2)
crates/health/src/collectors/sensors.rscrates/health/src/metrics.rs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/health/src/collectors/sensors.rs (1)
331-340: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider renaming the filter binding to reflect the value it inspects.
After the two
zips the tuple is((reading, reading_type), units), so the closure binding namedreadingis in fact the units string. The behaviour is correct — empty units are rejected asIncompleteReading— but the misleading name is a latent trap for anyone later tightening this completeness check.♻️ Proposed rename for clarity
.zip(sensor.reading_units.clone().flatten()) - .filter(|(_, reading)| !reading.is_empty()) + .filter(|(_, units)| !units.is_empty())🤖 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/sensors.rs` around lines 331 - 340, Rename the filter closure’s second tuple binding in the sensor projection flow to indicate it represents units, not the reading value; preserve the existing empty-units check and all surrounding tuple mapping behavior.
🤖 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/sensors.rs`:
- Around line 331-340: Rename the filter closure’s second tuple binding in the
sensor projection flow to indicate it represents units, not the reading value;
preserve the existing empty-units check and all surrounding tuple mapping
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 62ff68db-f7e9-4f29-b1a8-a1a1d2a101fa
📒 Files selected for processing (2)
crates/health/src/collectors/sensors.rscrates/health/src/metrics.rs
Replace the two range-only checks with table-driven cases for Redfish projection, unit normalization, sweep planning, collection failures, lifecycle events, and concurrency. Exercise the real collector path without repeating the drive and power-supply mapping covered by the entity projection tests. NVIDIA#4081 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
🐇🔍 ✅ Action performedFull review finished. |
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.
The sensor collector had two direct tests that checked only range suffix and
label strings. Redfish-to-metric projection, unit normalization, sweep
planning, and the collector boundary still had little or no owner coverage.
This extracts private projection and sweep-plan helpers, then replaces the
narrow checks with six table-driven entries and 37 rows:
The collection table exercises representative fetch, failure, derived-metric,
and stop paths. Drive and power-supply mapping stays with the entity projection
tests instead of being repeated here. The health suite moves from 405 Original
entries to 404 Converted entries and 409 Expanded entries.
Selected production coverage for
sensors.rsplussanitize_unit, excludingtest-module bodies:
Expanded adds ten
TestBmcmonomorphs when the collector table instantiatesits generic production methods. Those records add ten functions, 126
line-instances, and 172 regions to the denominator; all 33 common production
records retain identical non-count layouts, and test-body records are excluded.
Related issues
Type of Change
Breaking Changes
Testing
Unit tests added/updated
Integration tests added/updated
Manual testing performed
No testing required (docs, internal refactor, etc.)
cargo test -p carbide-health --lib— 409 passedcargo make format-nightlycargo make clippycargo make carbide-lintsLocal CodeRabbit review — no findings
Local Claude review — no correctness findings
Additional Notes
The collector keeps its existing event placement, failure accounting, and
unordered sensor fetch scheduling.