-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[improve][broker] add limitStatsLogging configuration when enabling bookkeeperClientExposeStatsToPrometheus #16734
Conversation
…nnel_bookie_client metrics of bkclient stats
There was a problem hiding this 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.
@TakaHiR07 Please provide a correct documentation label for your PR. |
@codelipenghui I add an unit test and doc of this configuration. Please take a look |
The pr had no activity for 30 days, mark with Stale label. |
There was a problem hiding this 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 Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Merging... |
…kkeeperClientExposeStatsToPrometheus (apache#16734) Signed-off-by: tison <[email protected]> Co-authored-by: fanjianye <[email protected]> Co-authored-by: tison <[email protected]>
…kkeeperClientExposeStatsToPrometheus (apache#16734) Signed-off-by: tison <[email protected]> Co-authored-by: fanjianye <[email protected]> Co-authored-by: tison <[email protected]>
…kkeeperClientExposeStatsToPrometheus (apache#16734) Signed-off-by: tison <[email protected]> Co-authored-by: fanjianye <[email protected]> Co-authored-by: tison <[email protected]>
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.
Does this pull request potentially affect one of the following parts:
If
yes
was chosen, please highlight the changesDocumentation
Check the box below or label this PR directly.
Need to update docs?
doc