feat(health): preserve NVUE reboot reason metadata#3712
Merged
Conversation
Contributor
Summary by CodeRabbit
WalkthroughNVUE reboot data now emits structured log records containing reboot metadata while retaining the existing metric. Discovery propagates the log setting, tests cover enabled and disabled emission, and ChangesReboot reason logging
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant NVUECollector
participant TracingSink
participant TracingSubscriber
NVUECollector->>TracingSink: emit structured reboot LogRecord
TracingSink->>TracingSink: process record with diagnostics setting
TracingSink->>TracingSubscriber: emit tracing info event with attributes
🚥 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/health/src/collectors/nvue/rest/collector.rs (1)
1356-1406: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the required
unknownfallbacks.Add missing-field cases and assert
reason,gentime, anduserbecomeunknown; the current test only protects the fully populated response. Use a table with the populated case and missing-value variants. As per coding guidelines, “Use a table whenever two or more tests call the same operation with different inputs.”🤖 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/nvue/rest/collector.rs` around lines 1356 - 1406, Expand test_reboot_reason_emits_log_metadata_and_limits_metric_labels_to_reason into a table-driven test covering the fully populated response plus cases where reason, gentime, or user is missing. For each case, call emit_reboot_reason_data and assert the corresponding log attributes use "unknown", while preserving the existing metric and populated-value assertions.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.
Inline comments:
In `@crates/health/src/sink/tracing.rs`:
- Around line 83-95: Update the tracing::info! call in the log-record export
path to emit each entry from record.attributes as an individual tracing field
instead of the aggregated attributes = ?record.attributes field, preserving keys
such as message_id, switch_id, reason, gentime, and user for OpenTelemetry
queries. Update the associated test assertions to verify each attribute key is
exported separately.
---
Nitpick comments:
In `@crates/health/src/collectors/nvue/rest/collector.rs`:
- Around line 1356-1406: Expand
test_reboot_reason_emits_log_metadata_and_limits_metric_labels_to_reason into a
table-driven test covering the fully populated response plus cases where reason,
gentime, or user is missing. For each case, call emit_reboot_reason_data and
assert the corresponding log attributes use "unknown", while preserving the
existing metric and populated-value assertions.
🪄 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: da134857-aff7-4d17-918f-74a33686ca46
📒 Files selected for processing (2)
crates/health/src/collectors/nvue/rest/collector.rscrates/health/src/sink/tracing.rs
jayzhudev
changed the title
Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
jayzhudev
force-pushed
the
health/nvue-reboot-data
branch
from
July 20, 2026 02:29
a986d89 to
a5f8f90
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/health/src/collectors/nvue/rest/collector.rs (1)
1364-1437: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake reboot-reason coverage table-driven and cover
unknownfallbacks.The tests cover only a fully populated payload and the logging toggle. Add cases for missing
reason,gentime, anduser, asserting the requiredunknownvalues in both the metric and structured log output. These variants should be consolidated into one table-driven test rather than separate tests.As per coding guidelines and path instructions, use table-driven coverage for this missing-field contract; the PR objective requires
unknownfor missing reboot fields.🤖 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/nvue/rest/collector.rs` around lines 1364 - 1437, Replace the separate reboot-reason tests with one table-driven test covering fully populated data and each missing reason, gentime, or user variant. In the test around emit_reboot_reason_data, assert that missing fields produce "unknown" in both the structured log attributes and metric reason label, while preserving the existing assertions for log metadata, sample values, and the log-disabled metric-only behavior.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/collectors/nvue/rest/collector.rs`:
- Around line 1364-1437: Replace the separate reboot-reason tests with one
table-driven test covering fully populated data and each missing reason,
gentime, or user variant. In the test around emit_reboot_reason_data, assert
that missing fields produce "unknown" in both the structured log attributes and
metric reason label, while preserving the existing assertions for log metadata,
sample values, and the log-disabled metric-only behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ef37031d-1a96-4a20-ac6f-4d39be30ebeb
📒 Files selected for processing (3)
crates/health/src/collectors/nvue/rest/collector.rscrates/health/src/discovery/spawn.rscrates/health/src/sink/tracing.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/health/src/sink/tracing.rs
yoks
approved these changes
Jul 20, 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.
Route NVUE reboot metadata through log sinks, including OTLP.
Related issues
Closes #3711
Type of Change
Breaking Changes
Testing