refactor(mqtt,instrument): pair DSX publish metrics and logs#3752
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
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:
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 (8)
Summary by CodeRabbit
WalkthroughThe PR introduces outcome-specific MQTT metrics with structured context, propagates machine identifiers through managed-host publishing, adds BMS-specific outcome reporting, and supports metric-only label aliases while preserving log field names. ChangesMQTT observability metrics
Metric label aliasing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant StateChangeHook
participant QueuedMessage
participant PublishWorker
participant MQTTBroker
participant MqttHookMetrics
StateChangeHook->>QueuedMessage: enqueue payload, topic, machine_id, deadline
PublishWorker->>QueuedMessage: consume queued message
PublishWorker->>MQTTBroker: publish payload before deadline
MQTTBroker-->>PublishWorker: success, error, or timeout
PublishWorker->>MqttHookMetrics: record managed outcome with topic and machine_id
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@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. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3752.docs.buildwithfern.com/infra-controller |
|
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. |
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 20 minutes. |
|
@coderabbitai full_review, thanks! |
1 similar comment
|
@coderabbitai full_review, 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/instrument-macros/src/lib.rs`:
- Around line 569-585: Update classify_field and its diagnostic handling to
reject metadata arguments on #[context] and #[observation] attributes, allowing
only path-form attributes for those field kinds. Add explicit diagnostic cases
for the unsupported non-path forms, while preserving label metadata parsing and
existing field classification behavior.
🪄 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: 4a63be55-6612-4701-b732-cbcfe181392b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
crates/api-core/src/mqtt_state_change_hook/hook.rscrates/api-core/src/mqtt_state_change_hook/republisher.rscrates/instrument-macros/Cargo.tomlcrates/instrument-macros/src/lib.rscrates/instrument/src/lib.rscrates/instrument/tests/matrix.rscrates/mqtt-common/src/hook.rscrates/mqtt-common/src/metrics.rscrates/rack/src/bms_client.rs
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/mqtt-common/src/metrics.rs (1)
366-366: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider
record_managed_overflowfor naming symmetry. Queue overflow is a managed-host-only outcome, yet this method drops themanaged_qualifier that its siblings (record_managed_success,record_managed_timeout,record_managed_publish_error) carry. Aligning the name now, while this public surface is fresh, keeps the API self-documenting and avoids a later rename touching every call site.♻️ Proposed rename
- pub fn record_overflow(&self, topic: String, machine_id: String, error: String) { + pub fn record_managed_overflow(&self, topic: String, machine_id: String, error: String) {Update the call site in
crates/api-core/src/mqtt_state_change_hook/hook.rs(record_overflow→record_managed_overflow) and the test in this file accordingly.🤖 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/mqtt-common/src/metrics.rs` at line 366, Rename the public method record_overflow to record_managed_overflow in the metrics implementation for naming consistency with its managed outcome siblings. Update every call site, including the MQTT state-change hook, and adjust the related test to use the new method name.
🤖 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/mqtt-common/src/metrics.rs`:
- Line 366: Rename the public method record_overflow to record_managed_overflow
in the metrics implementation for naming consistency with its managed outcome
siblings. Update every call site, including the MQTT state-change hook, and
adjust the related test to use the new method name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 157ef725-6dfd-49c9-81b3-2b43e75ba284
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
crates/api-core/src/mqtt_state_change_hook/hook.rscrates/api-core/src/mqtt_state_change_hook/republisher.rscrates/instrument-macros/Cargo.tomlcrates/instrument-macros/src/lib.rscrates/instrument/src/lib.rscrates/instrument/tests/matrix.rscrates/mqtt-common/src/hook.rscrates/mqtt-common/src/metrics.rscrates/rack/src/bms_client.rs
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
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/instrument-macros/src/lib.rs`:
- Around line 347-349: Update the Meta::Path branch in the attribute-name
parsing logic to validate ident.to_string() with the same metric-key rules used
for name aliases, rejecting non-ASCII names consistently. Add a regression test
covering a bare label with a non-ASCII identifier.
🪄 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: 82559268-d421-404b-a70e-4c98fe4f43d6
📒 Files selected for processing (8)
crates/api-core/src/mqtt_state_change_hook/hook.rscrates/api-core/src/mqtt_state_change_hook/republisher.rscrates/instrument-macros/src/lib.rscrates/instrument/src/lib.rscrates/instrument/tests/matrix.rscrates/mqtt-common/src/hook.rscrates/mqtt-common/src/metrics.rscrates/rack/src/bms_client.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- crates/api-core/src/mqtt_state_change_hook/hook.rs
- crates/instrument/src/lib.rs
- crates/instrument/tests/matrix.rs
- crates/rack/src/bms_client.rs
- crates/api-core/src/mqtt_state_change_hook/republisher.rs
- crates/mqtt-common/src/hook.rs
- crates/mqtt-common/src/metrics.rs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/instrument-macros/src/lib.rs (1)
339-385: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBare
#[label]names still bypass the metric-key ASCII guard.
Meta::Path(_) => Ok(ident.to_string())returns the Rust identifier verbatim with no validation, while thename = "..."alias branch enforces ASCII-letter/underscore-start plus ASCII-alphanumeric/underscore rest. Rust identifiers may legally contain non-ASCII Unicode characters (stablenon_ascii_idents), so a bare-labeled field can reach the metric label path with a key that violates the same contract the alias enforces.♻️ Proposed fix to share the validation
match &attr.meta { - Meta::Path(_) => Ok(ident.to_string()), + Meta::Path(_) => { + let name = ident.to_string(); + validate_metric_label_name(&name, ident)?; + Ok(name) + } Meta::List(_) => {Factor the ASCII check (currently inline in the
Meta::Listbranch) into a sharedvalidate_metric_label_name(&str, &impl ToTokens) -> syn::Result<()>helper and call it from both branches.🤖 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 339 - 385, Factor the ASCII metric-label name validation from label_metric_name into a shared validate_metric_label_name helper, accepting the name and an appropriate ToTokens span source for diagnostics. Invoke it for both bare #[label] identifiers and name = "..." aliases, preserving the existing error message and returning the original name only after validation.
🤖 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.
Duplicate comments:
In `@crates/instrument-macros/src/lib.rs`:
- Around line 339-385: Factor the ASCII metric-label name validation from
label_metric_name into a shared validate_metric_label_name helper, accepting the
name and an appropriate ToTokens span source for diagnostics. Invoke it for both
bare #[label] identifiers and name = "..." aliases, preserving the existing
error message and returning the original name only after validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d5f44b56-9628-414c-b667-5233ea2afa4f
📒 Files selected for processing (8)
crates/api-core/src/mqtt_state_change_hook/hook.rscrates/api-core/src/mqtt_state_change_hook/republisher.rscrates/instrument-macros/src/lib.rscrates/instrument/src/lib.rscrates/instrument/tests/matrix.rscrates/mqtt-common/src/hook.rscrates/mqtt-common/src/metrics.rscrates/rack/src/bms_client.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.
This supports NVIDIA#3732
Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
2 participants
The API state-change hook, managed-host republisher, and rack BMS worker counted DSX publish outcomes separately from their matching diagnostics.
Typed sibling
Events 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 reservedcomponentidentity, and field-attribute diagnostics reject unsupported metadata instead of silently ignoring it.Table-driven tests cover the outcome matrix and alias guardrails.
Related issues
Type of Change
Breaking Changes
Testing
The bounded component/status mappings and publish-outcome metric/log matrix use
value_scenarios!; the proc-macro diagnostics use namedcheck_valuesrows. 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-commoncargo test -p carbide-api-core mqtt_state_change_hook --libcargo test -p carbide-rack bms_client --libcargo 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
#[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 = "...")].