refactor(dsx): emit queue drops, deduplications, and leak alerts#3742
Conversation
|
@coderabbitai full_review, thanks! |
|
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 (7)
Summary by CodeRabbit
WalkthroughThe DSX exchange consumer replaces manual alert metrics with structured instrumentation events. Drop, deduplication, and leak-alert events include typed context and labels, runtime wiring uses the simplified ChangesDSX consumer observability
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MQTTConsumer
participant HealthUpdater
participant CarbideInstrument
participant Prometheus
MQTTConsumer->>CarbideInstrument: Emit typed MessageDropped event
HealthUpdater->>CarbideInstrument: Emit MessageDeduplicated or LeakAlertDetected
CarbideInstrument->>Prometheus: Record counters and labels
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🐇🔍 ✅ Action performedFull review finished. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3742.docs.buildwithfern.com/infra-controller |
![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/dsx-exchange-consumer/src/metrics.rs (1)
173-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider modeling
point_typeasLeakPointTypeinstead ofString.
MessageDeduplicated::point_typeis a bareStringsourced frommetadata.point_type.clone(), even though a validatedLeakPointType(leak_type) is already computed and in scope at the only call site (health_updater.rs,handle_value_message).DroppedMessageTypein this same file already proves#[context]fields can be non-Stringenums, so this isn't a macro limitation. Using the canonical enum instead of re-threading the raw metadata string avoids any risk of the loggedpoint_typedrifting from the value that was actually validated vialeak_point_type().♻️ Proposed refactor
pub struct MessageDeduplicated { #[context] pub point_path: String, #[context] - pub point_type: String, + pub point_type: LeakPointType, #[context] pub value: String, }And at the call site in
crates/dsx-exchange-consumer/src/health_updater.rs:emit(MessageDeduplicated { point_path: point_path.to_string(), - point_type: metadata.point_type.clone(), + point_type: leak_type, value: format!("{value:?}"), });As per coding guidelines,
**/*.{rs,rs.in}files should "model finite sets of values with enums or structs of enums, deriving string representations throughDisplay/FromStr; avoid passing them as bareStringor&strvalues."🤖 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/dsx-exchange-consumer/src/metrics.rs` around lines 173 - 192, Change MessageDeduplicated::point_type from String to the validated LeakPointType enum, following the existing non-String context field pattern such as DroppedMessageType. At the handle_value_message call site, pass the in-scope leak_type produced by leak_point_type() instead of metadata.point_type.clone(), preserving the logged value’s alignment with validation.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/dsx-exchange-consumer/src/metrics.rs`:
- Around line 173-192: Change MessageDeduplicated::point_type from String to the
validated LeakPointType enum, following the existing non-String context field
pattern such as DroppedMessageType. At the handle_value_message call site, pass
the in-scope leak_type produced by leak_point_type() instead of
metadata.point_type.clone(), preserving the logged value’s alignment with
validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 51ac0f23-abb4-45b3-b50b-cd32aaa1a9d8
📒 Files selected for processing (7)
crates/dsx-exchange-consumer/src/health_updater.rscrates/dsx-exchange-consumer/src/lib.rscrates/dsx-exchange-consumer/src/metrics.rscrates/dsx-exchange-consumer/src/mqtt_consumer.rscrates/dsx-exchange-consumer/tests/dedup_trace.rscrates/dsx-exchange-consumer/tests/metric_exposition.rsdocs/observability/core_metrics.md
|
@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/dsx-exchange-consumer/src/lib.rs`:
- Around line 103-105: Ensure the health-updater task created from
HealthUpdater::run is supervised when the metrics listener wins the
tokio::select! path: abort it and await its JoinHandle before returning, or
manage both tasks through a JoinSet. Preserve observable task panics and
shutdown completion, while retaining the intentional metrics-endpoint exception.
🪄 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: 0069364b-eada-476d-b274-5c359cba34ea
📒 Files selected for processing (7)
crates/dsx-exchange-consumer/src/health_updater.rscrates/dsx-exchange-consumer/src/lib.rscrates/dsx-exchange-consumer/src/metrics.rscrates/dsx-exchange-consumer/src/mqtt_consumer.rscrates/dsx-exchange-consumer/tests/dedup_trace.rscrates/dsx-exchange-consumer/tests/metric_exposition.rsdocs/observability/core_metrics.md
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/dsx-exchange-consumer/src/mqtt_consumer.rs
- docs/observability/core_metrics.md
- crates/dsx-exchange-consumer/tests/dedup_trace.rs
- crates/dsx-exchange-consumer/tests/metric_exposition.rs
- crates/dsx-exchange-consumer/src/health_updater.rs
- crates/dsx-exchange-consumer/src/metrics.rs
chet
changed the title
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 21 minutes. |
The DSX exchange consumer counted queue drops, deduplications, and leak alerts separately from the records that explained them. Leak alerts also retained the consumer's final hand-built counter. Typed `Event`s now own each existing counter and diagnostic together. Drop and deduplication counters remain zero-label, leak alerts preserve the BMS CamelCase `LeakPointType` values, and rack/point detail stays log-only. The empty metrics holder left behind is removed. Metric/log exposition and the bounded label mappings are covered with table-driven tests. This supports NVIDIA#3733 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
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. Your plan includes PR reviews subject to rate limits. More reviews will be available in 13 minutes. |
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
2 participants
The DSX exchange consumer counted queue drops, deduplications, and leak alerts separately from the records that explained them. Leak alerts also retained the consumer's final hand-built counter.
Typed
Events now own each existing counter and diagnostic together. Drop and deduplication counters remain zero-label, leak alerts preserve the BMS CamelCaseLeakPointTypevalues, and rack/point detail stays log-only. The empty metrics holder left behind is removed.Metric/log exposition and the bounded label mappings are covered with table-driven tests.
Related issues
Type of Change
Breaking Changes
Testing
The metric series, log fields, exporter names, and canonical leak-point labels are covered by named
check_valuesandvalue_scenarios!rows. Stateful deduplication and health-report persistence remain explicit tests.Verified with:
cargo test -p carbide-dsx-exchange-consumercargo make format-nightlycargo make clippycargo make carbide-lintscargo make check-event-namescargo xtask check-metric-docsAdditional Notes
LeakPointTypekeeps a manualLabelValuemapping because the existing metric contract uses the BMS CamelCase spellings. Deriving that mapping would silently change them to snake_case.The generated
docs/observability/core_metrics.mdrow stays with this implementation because it is validated as part of the Event contract and is excluded from technical-writer review.Closes #3733