fix: de-double the dsx-exchange-consumer counter metric names#3465
Conversation
|
@coderabbitai full_review, thanks! |
|
🐇✨ ✅ Action performedFull review finished. |
|
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 (2)
Summary by CodeRabbit
WalkthroughThe change normalizes message and alert counter registrations for Prometheus exporter suffix handling. Exposition tests now verify single ChangesMetric suffix normalization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
🚥 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}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/dsx-exchange-consumer/tests/metric_exposition.rs (1)
50-77: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCover
alerts_detectedin the exposition test.The registration changed in
crates/dsx-exchange-consumer/src/metrics.rsLines [139-143], but this test emits only the four message events. Add an alert-counter assertion for..._alerts_detected_totaland reject..._alerts_detected_total_total; otherwise that production change can regress without this test failing.🤖 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/tests/metric_exposition.rs` around lines 50 - 77, The exposition test currently omits the alerts_detected counter. Extend the metric names checked by the existing counter_delta loop to include carbide_dsx_exchange_consumer_alerts_detected_total, and add the corresponding doubled _total_total name to the rejection loop so both registration and de-duplication remain covered.
🤖 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.
Outside diff comments:
In `@crates/dsx-exchange-consumer/tests/metric_exposition.rs`:
- Around line 50-77: The exposition test currently omits the alerts_detected
counter. Extend the metric names checked by the existing counter_delta loop to
include carbide_dsx_exchange_consumer_alerts_detected_total, and add the
corresponding doubled _total_total name to the rejection loop so both
registration and de-duplication remain covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e92c4fef-8822-44c3-8ed3-6fddd50ab970
📒 Files selected for processing (2)
crates/dsx-exchange-consumer/src/metrics.rscrates/dsx-exchange-consumer/tests/metric_exposition.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/dsx-exchange-consumer/tests/metric_exposition.rs (1)
50-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the hand-rolled
alerts_detectedcounter.This test verifies only the four event counters. Add an alert recording and assert its single-suffix exposed name, including the
point_typelabel, plus the absence of its historical_total_totalname; otherwise the registration change incrates/dsx-exchange-consumer/src/metrics.rsLines 139-143 can regress unnoticed.🤖 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/tests/metric_exposition.rs` around lines 50 - 77, Extend the metric exposition test around the existing counter assertions to record one hand-rolled alerts_detected event, then assert its exposed single-suffix name increments by 1 with the point_type label. Also assert the rendered exposition does not contain the corresponding historical _total_total name, using the alerts_detected metric symbols and existing metric helpers.
🤖 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/tests/metric_exposition.rs`:
- Around line 50-77: Extend the metric exposition test around the existing
counter assertions to record one hand-rolled alerts_detected event, then assert
its exposed single-suffix name increments by 1 with the point_type label. Also
assert the rendered exposition does not contain the corresponding historical
_total_total name, using the alerts_detected metric symbols and existing metric
helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 43bc5425-25ae-4dcf-a14e-4afb7be91d20
📒 Files selected for processing (2)
crates/dsx-exchange-consumer/src/metrics.rscrates/dsx-exchange-consumer/tests/metric_exposition.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
The DSX exchange consumer's counters were exposed with a doubled suffix -- `carbide_dsx_exchange_consumer_messages_received_total_total` and its four siblings -- because the instruments were registered with a name already ending in `_total` and the Prometheus exporter appends another. NVIDIA#3427 preserved that byte-for-byte to keep the framework conversion a zero-observable-change reshape; this is the deliberate fix. - The four framework `#[derive(Event)]` counters drop the doubled `name = "..._total_total"` for the single-`_total` form and no longer need `name_unchecked` -- the framework strips one trailing `_total` before registering and the exporter re-appends exactly one, so a single-`_total` name exposes a single `_total`. - The hand-rolled `alerts_detected` counter registers without the trailing `_total` for the same reason, so all five dsx counters are now consistent. - The exposition tests assert the single-`_total` names and prove the doubled names are gone. The consumer serves its own `/metrics` and isn't in `core_metrics.md`, so there's no catalogue change -- but this renames the exposed series, so any dashboard or alert selecting the old `_total_total` names must migrate to the single-`_total` forms. Tests updated! This supports NVIDIA#3431 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 33 seconds. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3465.docs.buildwithfern.com/infra-controller |
2 participants
fix: de-double the dsx-exchange-consumer counter metric names
The DSX exchange consumer's counters were exposed with a doubled suffix --
carbide_dsx_exchange_consumer_messages_received_total_totaland its four siblings -- because the instruments were registered with a name already ending in_totaland the Prometheus exporter appends another. #3427 preserved that byte-for-byte to keep the framework conversion a zero-observable-change reshape; this is the deliberate fix.#[derive(Event)]counters drop the doubledname = "..._total_total"for the single-_totalform and no longer needname_unchecked-- the framework strips one trailing_totalbefore registering and the exporter re-appends exactly one, so a single-_totalname exposes a single_total.alerts_detectedcounter registers without the trailing_totalfor the same reason, so all five dsx counters are now consistent._totalnames and prove the doubled names are gone.The consumer serves its own
/metricsand isn't incore_metrics.md, so there's no catalogue change -- but this renames the exposed series, so any dashboard or alert selecting the old_total_totalnames must migrate to the single-_totalforms.This supports #3431