refactor(agent,instrument): emit DPU report outcomes with native context#3746
Conversation
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughThe PR adds display and native-value context modes to instrumentation events, preserves captured tracing field types in tests, and replaces generic DPU report-loop telemetry with specialized success and failure events across inventory, configuration, FMDS, and network-status flows. ChangesTyped instrumentation context
DPU agent report events
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/agent/src/periodic_config_fetcher.rs (1)
208-211: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueEliminate the whole-response clone by moving
respintonetconf.
resp.clone()deep-copies every field of the response, yetresp.instanceis moved on the very next line. Clone only theinstanceyou need and move the rest.♻️ Proposed refinement
- Ok(resp) => { - state.netconf.store(Some(Arc::new(resp.clone()))); - - match instance_metadata_from_instance(resp.instance, state.sitename.clone()) { + Ok(resp) => { + let instance = resp.instance.clone(); + state.netconf.store(Some(Arc::new(resp))); + + match instance_metadata_from_instance(instance, state.sitename.clone()) {As per coding guidelines: "Avoid needless clones by borrowing, consuming iterators with into_iter, ordering struct initialization to move values directly, or using Cow when values may be borrowed or owned."
🤖 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/agent/src/periodic_config_fetcher.rs` around lines 208 - 211, Update the Ok(resp) branch to eliminate the whole-response clone: move resp into state.netconf.store, while separately cloning only resp.instance before the move for instance_metadata_from_instance. Preserve the existing metadata lookup and storage 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/agent/src/periodic_config_fetcher.rs`:
- Around line 208-211: Update the Ok(resp) branch to eliminate the
whole-response clone: move resp into state.netconf.store, while separately
cloning only resp.instance before the move for instance_metadata_from_instance.
Preserve the existing metadata lookup and storage behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7b030cef-69d3-4594-b967-9e9f8a7a2b35
📒 Files selected for processing (12)
crates/agent/Cargo.tomlcrates/agent/src/fmds_client.rscrates/agent/src/instrumentation.rscrates/agent/src/instrumentation/config.rscrates/agent/src/machine_inventory_updater.rscrates/agent/src/main_loop.rscrates/agent/src/periodic_config_fetcher.rscrates/instrument-macros/src/lib.rscrates/instrument/src/lib.rscrates/instrument/src/testing.rscrates/instrument/tests/matrix.rsdocs/observability/instrumentation.md
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3746.docs.buildwithfern.com/infra-controller |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/instrument-macros/src/lib.rs (1)
901-936: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse table-driven coverage for context-mode behavior. Both sites manually enumerate input-to-result variants that should be maintained as test cases.
crates/instrument-macros/src/lib.rs#L901-L936: table-drive accepted and rejected context attribute forms.crates/instrument/tests/matrix.rs#L117-L128: table-drive expected rendered values andCapturedFieldKindvalues.🤖 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/instrument-macros/src/lib.rs` around lines 901 - 936, Convert the context-mode tests in crates/instrument-macros/src/lib.rs (lines 901-936) into table-driven cases covering accepted and rejected attribute forms, while preserving the existing FieldKind assertion and error validation. Also table-drive the expected rendered values and CapturedFieldKind assertions in crates/instrument/tests/matrix.rs (lines 117-128), using each case’s input and expected outcomes.Source: Coding guidelines
crates/agent/src/periodic_config_fetcher.rs (1)
220-235: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnify error rendering with the sibling emission sites.
Two divergent formats appear here:
err.to_string()(Line 221) yields only the outermost eyre message and drops the cause chain, whileformat!("{err:?}")(Line 233) emits the verbose Debug report. Both differ from the convention used infmds_client.rsandmain_loop.rs, which render withformat!("{err:#}"). Aligning on the compact single-line chain preserves diagnostic context and keeps theerrorcontext field consistent across all report loops.♻️ Proposed alignment on
{err:#}- Err(err) => emit(ConfigFetchFailed::new( - err.to_string(), - state.config.config_fetch_interval.as_secs_f64(), - )), + Err(err) => emit(ConfigFetchFailed::new( + format!("{err:#}"), + state.config.config_fetch_interval.as_secs_f64(), + )),- _ => emit(ConfigFetchFailed::new( - format!("{err:?}"), - state.config.config_fetch_interval.as_secs_f64(), - )), + _ => emit(ConfigFetchFailed::new( + format!("{err:#}"), + state.config.config_fetch_interval.as_secs_f64(), + )),🤖 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/agent/src/periodic_config_fetcher.rs` around lines 220 - 235, Unify the error formatting in both ConfigFetchFailed emission sites within the periodic fetch flow: replace err.to_string() and format!("{err:?}") with the established format!("{err:#}") rendering. Keep the existing NotFound handling and retry interval values unchanged.
🤖 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/agent/src/periodic_config_fetcher.rs`:
- Around line 220-235: Unify the error formatting in both ConfigFetchFailed
emission sites within the periodic fetch flow: replace err.to_string() and
format!("{err:?}") with the established format!("{err:#}") rendering. Keep the
existing NotFound handling and retry interval values unchanged.
In `@crates/instrument-macros/src/lib.rs`:
- Around line 901-936: Convert the context-mode tests in
crates/instrument-macros/src/lib.rs (lines 901-936) into table-driven cases
covering accepted and rejected attribute forms, while preserving the existing
FieldKind assertion and error validation. Also table-drive the expected rendered
values and CapturedFieldKind assertions in crates/instrument/tests/matrix.rs
(lines 117-128), using each case’s input and expected outcomes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 26b372a9-9dfa-4367-ab74-bea3210a5969
📒 Files selected for processing (12)
crates/agent/Cargo.tomlcrates/agent/src/fmds_client.rscrates/agent/src/instrumentation.rscrates/agent/src/instrumentation/config.rscrates/agent/src/machine_inventory_updater.rscrates/agent/src/main_loop.rscrates/agent/src/periodic_config_fetcher.rscrates/instrument-macros/src/lib.rscrates/instrument/src/lib.rscrates/instrument/src/testing.rscrates/instrument/tests/matrix.rsdocs/observability/instrumentation.md
chet
changed the title
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 `@docs/observability/instrumentation.md`:
- Around line 169-171: Update the guidance around #[context(value)] to require
checked, lossless conversion when converting numeric widths; recommend retaining
default formatting whenever the conversion cannot be guaranteed lossless, and
avoid wording that could imply lossy casts are acceptable.
🪄 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: fd8eb928-3835-4a55-bebf-658d136181de
📒 Files selected for processing (12)
crates/agent/Cargo.tomlcrates/agent/src/fmds_client.rscrates/agent/src/instrumentation.rscrates/agent/src/instrumentation/config.rscrates/agent/src/machine_inventory_updater.rscrates/agent/src/main_loop.rscrates/agent/src/periodic_config_fetcher.rscrates/instrument-macros/src/lib.rscrates/instrument/src/lib.rscrates/instrument/src/testing.rscrates/instrument/tests/matrix.rsdocs/observability/instrumentation.md
🚧 Files skipped from review as they are similar to previous changes (11)
- crates/instrument/src/lib.rs
- crates/agent/Cargo.toml
- crates/agent/src/instrumentation/config.rs
- crates/agent/src/machine_inventory_updater.rs
- crates/instrument/tests/matrix.rs
- crates/agent/src/fmds_client.rs
- crates/agent/src/main_loop.rs
- crates/instrument-macros/src/lib.rs
- crates/instrument/src/testing.rs
- crates/agent/src/periodic_config_fetcher.rs
- crates/agent/src/instrumentation.rs
|
@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. |
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 8 minutes. |
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/instrument-macros/src/lib.rs (1)
901-936: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse table-driven context-mode parser cases.
Cover valid default/value modes and malformed/unknown forms in one scenario table, rather than maintaining isolated assertions.
As per coding guidelines, “Use table-driven tests for functions mapping inputs to outputs, errors, or observable results.”
🤖 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/instrument-macros/src/lib.rs` around lines 901 - 936, Refactor the test context_value_mode_is_explicit_and_validated into a table-driven set of context-mode parser cases. Include valid default and value modes plus malformed and unknown modes, and for each case assert the expected FieldKind or expansion error using the existing classify_field and expansion_error helpers.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/instrument-macros/src/lib.rs`:
- Around line 901-936: Refactor the test
context_value_mode_is_explicit_and_validated into a table-driven set of
context-mode parser cases. Include valid default and value modes plus malformed
and unknown modes, and for each case assert the expected FieldKind or expansion
error using the existing classify_field and expansion_error helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a30ff7d8-f221-4576-b83a-e418f78cba5f
📒 Files selected for processing (12)
crates/agent/Cargo.tomlcrates/agent/src/fmds_client.rscrates/agent/src/instrumentation.rscrates/agent/src/instrumentation/config.rscrates/agent/src/machine_inventory_updater.rscrates/agent/src/main_loop.rscrates/agent/src/periodic_config_fetcher.rscrates/instrument-macros/src/lib.rscrates/instrument/src/lib.rscrates/instrument/src/testing.rscrates/instrument/tests/matrix.rsdocs/observability/instrumentation.md
chet
changed the title
`carbide_dpu_agent_report_total` already described report-loop outcomes, but their diagnostic records were assembled independently at each call site and needed different context for each loop. Semantic sibling `Event`s now fix each valid loop/outcome pair in their constructors and preserve the existing metric series, log levels, messages, and context. A narrow `#[context(value)]` form keeps supported tracing values native without turning operational context into metric labels. Inventory failures remain metric-only because the scheduler owns their diagnostic. Table-driven tests cover the outcome matrix, native values, parser guardrails, and metric exposition. This supports NVIDIA#3731 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/agent/src/periodic_config_fetcher.rs (2)
208-214: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winClone only
instance, then moverespinto the store.
resp.instanceis the sole field consumed after the store, yet the entireManagedHostNetworkConfigResponseis deep-cloned. Cloning just the instance and movingrespavoids copying the network-configuration payload on every successful fetch.As per coding guidelines: "Avoid needless
.clone()calls by borrowing, moving withinto_iter, ordering struct initialization to permit moves".♻️ Proposed clone reduction
Ok(resp) => { - state.netconf.store(Some(Arc::new(resp.clone()))); - - match instance_metadata_from_instance(resp.instance, state.sitename.clone()) { + let instance = resp.instance.clone(); + state.netconf.store(Some(Arc::new(resp))); + + match instance_metadata_from_instance(instance, state.sitename.clone()) {🤖 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/agent/src/periodic_config_fetcher.rs` around lines 208 - 214, In the successful response branch, update the handling around instance_metadata_from_instance so only resp.instance is cloned before storing the original response, then move resp into state.netconf.store instead of cloning the entire ManagedHostNetworkConfigResponse. Preserve the existing metadata extraction and success-event behavior.Source: Coding guidelines
220-235: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnify error rendering on
{err:#}.This function renders the failure error two different ways —
err.to_string()for the conversion failure andformat!("{err:?}")for the RPC failure — while the rest of this PR (fmds_client.rs,main_loop.rs) usesformat!("{err:#}"). The alternateDisplayform ({err:#}) yields the fulleyrecause chain on a single line, whereasto_string()drops the chain and{err:?}emitsDebug. Aligning both call sites keeps theerrorcontext field consistent and complete.♻️ Proposed consistency fix
- Err(err) => emit(ConfigFetchFailed::new( - err.to_string(), - state.config.config_fetch_interval.as_secs_f64(), - )), + Err(err) => emit(ConfigFetchFailed::new( + format!("{err:#}"), + state.config.config_fetch_interval.as_secs_f64(), + )),- _ => emit(ConfigFetchFailed::new( - format!("{err:?}"), - state.config.config_fetch_interval.as_secs_f64(), - )), + _ => emit(ConfigFetchFailed::new( + format!("{err:#}"), + state.config.config_fetch_interval.as_secs_f64(), + )),🤖 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/agent/src/periodic_config_fetcher.rs` around lines 220 - 235, In the error-handling branches of the periodic config fetch flow, update both ConfigFetchFailed error renderings to use the eyre alternate Display form `{err:#}`. Replace the `err.to_string()` conversion-failure rendering and the `format!("{err:?}")` RPC-failure rendering, while preserving the existing intervals and NotFound handling.
🤖 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/agent/src/periodic_config_fetcher.rs`:
- Around line 208-214: In the successful response branch, update the handling
around instance_metadata_from_instance so only resp.instance is cloned before
storing the original response, then move resp into state.netconf.store instead
of cloning the entire ManagedHostNetworkConfigResponse. Preserve the existing
metadata extraction and success-event behavior.
- Around line 220-235: In the error-handling branches of the periodic config
fetch flow, update both ConfigFetchFailed error renderings to use the eyre
alternate Display form `{err:#}`. Replace the `err.to_string()`
conversion-failure rendering and the `format!("{err:?}")` RPC-failure rendering,
while preserving the existing intervals and NotFound handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0acc0d36-ec28-4d29-91ba-692bd7dd2849
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
crates/agent/Cargo.tomlcrates/agent/src/fmds_client.rscrates/agent/src/instrumentation.rscrates/agent/src/instrumentation/config.rscrates/agent/src/machine_inventory_updater.rscrates/agent/src/main_loop.rscrates/agent/src/periodic_config_fetcher.rscrates/instrument-macros/Cargo.tomlcrates/instrument-macros/src/lib.rscrates/instrument/src/lib.rscrates/instrument/src/testing.rscrates/instrument/tests/matrix.rs
The API state-change hook, managed-host republisher, and rack BMS worker
counted DSX publish outcomes separately from their matching diagnostics.
Typed sibling `Event`s now pair those signals while preserving the
frozen `{component,status}` series and each publisher's historical log
behavior. Operational detail remains log-only. The narrow `#[label(name
= "component")]` alias keeps the existing metric key without colliding
with the Event log's reserved `component` identity, and field-attribute
diagnostics reject unsupported metadata instead of silently ignoring it.
Table-driven tests cover the outcome matrix and alias guardrails.
## Related issues
- This supports #3732
- Documentation: #3767
- Part of #3169
## Type of Change
- [ ] **Add** - New feature or capability
- [ ] **Change** - Changes in existing functionality
- [ ] **Fix** - Bug fixes
- [ ] **Remove** - Removed features or deprecated functionality
- [x] **Internal** - Internal changes (refactoring, tests, docs, etc.)
## Breaking Changes
- [ ] **This PR contains breaking changes**
## Testing
- [x] Unit tests added/updated
- [x] Integration tests added/updated
- [ ] Manual testing performed
- [ ] No testing required (docs, internal refactor, etc.)
The bounded component/status mappings and publish-outcome metric/log
matrix use `value_scenarios!`; the proc-macro diagnostics use named
`check_values` rows. Stateful queue, republisher, and rack BMS behavior
remains covered by their existing focused tests.
Verified with:
- `cargo test -p carbide-instrument-macros -p carbide-instrument -p
carbide-mqtt-common`
- `cargo test -p carbide-api-core mqtt_state_change_hook --lib`
- `cargo test -p carbide-rack bms_client --lib`
- `cargo make format-nightly`
- `cargo make clippy`
- `cargo make carbide-lints`
- `cargo make check-event-names`
- `cargo xtask check-metric-docs`
- `cargo make check-workspace-deps`
- `cargo make check-licenses`
- `cargo make check-bans`
## Additional Notes
The `#[label(name = "component")]` guardrail tests and code-local
rustdoc stay with the implementation. The broader instrumentation-guide
update is split into #3767 and depends on this PR.
This PR and #3746 both extend Event field-attribute parsing. Merge
reconciliation must retain #3746's `#[context(value)]` mode and this
PR's bare-only `#[observation]` validation; metric-key aliases remain
exclusive to `#[label(name = "...")]`.
Signed-off-by: Chet Nichols III <chetn@nvidia.com>
2 participants
carbide_dpu_agent_report_totalalready described report-loop outcomes, but their diagnostic records were assembled independently at each call site and needed different context for each loop.Semantic sibling
Events now fix each valid loop/outcome pair in their constructors and preserve the existing metric series, log levels, messages, and context. A narrow#[context(value)]form keeps supported tracing values native without turning operational context into metric labels. Inventory failures remain metric-only because the scheduler owns their diagnostic.Table-driven tests cover the outcome matrix, native values, parser guardrails, and metric exposition.
Related issues
Type of Change
Breaking Changes
Testing
The ten report outcomes form one named
check_valuestable. Native tracing value coverage usesvalue_scenarios!, and the context-mode parser covers its valid and malformed forms through six namedcheck_casesrows. Metric exposition remains covered explicitly.Verified with:
cargo test -p carbide-agent --libcargo test -p carbide-instrument-macros -p carbide-instrumentcargo make format-nightlycargo make clippycargo make carbide-lintscargo make check-event-namescargo xtask check-metric-docscargo make check-workspace-depscargo make check-licensescargo make check-bansAdditional Notes
The broader
#[context(value)]guide is split into #3768 and depends on this implementation. Its parser tests pin diagnostics owned by this crate while only asserting rejection forsyn-owned wording, so a dependency copy edit does not break the suite. The pre-existing whole-response clone remains outside this instrumentation change, and the two config-fetch error records keep their historical%errand?errrenderings.Closes #3731