RockteMQ-AI commented on code in PR #2189:
URL:
https://github.com/apache/rocketmq-dashboard/pull/2189#discussion_r3781984338
##########
server/src/test/java/org/apache/rocketmq/studio/cluster/metrics/MetricProfileServiceTest.java:
##########
@@ -53,8 +53,12 @@ void rocketmq5ProfileShouldUseNativeMetricNames() {
"sum(rate(rocketmq_messages_in_total[1m])) by
(cluster, node_id)");
assertThat(mapping(profile,
SemanticMetric.CONSUMER_LAG_MESSAGES).getPrometheusMetric())
.isEqualTo("rocketmq_consumer_lag_messages");
- assertThat(mapping(profile,
SemanticMetric.BROKER_HEALTH).getPrometheusMetric())
- .isEqualTo("rocketmq_processor_watermark");
+ assertThat(mapping(profile, SemanticMetric.BROKER_HEALTH))
Review Comment:
The test verifies metric name, PromQL, and labels for the RocketMQ 5 native
profile, which is good. However, there is no assertion confirming that other
profiles (e.g. RocketMQ 4 / fallback) do NOT use the `up` metric, and no test
for the semantic metric ordering or total count in the profile. Adding a
negative assertion or a profile-size check would guard against accidental
duplication.
##########
server/src/main/java/org/apache/rocketmq/studio/cluster/metrics/MetricProfileService.java:
##########
@@ -100,9 +100,9 @@ private List<MetricProfileVO.MetricMappingVO>
rocketmq5NativeMetrics() {
mapping(SemanticMetric.CONSUMER_LAG_LATENCY,
"rocketmq_consumer_lag_latency",
"max(rocketmq_consumer_lag_latency) by (cluster,
topic, consumer_group)",
"cluster", "topic", "consumer_group"),
- mapping(SemanticMetric.BROKER_HEALTH,
"rocketmq_processor_watermark",
- "max(rocketmq_processor_watermark) by (cluster,
node_id, processor)",
- "cluster", "node_id", "processor")
+ mapping(SemanticMetric.BROKER_HEALTH, "up",
+ "min(up{job=~\".*rocketmq.*\"}) by (job, instance)",
Review Comment:
The `up` metric is scrape-level availability and does not distinguish
between 'broker process is running but unhealthy' and 'broker endpoint is
unreachable'. The old `rocketmq_processor_watermark` at least reflected
internal broker state. If deeper liveness semantics are needed later (e.g.
distinguishing a broker that is up but not accepting writes), this will need to
be revisited. Documenting this trade-off in a code comment or the PR
description would help future maintainers.
##########
server/src/main/java/org/apache/rocketmq/studio/cluster/metrics/MetricProfileService.java:
##########
@@ -100,9 +100,9 @@ private List<MetricProfileVO.MetricMappingVO>
rocketmq5NativeMetrics() {
mapping(SemanticMetric.CONSUMER_LAG_LATENCY,
"rocketmq_consumer_lag_latency",
"max(rocketmq_consumer_lag_latency) by (cluster,
topic, consumer_group)",
"cluster", "topic", "consumer_group"),
- mapping(SemanticMetric.BROKER_HEALTH,
"rocketmq_processor_watermark",
- "max(rocketmq_processor_watermark) by (cluster,
node_id, processor)",
- "cluster", "node_id", "processor")
+ mapping(SemanticMetric.BROKER_HEALTH, "up",
+ "min(up{job=~\".*rocketmq.*\"}) by (job, instance)",
Review Comment:
Backward-compatibility break: the label set changes from (cluster, node_id,
processor) to (job, instance). Any downstream consumer — dashboards, alerts, or
API clients — that reads BROKER_HEALTH results and keys on 'cluster' or
'node_id' will silently receive null/missing labels. The RocketMQ-domain
identity (which cluster, which broker node) is replaced by Prometheus
scrape-target identity (which scrape job, which endpoint). If this is
acceptable, it should be called out in release notes; if not, consider keeping
at least 'cluster' as a label or adding a relabeling rule in the Prometheus
config.
##########
server/src/main/java/org/apache/rocketmq/studio/cluster/metrics/MetricProfileService.java:
##########
@@ -100,9 +100,9 @@ private List<MetricProfileVO.MetricMappingVO>
rocketmq5NativeMetrics() {
mapping(SemanticMetric.CONSUMER_LAG_LATENCY,
"rocketmq_consumer_lag_latency",
"max(rocketmq_consumer_lag_latency) by (cluster,
topic, consumer_group)",
"cluster", "topic", "consumer_group"),
- mapping(SemanticMetric.BROKER_HEALTH,
"rocketmq_processor_watermark",
- "max(rocketmq_processor_watermark) by (cluster,
node_id, processor)",
- "cluster", "node_id", "processor")
+ mapping(SemanticMetric.BROKER_HEALTH, "up",
+ "min(up{job=~\".*rocketmq.*\"}) by (job, instance)",
Review Comment:
The job-name regex `.*rocketmq.*` is broad. Any Prometheus scrape job whose
name contains the substring 'rocketmq' (e.g. a sidecar exporter, a CI job, or a
differently-purposed service) will be included in the health signal,
potentially masking a real broker outage or producing false negatives. Consider
tightening to a more specific pattern (e.g. `rocketmq-broker.*` or
`rocketmq-exporter.*`) once the expected job names are documented, or make the
filter configurable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
