-
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] PIP-307: Add monitoring metrics for graceful closure of producers/consumers #21854
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
heesung-sn
reviewed
Jan 4, 2024
...src/main/java/org/apache/pulsar/broker/loadbalance/extensions/ExtensibleLoadManagerImpl.java
Outdated
Show resolved
Hide resolved
...ker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/manager/UnloadManager.java
Show resolved
Hide resolved
...ker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/manager/UnloadManager.java
Outdated
Show resolved
Hide resolved
...ker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/manager/UnloadManager.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/pulsar/broker/loadbalance/extensions/ExtensibleLoadManagerImpl.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/stats/PrometheusMetricsTest.java
Outdated
Show resolved
Hide resolved
...ker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/manager/UnloadManager.java
Outdated
Show resolved
Hide resolved
heesung-sn
reviewed
Jan 4, 2024
...ker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/manager/UnloadManager.java
Show resolved
Hide resolved
heesung-sn
reviewed
Jan 5, 2024
...ker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/manager/UnloadManager.java
Outdated
Show resolved
Hide resolved
...ker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/manager/UnloadManager.java
Outdated
Show resolved
Hide resolved
...ava/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannelImpl.java
Outdated
Show resolved
Hide resolved
...ava/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannelImpl.java
Outdated
Show resolved
Hide resolved
.../main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/StateChangeListeners.java
Outdated
Show resolved
Hide resolved
heesung-sn
approved these changes
Jan 5, 2024
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #21854 +/- ##
============================================
- Coverage 73.59% 73.58% -0.02%
- Complexity 32323 32336 +13
============================================
Files 1858 1859 +1
Lines 138174 138273 +99
Branches 15148 15155 +7
============================================
+ Hits 101696 101751 +55
- Misses 28608 28643 +35
- Partials 7870 7879 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
|
...ker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/manager/UnloadManager.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java
Show resolved
Hide resolved
Demogorgon314
approved these changes
Jan 9, 2024
merlimat
approved these changes
Jan 10, 2024
Hi @heesung-sn we're now releasing 3.2, so all the new features need to label with milestone-3.3 |
I see. Thanks for the correction. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PIP: PIP-307
Motivation
This PR adds metrics to monitor the broker behavior during the graceful closure of producers and consumers.
Modifications
These new broker level metrics are being proposed:
brk_lb_unload_latency
: exposes a histogram of total time spent (in milliseconds) unloading a topic (from stateRELEASE
to stateFREE
orOWNED
) on the source brokerbrk_lb_release_latency
: exposes a histogram of milliseconds spent in theRELEASE
state on the source brokerbrk_lb_assign_latency
: exposes a histogram of milliseconds spent in theASSIGN
state on the destination brokerbrk_lb_disconnect_latency
: exposes a histogram of milliseconds spent in the disconnected state on the source brokerbrk_lb_ignored_ack_total
: type gauge, exposes the total number of message ACKs ignored from consumers during topic unloading on the source brokerbrk_lb_ignored_send_total
: type gauge, exposes the total number of ignored messages sent by producers during topic unloading on the source brokerFor the histogram metrics, the buckets are
1, 10, 100, 200, 1000
milliseconds.A sample reading of these metrics from the
/metrics/
endpoint follows. Note that gauge metrics have their prefixes changed frombrk_
topulsar_
here; for consistency, let's stick tobrk_
for now, even though this may change soon.Verifying this change
This change added tests and can be verified as follows:
org.apache.pulsar.broker.loadbalance.extensions.ExtensibleLoadManagerImplTest#testGetMetrics
to validate ignored (ack/send) count metric valuesorg.apache.pulsar.broker.stats.PrometheusMetricsTest#testBundlesMetrics
to validate the existence of new latency metricsManually verified that the metrics exposed are correct (see sample reading above) using a k8s deployment of Pulsar.
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
doc
doc-required
doc-not-needed
doc-complete
Matching PR in forked repository
PR in forked repository: dragosvictor#4