[ISSUE #3454]🚀Add BrokerMetricsConstant struct and organized metrics for RocketMQ monitoring✨ #3455
Conversation
…for RocketMQ monitoring✨
WalkthroughA new module Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BrokerMetricsConstant
User->>BrokerMetricsConstant: Call utility method (e.g., get_all_gauge_metrics)
BrokerMetricsConstant-->>User: Return requested metric names
User->>BrokerMetricsConstant: Classify metric name (e.g., is_counter_metric)
BrokerMetricsConstant-->>User: Return classification result
User->>BrokerMetricsConstant: Retrieve label names
BrokerMetricsConstant-->>User: Return label names
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
🔊@mxsm 🚀Thanks for your contribution🎉！ 💡CodeRabbit(AI) will review your code first🔥！ Note 🚨The code review suggestions from CodeRabbit are to be used as a reference only, and the PR submitter can decide whether to make changes based on their own judgment. Ultimately, the project management personnel will conduct the final code review💥. |
rocketmq-rust-bot
requested review from
rocketmq-rust-bot,
SpaceXCN and
TeslaRustor
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new module to organize broker metrics constants for RocketMQ monitoring and updates the module visibility of consumer attributes.
- Added a new pub(crate) module "broker_metrics_constant" for broker metrics constants.
- Changed the module declaration of consumer_attr to pub(crate) to improve accessibility within the crate.
Comments suppressed due to low confidence (1)
rocketmq-broker/src/metrics.rs:17
- [nitpick] Consider using plural naming if this module is intended to hold multiple constants; for example, 'broker_metrics_constants' might better convey its purpose.
pub(crate) mod broker_metrics_constant;
![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
🧹 Nitpick comments (2)
rocketmq-broker/src/metrics/broker_metrics_constant.rs (2)
306-336:MetricCategory::from_metric_nameordering hides theTransactionbranchBecause the
"messages"check appears before the"commit"/"rollback"/"half"clause,
rocketmq_commit_messages_totalis categorised asMessageinstead ofTransaction.
This contradicts the semantic grouping and will surprise users querying dashboards.If that’s unintended, reorder the match arms:
- name if name.contains("messages") - || name.contains("throughput") - || name.contains("message_size") => - { - Some(Self::Message) - } + name if name.contains("commit") + || name.contains("rollback") + || name.contains("half") => + { + Some(Self::Transaction) + } + name if name.contains("messages") + || name.contains("throughput") + || name.contains("message_size") => + { + Some(Self::Message) + }If you intentionally treat commit/rollback as message metrics (the current unit-test expectation), please add a clarifying comment to avoid future confusion.
189-248: Avoid per-call heap allocations inget_all_*helpersEach helper currently allocates a fresh
Vec.
These are static sets that will never change at runtime; using&'static [&'static str]avoids repeated allocations and enables compile-time guarantees:- pub fn get_all_gauge_metrics() -> Vec<&'static str> { - vec![ /* … */ ] - } + pub const fn get_all_gauge_metrics() -> &'static [&'static str] { + &[ + Self::GAUGE_PROCESSOR_WATERMARK, + /* … */ + ] + }The same applies to the counter, histogram and label helpers.
This is a micro-optimisation but also clarifies the immutability of the data.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rocketmq-broker/src/metrics.rs(1 hunks)rocketmq-broker/src/metrics/broker_metrics_constant.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build
- GitHub Check: build (ubuntu-latest, nightly)
- GitHub Check: test
- GitHub Check: build (windows-latest, stable)
- GitHub Check: auto-approve
- GitHub Check: build (macos-latest, stable)
- GitHub Check: build (macos-latest, nightly)
- GitHub Check: build (windows-latest, nightly)
- GitHub Check: build (ubuntu-latest, stable)
🔇 Additional comments (1)
rocketmq-broker/src/metrics.rs (1)
17-18: Visibility change looks correctPromoting
consumer_attrtopub(crate)and exposing the newbroker_metrics_constantmodule integrates cleanly with the crate hierarchy.
No further action required.
| /// Check if a metric name is a gauge | ||
| pub fn is_gauge_metric(metric_name: &str) -> bool { | ||
| metric_name.starts_with("rocketmq_") | ||
| && !metric_name.contains("_total") | ||
| && !metric_name.contains("_time") | ||
| && !metric_name.contains("_size") | ||
| && !metric_name.contains("_latency") | ||
| } | ||
|
|
||
| /// Check if a metric name is a counter | ||
| pub fn is_counter_metric(metric_name: &str) -> bool { | ||
| metric_name.ends_with("_total") | ||
| } | ||
|
|
||
| /// Check if a metric name is a histogram | ||
| pub fn is_histogram_metric(metric_name: &str) -> bool { | ||
| metric_name.contains("_time") | ||
| || metric_name.contains("_size") | ||
| || metric_name.contains("_latency") | ||
| } |
There was a problem hiding this comment.
is_gauge_metric / is_histogram_metric mis-classify several declared gauge metrics
is_gauge_metric explicitly rejects names that contain _latency, _size, or _time, while is_histogram_metric treats those substrings as a positive signal.
However, you have six gauges that contain _latency:
rocketmq_consumer_lag_latency
rocketmq_consumer_queueing_latency
rocketmq_finish_message_latency // in histogram list
Consequences:
is_gauge_metric("rocketmq_consumer_lag_latency")→ false (should be true).is_histogram_metric("rocketmq_consumer_lag_latency")→ true (should be false).MetricType::from_metric_nametherefore yieldsHistogramfor a gauge, breaking downstream logic and any Prom-reg-type registration.
Minimal, allocation-free fix:
- pub fn is_gauge_metric(metric_name: &str) -> bool {
- metric_name.starts_with("rocketmq_")
- && !metric_name.contains("_total")
- && !metric_name.contains("_time")
- && !metric_name.contains("_size")
- && !metric_name.contains("_latency")
- }
-
- pub fn is_histogram_metric(metric_name: &str) -> bool {
- metric_name.contains("_time")
- || metric_name.contains("_size")
- || metric_name.contains("_latency")
- }
+ pub fn is_gauge_metric(metric_name: &str) -> bool {
+ Self::get_all_gauge_metrics().iter().any(|m| *m == metric_name)
+ }
+
+ pub fn is_histogram_metric(metric_name: &str) -> bool {
+ Self::get_all_histogram_metrics().iter().any(|m| *m == metric_name)
+ }This leverages the authoritative constant lists, eliminating heuristic drift.
Please update the related unit tests to cover the latency-suffixed gauges so the issue can’t regress.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In rocketmq-broker/src/metrics/broker_metrics_constant.rs around lines 250 to
269, the functions is_gauge_metric and is_histogram_metric incorrectly classify
metrics containing "_latency" by using substring heuristics that conflict with
the actual gauge metrics list. To fix this, replace the heuristic checks with
lookups against authoritative constant lists of gauge and histogram metric names
to ensure accurate classification without allocations. Also, update or add unit
tests to cover latency-suffixed gauge metrics like
"rocketmq_consumer_lag_latency" to prevent future regressions.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3455 +/- ##
==========================================
+ Coverage 26.09% 26.25% +0.16%
==========================================
Files 543 544 +1
Lines 77329 77511 +182
==========================================
+ Hits 20176 20354 +178
- Misses 57153 57157 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rocketmq-rust-bot
added
approved
Which Issue(s) This PR Fixes(Closes)
Fixes #3454
Brief Description
How Did You Test This Change?
Summary by CodeRabbit