fix(hw-health): don't let BMC Ok suppress critical/fatal sensor alerts (#3402)#3440
fix(hw-health): don't let BMC Ok suppress critical/fatal sensor alerts (#3402)#3440hatamzad-nv wants to merge 1 commit into
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughThe health report processor now suppresses only warning-level threshold alerts when the BMC reports ChangesHealth classification
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 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}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/health/src/processor/health_report.rs (2)
143-175: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSuppression scoping is correct.
The narrowed condition correctly restricts silencing to
SensorHealth::Warningonly whenBmcHealth::Ok, leavingCritical/Fatal/SensorFailureunaffected — matches the stated intent, and the inline rationale comment is a nice touch for future maintainers.One optional thought: this suppression is a decision worth trending over time (e.g., to detect systematically mis-tuned caution thresholds). Per the repo's instrumentation guideline, an occurrence "you'd trend" is the signal to declare a
carbide_instrument::Event(log = warn, metric = counter) rather than a plaintracing::warn!, so a future pass could promote this into acarbide_..._totalcounter withsensor_id/entity_typekept in#[context](not#[label], given their cardinality).Based on coding guidelines: "Does the event deserve a count, rate, or duration... Declare it once as a
carbide_instrument::Eventandemit()."🤖 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/processor/health_report.rs` around lines 143 - 175, Replace the plain tracing::warn! in the SensorHealth::Warning suppression branch with a carbide_instrument::Event declared once and emitted for each suppression. Configure it as a warning counter, keeping sensor_id and entity_type as context fields rather than labels, while preserving the existing diagnostic fields and Success return behavior.Source: Coding guidelines
344-405: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing
SensorFailure+BmcHealth::Okcoverage, and these four tests are a textbook table-driven candidate.The PR description explicitly promises
SensorFailurealerts remain active regardless of BMC status, but no test exercises that combination — onlyFatalandCriticalare covered alongsideWarning. Please add a case forSensorFailurewithBmcHealth::Okto actually verify that claim.Separately, all four new tests call the same operation (
to_health_result) varying onlyBmcHealth/metric value and assert a discriminant on the result — exactly the case the coding guidelines call out forvalue_scenarios!/check_valuesinstead of hand-rolled#[test]functions.Based on coding guidelines: "Reach for a table whenever two or more tests call the same operation with different inputs" and "Use check_cases / check_values directly when a macro would obscure a table with several inputs or several expected fields per row." Since the exact expected
SensorHealthResultisn't easilyPartialEq-comparable, a small wrapper predicate keeps the table simple:♻️ Illustrative table-driven refactor
fn is_alert(bmc_health: BmcHealth, value: f64) -> bool { let health = threshold_context(bmc_health); let metric = metric_with_value(value); matches!( HealthReportProcessor::to_health_result(&metric, &health), SensorHealthResult::Alert(_) ) } value_scenarios!(is_alert: "bmc ok" { (BmcHealth::Ok, 3.0) => Yields(true), // Fatal still alerts (BmcHealth::Ok, 85.0) => Yields(true), // Critical still alerts (BmcHealth::Ok, 95.0) => Yields(true), // SensorFailure still alerts (missing today) (BmcHealth::Ok, 72.0) => Yields(false), // Warning suppressed } "bmc not ok" { (BmcHealth::Warning, 72.0) => Yields(true), // Warning alerts when BMC disagrees } );Please confirm whether
carbide-test-supportis already a dev-dependency of this crate before adoptingvalue_scenarios!here.🤖 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/processor/health_report.rs` around lines 344 - 405, Refactor the four individual tests around HealthReportProcessor::to_health_result into a table-driven check using the project’s existing value_scenarios!/check_values support, after confirming carbide-test-support is available as a dev-dependency. Add the missing BmcHealth::Ok SensorFailure case (use a value above the fatal range, such as 95.0), while preserving expected alert results for Fatal, Critical, and non-Ok Warning and suppression for Ok Warning; use a small is_alert predicate if needed.Sources: Coding guidelines, Path instructions
🤖 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/processor/health_report.rs`:
- Around line 143-175: Replace the plain tracing::warn! in the
SensorHealth::Warning suppression branch with a carbide_instrument::Event
declared once and emitted for each suppression. Configure it as a warning
counter, keeping sensor_id and entity_type as context fields rather than labels,
while preserving the existing diagnostic fields and Success return behavior.
- Around line 344-405: Refactor the four individual tests around
HealthReportProcessor::to_health_result into a table-driven check using the
project’s existing value_scenarios!/check_values support, after confirming
carbide-test-support is available as a dev-dependency. Add the missing
BmcHealth::Ok SensorFailure case (use a value above the fatal range, such as
95.0), while preserving expected alert results for Fatal, Critical, and non-Ok
Warning and suppression for Ok Warning; use a small is_alert predicate if
needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0a47ec00-b870-4566-a53a-b9ebb9cf3a12
📒 Files selected for processing (1)
crates/health/src/processor/health_report.rs
hatamzad-nv
force-pushed
the
fix/3402-bmc-ok-suppresses-alerts
branch
from
1b3bf1d to
d0eed82
Compare
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
hatamzad-nv
force-pushed
the
fix/3402-bmc-ok-suppresses-alerts
branch
from
d0eed82 to
ec2ae7e
Compare
hatamzad-nv
force-pushed
the
fix/3402-bmc-ok-suppresses-alerts
branch
from
ec2ae7e to
8b42bad
Compare
|
@coderabbitai Would the tests here benefit from the table-driven scenario testing that is framed out by |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
fd . crates/test-support -t f | head -50
echo "---outline---"
ast-grep outline crates/test-support --items all 2>/dev/null | head -100
echo "---health_report test file---"
fd health_report.rsLength of output: 1415 🏁 Script executed: #!/bin/bash
set -euo pipefail
cat -n crates/test-support/src/lib.rs
echo "=== health_report.rs tests ==="
rg -n "mod tests" -A 5 crates/health/src/processor/health_report.rs
sed -n '1,400p' crates/health/src/processor/health_report.rs | grep -n "fn to_health_result\|SensorHealth\|BmcHealth\|struct\|enum" | head -60Length of output: 25361 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '1,60p;80,260p' crates/health/src/processor/health_report.rs
echo "=== full test module ==="
sed -n '260,460p' crates/health/src/processor/health_report.rsLength of output: 15770 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n "test-support" crates/health/Cargo.toml crates/*/Cargo.toml 2>/dev/null
cat crates/test-support/Cargo.tomlLength of output: 12413 Good —
Since there are two inputs ( Something like this would replace the five use carbide_test_support::value_scenarios;
struct Row {
value: f64,
bmc_health: BmcHealth,
}
#[test]
fn bmc_ok_suppresses_only_warning_alerts() {
value_scenarios!(run = |Row { value, bmc_health }| classify_result(value, bmc_health);
"BMC Ok does not suppress higher-severity alerts" {
// above upper_fatal (90.0) -> Fatal
Row { value: 95.0, bmc_health: BmcHealth::Ok } => Some(vec![Classification::SensorFatal]),
// above upper_critical (80.0) -> Critical
Row { value: 85.0, bmc_health: BmcHealth::Ok } => Some(vec![Classification::SensorCritical]),
// above range_max (100.0) -> SensorFailure
Row { value: 150.0, bmc_health: BmcHealth::Ok } => Some(vec![Classification::SensorFailure]),
}
"BMC Ok suppresses Warning (treated as likely mis-tuned caution thresholds)" {
// above upper_caution (70.0) -> Warning
Row { value: 72.0, bmc_health: BmcHealth::Ok } => None,
}
"BMC not Ok still alerts on Warning" {
Row { value: 72.0, bmc_health: BmcHealth::Warning } => Some(vec![Classification::SensorWarning]),
}
);
}A couple of notes:
Happy to draft this as a follow-up diff if you'd like me to open a suggestion — just let me know. (=^･ω･^=) |
2 participants
Completes #3402 by fixing the BMC-
Oksuppression. The other half of the issue(
lower_fatalboundrange_mininstead ofhealth.lower_fatal) was already fixedon
mainin #3401 (76357e2c5). Closes #3402.In
to_health_result, any non-Okclassification was downgraded toSuccesswheneverbmc_health == Ok. This meant a faulty or compromised BMC that self-reportsOksilencedindependent threshold alerting entirely — including
Critical,Fatal, andSensorFailure.This change gates the BMC-
Oksuppression to the lowest severity (Warning) only.Critical,Fatal, andSensorFailurenow always alert regardless of BMC self-report,while the noise-mitigation for mis-tuned caution thresholds is preserved for
Warning.Changes
crates/health/src/processor/health_report.rs: suppress on BMCOkonly when thecalculated state is
Warning.Tests
Added unit tests covering the new policy — alert cases assert the exact emitted
Classification, not just that an alert fired (all pass):bmc_ok_does_not_suppress_fatal_alert— Fatal + BMC Ok → Alert (SensorFatal)bmc_ok_does_not_suppress_critical_alert— Critical + BMC Ok → Alert (SensorCritical)bmc_ok_does_not_suppress_sensor_failure_alert— SensorFailure + BMC Ok → Alert (SensorFailure)bmc_ok_suppresses_warning_alert— Warning + BMC Ok → Success (intended)bmc_not_ok_still_alerts_on_warning— Warning + BMC not Ok → Alert (SensorWarning)cargo test -p carbide-healthandcargo clippy -p carbide-health --all-targetspass locally.