Skip to content

[improve][broker] Add metric prefix for topic_load_times #20472

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.

Sign up for GitHub

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

Jump to bottom
Merged
Technoboy- merged 4 commits into from
Jun 29, 2023
Merged

[improve][broker] Add metric prefix for topic_load_times #20472

Technoboy- merged 4 commits into from
Jun 29, 2023

Conversation

@Technoboy-
Copy link
Contributor

@Technoboy- Technoboy- commented Jun 2, 2023 •
edited by tisonkun
Loading

Fix PIP-273 #20476

Motivation

topic_load_times was added by #507, it's too earlier. And then we added the pulsar prefix for all the metrics, but leaves it. So in order to keep the same prefix, we'd better update it.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@Technoboy- Technoboy- self-assigned this Jun 2, 2023
@Technoboy- Technoboy- added this to the 3.1.0 milestone Jun 2, 2023
@Technoboy- Technoboy- added area/metrics release/important-notice The changes which are important should be mentioned in the release note labels Jun 2, 2023
@github-actions github-actions bot added the doc-required Your PR changes impact docs and you will update later. label Jun 2, 2023
codelipenghui
codelipenghui reviewed Jun 2, 2023
View reviewed changes
pulsar-broker/src/main/java/org/apache/pulsar/broker/stats/BrokerOperabilityMetrics.java Outdated

Metrics getTopicLoadMetrics() {
return getDimensionMetrics("topic_load_times", "topic_load", topicLoadStats);
return getDimensionMetrics("brk_topic_load_times", "topic_load", topicLoadStats);
Copy link
Contributor

@codelipenghui codelipenghui Jun 2, 2023

Choose a reason for hiding this comment

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

@Technoboy- Will it break the existing metrics system? As we discussed before, all the metrics changes should be started with a proposal since it might surprise the pulsar operators, and impact the alert system, and metrics system.

Copy link
Contributor Author

@Technoboy- Technoboy- Jun 2, 2023

Choose a reason for hiding this comment

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

Ok, I will create a PIP for this.

@codecov-commenter
Copy link

codecov-commenter commented Jun 2, 2023

Codecov Report

Merging #20472 (7c049e8) into master (5e6e6ce) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #20472      +/-   ##
============================================
- Coverage     72.95%   72.92%   -0.04%     
+ Complexity    31914    31898      -16     
============================================
  Files          1867     1867              
  Lines        138485   138484       -1     
  Branches      15202    15202              
============================================
- Hits         101037   100990      -47     
- Misses        29438    29481      +43     
- Partials       8010     8013       +3
Flag Coverage Δ
inttests 24.17% <50.00%> (+0.05%) ⬆️
systests 24.99% <50.00%> (-0.04%) ⬇️
unittests 72.21% <100.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...bookkeeper/mledger/ManagedLedgerFactoryConfig.java 89.47% <ø> (-0.53%) ⬇️
.../pulsar/broker/stats/BrokerOperabilityMetrics.java 100.00% <100.00%> (ø)
...org/apache/pulsar/broker/stats/DimensionStats.java 87.27% <100.00%> (ø)

... and 73 files with indirect coverage changes

codelipenghui
codelipenghui reviewed Jun 27, 2023
View reviewed changes
Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

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

@Technoboy- Since the indicator name has been changed, could you please help add a test to avoid regression?

Demogorgon314
Demogorgon314 approved these changes Jun 27, 2023
View reviewed changes
Copy link
Member

@Demogorgon314 Demogorgon314 left a comment

Choose a reason for hiding this comment

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

LGTM. Do we have a unit test to cover this?

@Technoboy-
Copy link
Contributor Author

Technoboy- commented Jun 28, 2023

LGTM. Do we have a unit test to cover this?

added

@Technoboy-
Copy link
Contributor Author

Technoboy- commented Jun 28, 2023

@Technoboy- Since the indicator name has been changed, could you please help add a test to avoid regression?

I thought the current tests might cover this, but they didn't. So I have added the tests. Also during adding the tests, I find the indicator name should be "pulsar_xxx", not "brk_xxx", please also review again.
@codelipenghui @Demogorgon314 @hangc0276

@Technoboy- Technoboy- force-pushed the change-metric-name branch from d924e2d to 1d01a67 Compare June 29, 2023 01:08
@Technoboy- Technoboy- merged commit 8d7b633 into apache:master Jun 29, 2023
@Anonymitaet Anonymitaet mentioned this pull request Jul 5, 2023
Merged
1 task
@Anonymitaet
Copy link
Member

Anonymitaet commented Jul 5, 2023

I've added the doc in apache/pulsar-site#631

@Anonymitaet Anonymitaet added doc-complete Your PR changes impact docs and the related docs have been already added. and removed doc-required Your PR changes impact docs and you will update later. labels Jul 7, 2023
@Technoboy- Technoboy- deleted the change-metric-name branch November 11, 2023 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hangc0276 hangc0276 hangc0276 approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@Demogorgon314 Demogorgon314 Demogorgon314 approved these changes

@poorbarcode poorbarcode poorbarcode approved these changes

Assignees

@Technoboy- Technoboy-

Labels

area/metrics doc-complete Your PR changes impact docs and the related docs have been already added. ready-to-test release/important-notice The changes which are important should be mentioned in the release note

Projects

None yet

Milestone

3.1.0

Development

Successfully merging this pull request may close these issues.

7 participants

@Technoboy- @codecov-commenter @Anonymitaet @hangc0276 @codelipenghui @Demogorgon314 @poorbarcode