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

[feat] [broker] Add broker health check status into prometheus metrics #20147

Merged
merged 1 commit into from
Oct 7, 2024

Conversation

vineeth1995
Copy link
Contributor

@vineeth1995 vineeth1995 commented Apr 20, 2023

Motivation

Issue: #20146
To add broker health check into prometheus metric.

Modifications

Schedule a job at 1 minute time interval which calls healthCheck API on broker and updates the pulsar stats based on the broker health.

Verifying this change

Unit test cases were added to verify this change.

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

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • Anything that affects deployment

Documentation

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

@github-actions
Copy link

@vineeth1995 Please select only one documentation label in your PR description.

@github-actions github-actions bot added the doc-complete Your PR changes impact docs and the related docs have been already added. label Apr 20, 2023
@vineeth1995
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@codecov-commenter
Copy link

codecov-commenter commented Apr 20, 2023

Codecov Report

Attention: Patch coverage is 76.31579% with 9 lines in your changes missing coverage. Please review.

Project coverage is 74.51%. Comparing base (bbc6224) to head (84c4a3e).
Report is 636 commits behind head on master.

Files with missing lines Patch % Lines
...g/apache/pulsar/broker/admin/impl/BrokersBase.java 68.75% 5 Missing ⚠️
...rg/apache/pulsar/broker/service/BrokerService.java 81.81% 2 Missing ⚠️
.../pulsar/broker/stats/BrokerOperabilityMetrics.java 77.77% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #20147      +/-   ##
============================================
+ Coverage     73.57%   74.51%   +0.94%     
- Complexity    32624    34551    +1927     
============================================
  Files          1877     1937      +60     
  Lines        139502   145459    +5957     
  Branches      15299    15898     +599     
============================================
+ Hits         102638   108395    +5757     
+ Misses        28908    28743     -165     
- Partials       7956     8321     +365     
Flag Coverage Δ
inttests 27.47% <47.36%> (+2.89%) ⬆️
systests 24.48% <26.31%> (+0.15%) ⬆️
unittests 73.87% <76.31%> (+1.03%) ⬆️

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

Files with missing lines Coverage Δ
...org/apache/pulsar/broker/ServiceConfiguration.java 98.97% <100.00%> (-0.42%) ⬇️
...rg/apache/pulsar/broker/service/BrokerService.java 83.58% <81.81%> (+2.80%) ⬆️
.../pulsar/broker/stats/BrokerOperabilityMetrics.java 93.61% <77.77%> (-4.78%) ⬇️
...g/apache/pulsar/broker/admin/impl/BrokersBase.java 74.07% <68.75%> (+1.61%) ⬆️

... and 610 files with indirect coverage changes

Copy link
Contributor

@rdhabalia rdhabalia left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@mattisonchao mattisonchao left a comment

Choose a reason for hiding this comment

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

Hi, @vineeth1995

If I remember correctly, the new configuration should draft a proposal.

Refer to: https://github.com/apache/pulsar/blob/master/wiki/proposals/PIP.md

@rdhabalia
Copy link
Contributor

@mattisonchao
I don't think and I also don't see draft requirements for configuration-related changes at : https://github.com/apache/pulsar/blob/master/wiki/proposals/PIP.md
also, this PR seems added an optional config to enable certain metrics which doesn't have any impact on current behavior as well.

@vineeth1995 vineeth1995 force-pushed the healthcheck branch 2 times, most recently from f74cf57 to 8216576 Compare April 24, 2023 20:19
@vineeth1995
Copy link
Contributor Author

@mattisonchao I have addressed the changes. Can you please unblock the PR?

@mattisonchao
Copy link
Member

Hi, @rdhabalia

image

@rdhabalia
Copy link
Contributor

Sure.

If I remember correctly, the new configuration should draft a proposal.

@mattisonchao but this PR was blocked with reason of configuration change so, I mentioned that it was not part of PIP doc.

@mattisonchao
Copy link
Member

Ohh, I see. Thanks!

@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 May 29, 2023
@Technoboy- Technoboy- added this to the 3.2.0 milestone Jul 31, 2023
@Technoboy- Technoboy- modified the milestones: 3.2.0, 3.3.0 Dec 22, 2023
@coderzc coderzc modified the milestones: 3.3.0, 3.4.0 May 8, 2024
@rdhabalia
Copy link
Contributor

PIP has been approved: #20389
@vineeth1995 can you please address the changes and rebase it.

@vineeth1995 vineeth1995 force-pushed the healthcheck branch 3 times, most recently from 50bb381 to 29f77e9 Compare October 4, 2024 23:13
@rdhabalia rdhabalia dismissed mattisonchao’s stale review October 4, 2024 23:28

PIP is already approved,

@rdhabalia rdhabalia merged commit 6c7ec4c into apache:master Oct 7, 2024
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-complete Your PR changes impact docs and the related docs have been already added. ready-to-test Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants