[ISSUE #3456]🚀Add PopMetricsConstant struct and organized metrics for RocketMQ Pop operations✨ #3457
Conversation
… RocketMQ Pop operations✨
WalkthroughA new module, Changes
Sequence Diagram(s)sequenceDiagram
participant Broker as Broker
participant PopMetricsConstant as PopMetricsConstant
participant MetricsSystem as Metrics System
Broker->>PopMetricsConstant: Retrieve metric/label constants
Broker->>PopMetricsConstant: Use utility functions to classify metrics
Broker->>PopMetricsConstant: Build labels with PopMetricLabels
Broker->>MetricsSystem: Register or update Pop metrics with names/labels
Assessment against linked issues
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
|
rocketmq-rust-bot
requested review from
rocketmq-rust-bot,
SpaceXCN and
TeslaRustor
|
🔊@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
added
AI review first
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new pop_metrics_constant module to centralize constants for RocketMQ Pop operation metrics.
- Added
pop_metrics_constantto the broker metrics registry.
Comments suppressed due to low confidence (1)
rocketmq-broker/src/metrics.rs:19
- [nitpick] Consider adding a module-level doc comment in
pop_metrics_constant.rsto describe its purpose and usage, consistent with other metric modules.
pub(crate) mod pop_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: 3
🧹 Nitpick comments (3)
rocketmq-broker/src/metrics.rs (1)
19-19: Add a brief module-level docstring.Every sub-module exposed here already carries a one-liner comment except the new
pop_metrics_constant; adding the same will keep the public API list self-documented.rocketmq-broker/src/metrics/pop_metrics_constant.rs (2)
124-130:get_all_metrics()may contain duplicates.If a constant is accidentally listed in two of the subgroup helpers, duplicates will appear.
Returning aHashSet<&'static str>(or deduplicating withVec::sort_unstable+dedup) guards against this with negligible cost.
301-327: Label vector order is non-deterministic.Prometheus client libraries don’t rely on order, but caching layers sometimes do.
Using aBTreeMapinternally or sorting before return guarantees stable ordering:- labels + labels.sort_by(|a, b| a.0.cmp(&b.0)); + labels
📜 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/pop_metrics_constant.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build
- GitHub Check: test
- GitHub Check: build (windows-latest, nightly)
- GitHub Check: build (macos-latest, nightly)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: build (ubuntu-latest, stable)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build (ubuntu-latest, nightly)
- GitHub Check: auto-approve
🔇 Additional comments (1)
rocketmq-broker/src/metrics/pop_metrics_constant.rs (1)
186-196: Category detection order can hide thePerformanceclass.Because the
buffercheck precedestime_consume, a metric such as
rocketmq_pop_buffer_scan_time_consumeis categorised asBuffer, neverPerformance.
Is that intentional? If not, swap the twocontainschecks or use a more explicit mapping table.
| impl From<&str> for PutStatus { | ||
| fn from(s: &str) -> Self { | ||
| match s.to_lowercase().as_str() { | ||
| "put_ok" => Self::PutOk, | ||
| "create_mapped_file_failed" => Self::CreateMappedFileFailed, | ||
| "message_illegal" => Self::MessageIllegal, | ||
| "properties_length_too_long" => Self::PropertiesLengthTooLong, | ||
| "service_not_available" => Self::ServiceNotAvailable, | ||
| "os_page_cache_busy" => Self::OsPageCacheBusy, | ||
| _ => Self::UnknownError, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same fallback issue for PutStatus.
UnknownError becomes a catch-all; callers cannot distinguish “real unknown error” from “bad input”.
Adopt TryFrom/Option as suggested for ReviveMessageType.
🤖 Prompt for AI Agents
In rocketmq-broker/src/metrics/pop_metrics_constant.rs around lines 255 to 266,
the From<&str> implementation for PutStatus uses a catch-all UnknownError for
any unrecognized input, which prevents callers from distinguishing between
actual unknown errors and invalid inputs. Change the implementation to use
TryFrom<&str> returning a Result or Option, so that invalid inputs can be
explicitly handled without conflating them with genuine unknown errors.
| /// Check if a metric name is a Pop-related metric | ||
| pub fn is_pop_metric(metric_name: &str) -> bool { | ||
| metric_name.starts_with("rocketmq_pop_") | ||
| } | ||
|
|
||
| /// Check if a metric name is a Pop counter | ||
| pub fn is_pop_counter_metric(metric_name: &str) -> bool { | ||
| Self::is_pop_metric(metric_name) && metric_name.ends_with("_total") | ||
| } | ||
|
|
||
| /// Check if a metric name is a Pop gauge | ||
| pub fn is_pop_gauge_metric(metric_name: &str) -> bool { | ||
| Self::is_pop_metric(metric_name) | ||
| && !metric_name.ends_with("_total") | ||
| && !metric_name.contains("_time_consume") | ||
| } | ||
|
|
||
| /// Check if a metric name is a Pop histogram | ||
| pub fn is_pop_histogram_metric(metric_name: &str) -> bool { | ||
| Self::is_pop_metric(metric_name) && metric_name.contains("_time_consume") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Heuristic may misclassify future metrics.
is_pop_gauge_metric tags every name that is not _total and not _time_consume as a gauge.
A future counter such as rocketmq_pop_revive_failure_count (no _total) or another histogram without _time_consume would be reported as a gauge.
Consider a whitelist/lookup generated from get_all_*_metrics() instead of suffix heuristics to avoid silent mis-categorisation.
🤖 Prompt for AI Agents
In rocketmq-broker/src/metrics/pop_metrics_constant.rs around lines 132 to 152,
the current heuristic in is_pop_gauge_metric uses suffix and substring checks
that may misclassify future metrics without expected suffixes. To fix this,
replace the heuristic logic with a whitelist or lookup approach by generating
sets of known metric names from get_all_*_metrics() functions and checking
membership in those sets to accurately categorize metrics and avoid silent
misclassification.
| impl From<&str> for ReviveMessageType { | ||
| fn from(s: &str) -> Self { | ||
| match s.to_lowercase().as_str() { | ||
| "normal" => Self::Normal, | ||
| "retry" => Self::Retry, | ||
| "dlq" => Self::Dlq, | ||
| _ => Self::Normal, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
From<&str> silently falls back to Normal.
Returning a valid enum for unknown input risks masking bad label values.
Prefer:
-impl From<&str> for ReviveMessageType {
- fn from(s: &str) -> Self {
+impl TryFrom<&str> for ReviveMessageType {
+ type Error = ();
+ fn try_from(s: &str) -> Result<Self, Self::Error> {and propagate the error, or at least return None via Option.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In rocketmq-broker/src/metrics/pop_metrics_constant.rs around lines 217 to 226,
the From<&str> implementation for ReviveMessageType silently defaults to Normal
for unknown strings, which can mask invalid inputs. Change the implementation to
return a Result or Option instead of From, so that invalid strings produce an
error or None. This involves replacing the From trait with a TryFrom or a custom
parsing function that returns Result<Self, Error> or Option<Self>, and handling
unknown inputs explicitly without defaulting to Normal.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3457 +/- ##
==========================================
+ Coverage 26.25% 26.47% +0.21%
==========================================
Files 544 545 +1
Lines 77511 77749 +238
==========================================
+ Hits 20354 20583 +229
- Misses 57157 57166 +9 ☔ 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 #3456
Brief Description
How Did You Test This Change?
Summary by CodeRabbit