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

[improve][broker] add limitStatsLogging configuration when enabling bookkeeperClientExposeStatsToPrometheus #16734

Merged
merged 4 commits into from
Dec 10, 2022

Conversation

TakaHiR07
Copy link
Contributor

@TakaHiR07 TakaHiR07 commented Jul 22, 2022

Motivation

This closes #16732.

Modifications

Add bookkeeperClientLimitStatsLogging configuration for broker.conf

After enable bookkeeperClientExposeStatsToPrometheus and bookkeeperClientLimitStatsLogging, there are no more per_channel_bookie_client metrics in bookie client stats.

wecom-temp-6ba3feaee4543aeb5bbcba3c26f51956
wecom-temp-9ba30e4d92209f2120c2850ac1b6d120

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): no
  • The public API: no
  • The schema: no
  • The default values of configurations: yes
  • The wire protocol: no
  • The rest endpoints: no
  • The admin cli options: no
  • Anything that affects deployment: no

Documentation

Check the box below or label this PR directly.

Need to update docs?

  • doc

…nnel_bookie_client metrics of bkclient stats
@TakaHiR07 TakaHiR07 changed the title add limitStatsLogging configuration when enable bookkeeperClientExposeStatsToPrometheus [improve] add limitStatsLogging configuration when enable bookkeeperClientExposeStatsToPrometheus Jul 22, 2022
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.

Could you please add an unit test to cover the new configuration? Make sure the default value is false and change the value is work property.

@codelipenghui codelipenghui added this to the 2.11.0 milestone Jul 23, 2022
@codelipenghui codelipenghui added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/metrics labels Jul 23, 2022
@github-actions
Copy link

@TakaHiR07 Please provide a correct documentation label for your PR.
Instructions see Pulsar Documentation Label Guide.

@TakaHiR07
Copy link
Contributor Author

Could you please add an unit test to cover the new configuration? Make sure the default value is false and change the value is work property.

@codelipenghui I add an unit test and doc of this configuration. Please take a look

@codelipenghui codelipenghui modified the milestones: 2.11.0, 2.12.0 Jul 26, 2022
@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions bot added the Stale label Aug 25, 2022
@tisonkun tisonkun changed the title [improve] add limitStatsLogging configuration when enable bookkeeperClientExposeStatsToPrometheus [improve][broker] add limitStatsLogging configuration when enable bookkeeperClientExposeStatsToPrometheus Dec 9, 2022
@github-actions github-actions bot added doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. and removed doc-label-missing labels Dec 9, 2022
@tisonkun tisonkun removed the Stale label Dec 9, 2022
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

LGTM.

The config document will be updated automatically since we're now generating from the code.

Signed-off-by: tison <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Dec 9, 2022

Codecov Report

Merging #16734 (02c38f5) into master (d30e86c) will decrease coverage by 8.54%.
The diff coverage is 14.28%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #16734      +/-   ##
============================================
- Coverage     47.92%   39.38%   -8.55%     
+ Complexity    10679     8260    -2419     
============================================
  Files           703      703              
  Lines         68831    68838       +7     
  Branches       7378     7379       +1     
============================================
- Hits          32988    27111    -5877     
- Misses        32119    38304    +6185     
+ Partials       3724     3423     -301     
Flag Coverage Δ
unittests 39.38% <14.28%> (-8.55%) ⬇️

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

Impacted Files Coverage Δ
...er/loadbalance/impl/PulsarResourceDescription.java 11.11% <0.00%> (-1.71%) ⬇️
...che/pulsar/broker/BookKeeperClientFactoryImpl.java 49.53% <100.00%> (+49.53%) ⬆️
...ain/java/org/apache/pulsar/broker/rest/Topics.java 0.00% <0.00%> (-100.00%) ⬇️
...ava/org/apache/pulsar/broker/admin/v1/Brokers.java 0.00% <0.00%> (-100.00%) ⬇️
...va/org/apache/pulsar/broker/admin/v1/Clusters.java 0.00% <0.00%> (-100.00%) ⬇️
.../org/apache/pulsar/broker/admin/v1/Properties.java 0.00% <0.00%> (-100.00%) ⬇️
...ar/common/naming/PartitionedManagedLedgerInfo.java 0.00% <0.00%> (-100.00%) ⬇️
...naming/RangeEquallyDivideBundleSplitAlgorithm.java 0.00% <0.00%> (-100.00%) ⬇️
...e/pulsar/broker/admin/impl/ResourceQuotasBase.java 0.00% <0.00%> (-96.43%) ⬇️
...he/pulsar/broker/service/AnalyzeBacklogResult.java 0.00% <0.00%> (-92.31%) ⬇️
... and 187 more

Copy link
Member

@zymap zymap left a comment

Choose a reason for hiding this comment

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

LGTM

@tisonkun
Copy link
Member

Merging...

@tisonkun tisonkun merged commit 71bf3b2 into apache:master Dec 10, 2022
Demogorgon314 pushed a commit to Demogorgon314/pulsar that referenced this pull request Dec 26, 2022
…kkeeperClientExposeStatsToPrometheus (apache#16734)

Signed-off-by: tison <[email protected]>
Co-authored-by: fanjianye <[email protected]>
Co-authored-by: tison <[email protected]>
Demogorgon314 pushed a commit to Demogorgon314/pulsar that referenced this pull request Dec 29, 2022
…kkeeperClientExposeStatsToPrometheus (apache#16734)

Signed-off-by: tison <[email protected]>
Co-authored-by: fanjianye <[email protected]>
Co-authored-by: tison <[email protected]>
lifepuzzlefun pushed a commit to lifepuzzlefun/pulsar that referenced this pull request Jan 10, 2023
…kkeeperClientExposeStatsToPrometheus (apache#16734)

Signed-off-by: tison <[email protected]>
Co-authored-by: fanjianye <[email protected]>
Co-authored-by: tison <[email protected]>
@momo-jun momo-jun changed the title [improve][broker] add limitStatsLogging configuration when enable bookkeeperClientExposeStatsToPrometheus [improve][broker] add limitStatsLogging configuration when enabling bookkeeperClientExposeStatsToPrometheus Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/metrics doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enhance enable bookkeeperClientExposeStatsToPrometheus would drop pulsar performance
5 participants