refactor(api): pair measured-boot failures with iteration latency#3740
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)
Summary by CodeRabbit
WalkthroughMeasured-boot iteration telemetry now logs failed iterations at WARN level with error context, keeps successful iterations silent, removes duplicate loop-level warnings, and tests logging and histogram behavior for both outcomes. ChangesMeasured-boot iteration logging
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
chet
changed the title
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 17 minutes. |
|
Hey @coderabbitai can you do a full_review when you get a chance? CICD isn't letting me continue because the CodeRabbit check "failed". Thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/api-core/src/measured_boot/metrics_collector/mod.rs`:
- Around line 120-121: Update the ignored result in the measured-boot iteration
flow around run_single_iteration to call .ok() on the awaited result instead of
assigning it to an underscore. Preserve the existing behavior and surrounding
failure logging.
🪄 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: 7a785a48-1282-4187-987e-e795b3a1fcc4
📒 Files selected for processing (1)
crates/api-core/src/measured_boot/metrics_collector/mod.rs
| // The iteration Event logs any failure before the result returns. | ||
| let _ = self.run_single_iteration().await; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Acknowledge the intentionally ignored result with .ok().
let _ = suppresses the fallible result directly. Use .ok() to make the deliberate error discard explicit while keeping future async-result changes visible.
Proposed fix
- let _ = self.run_single_iteration().await;
+ self.run_single_iteration().await.ok();As per coding guidelines, “Do not discard fallible results with let _unused = foo();; use .ok() when the error is intentionally ignored so future async changes remain compiler-visible.”
📝 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.
| // The iteration Event logs any failure before the result returns. | |
| let _ = self.run_single_iteration().await; | |
| self.run_single_iteration().await.ok(); |
🤖 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/api-core/src/measured_boot/metrics_collector/mod.rs` around lines 120
- 121, Update the ignored result in the measured-boot iteration flow around
run_single_iteration to call .ok() on the awaited result instead of assigning it
to an underscore. Preserve the existing behavior and surrounding failure
logging.
Source: Coding guidelines
chet
changed the title
Measured-boot collector iterations already recorded latency by outcome, but the warning for a failed pass came from the outer run loop. `MeasuredBootCollectorIteration` now owns both signals. Every pass still records one duration under the existing outcome series; failures add the historical warning and log-only error context, while successful passes remain silent. A table-driven test covers both outcomes without changing the histogram's name, description, labels, or millisecond observations. This supports NVIDIA#3729 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 21 minutes. |
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 2 minutes. |
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
2 participants
Measured-boot collector iterations already recorded latency by outcome, but the warning for a failed pass came from the outer run loop.
MeasuredBootCollectorIterationnow owns both signals. Every pass still records one duration under the existing outcome series; failures add the historical warning and log-only error context, while successful passes remain silent.A table-driven test covers both outcomes without changing the histogram's name, description, labels, or millisecond observations.
Related issues
Type of Change
Breaking Changes
Testing
A table-driven
check_valuestest covers the successful and failed outcomes, including log presence, histogram count, and histogram sum.Verified with:
carbide-api-coremeasured-bootEventtestscargo make format-nightlycargo make clippycargo make carbide-lintscargo make check-event-namescargo xtask check-metric-docsAdditional Notes
The dashboard-facing histogram name, description, labels, and millisecond observations are unchanged; only failed samples gain a correlated diagnostic record.
Closes #3729