Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ISSUE #8211] Add two metrics rocketmq_topic_create_execution_time and rocketmq_consumer_group_create_execution_time #8212

Merged
merged 14 commits into from
May 31, 2024

Conversation

Stephanie0002
Copy link
Contributor

@Stephanie0002 Stephanie0002 commented May 27, 2024

Which Issue(s) This PR Fixes

Fixes #8211

Brief Description

Add two metrics rocketmq_topic_create_execution_time and rocketmq_consumer_group_create_execution_time as following:

Type Name Unit Description Label
histogram rocketmq_topic_create_execution_time millisecond The topic creation time:
le_10_ms
le_100_ms
le_1_s
le_3_s
le_5_s
le_overflow
cluster,node_type,node_id,request_is_success,is_system
histogram rocketmq_consumer_group_create_execution_time millisecond The subscription group creation time:
le_10_ms
le_100_ms
le_1_s
le_3_s
le_5_s
le_overflow
cluster,node_type,node_id,request_is_success

How Did You Test This Change?

Simplified Testing Strategy

  1. Modify the metricsExporterType within the brokerConfig to PROM, which will export data in Prometheus format.
    ...
private MetricsExporterType metricsExporterType = MetricsExporterType.PROM;
...
private int metricsPromExporterPort = 5557;
private String metricsPromExporterHost = "127.0.0.1";
...
  1. Initiate both the broker and nameserver processes.
  2. Execute the following command in the local terminal: curl localhost:<export_port> (default is 5557), then verify that the output metrics align with expectations.

@Stephanie0002 Stephanie0002 changed the title [ISSUE #8211] Add two metrics rocketmq_create_topic_time and rocketmq_create_subscription_time #8211 [ISSUE #8211] Add two metrics rocketmq_create_topic_time and rocketmq_create_subscription_time May 27, 2024
@codecov-commenter
Copy link

codecov-commenter commented May 27, 2024

Codecov Report

Attention: Patch coverage is 43.75000% with 45 lines in your changes are missing coverage. Please review.

Project coverage is 42.85%. Comparing base (9c8fdb7) to head (0db33bb).
Report is 3 commits behind head on develop.

Files Patch % Lines
.../rocketmq/broker/metrics/BrokerMetricsManager.java 5.88% 32 Missing ⚠️
...ocketmq/broker/processor/AdminBrokerProcessor.java 66.66% 8 Missing and 5 partials ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #8212      +/-   ##
=============================================
- Coverage      42.94%   42.85%   -0.10%     
+ Complexity     10383    10371      -12     
=============================================
  Files           1270     1271       +1     
  Lines          88727    88775      +48     
  Branches       11407    11408       +1     
=============================================
- Hits           38108    38047      -61     
- Misses         45925    46023      +98     
- Partials        4694     4705      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines 526 to 527
Attributes attributes = BrokerMetricsManager.newAttributesBuilder()
.put(LABEL_REQUEST_IS_SUCCESS, response.getCode() == ResponseCode.SUCCESS)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the code reaches this point, it appears that the request must have been successful. Because the failed request has already been returned in advance

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I overlooked the issue with the logical branches. The latest changes incorporate the metric recording logic into the finally block, ensuring it executes under all circumstances.

黄梓淇 added 2 commits May 29, 2024 13:54
RongtongJin
RongtongJin previously approved these changes May 29, 2024
RongtongJin
RongtongJin previously approved these changes May 30, 2024
黄梓淇 added 3 commits May 30, 2024 18:01
…ription_number

Signed-off-by: 黄梓淇 <me@U-0MV57FM9-2309.local>
# Conflicts:
#	broker/src/main/java/org/apache/rocketmq/broker/metrics/BrokerMetricsConstant.java
#	broker/src/main/java/org/apache/rocketmq/broker/metrics/BrokerMetricsManager.java
Signed-off-by: 黄梓淇 <me@U-0MV57FM9-2309.local>
@Stephanie0002 Stephanie0002 changed the title [ISSUE #8211] Add two metrics rocketmq_create_topic_time and rocketmq_create_subscription_time [ISSUE #8211] Add two metrics rocketmq_topic_create_execution_time and rocketmq_consumer_group_create_execution_time May 31, 2024
黄梓淇 added 2 commits May 31, 2024 14:23
Signed-off-by: 黄梓淇 <me@U-0MV57FM9-2309.local>
Signed-off-by: 黄梓淇 <me@U-0MV57FM9-2309.local>
@RongtongJin RongtongJin merged commit 4027139 into apache:develop May 31, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Add two metrics rocketmq_create_topic_time and rocketmq_create_subscription_time
3 participants