-
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 msgInReplay subscription stat and metric to improve Key_Shared observability #23224
[improve][broker] Add msgInReplay subscription stat and metric to improve Key_Shared observability #23224
Conversation
…rove Key_Shared observability
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #23224 +/- ##
============================================
+ Coverage 73.57% 74.54% +0.96%
- Complexity 32624 34226 +1602
============================================
Files 1877 1922 +45
Lines 139502 144768 +5266
Branches 15299 15832 +533
============================================
+ Hits 102638 107916 +5278
+ Misses 28908 28578 -330
- Partials 7956 8274 +318
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.
Could you add a test?
LGTM, but maybe the change needs a PIP? |
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.
+1 for having a PIP for this change since it actually changed the Public API output and metrics.
Another point is can we only have new the field in the topic stats? Since we have a lot of metrics on the topic level. Finally, we can't add everything to the prometheus metrics. The stats, the logs are also the part of observability.
@codelipenghui This is not a breaking change in metrics and this change isn't adding significantly more metrics than what currently exists. It's currently a gap in observability of all released versions of Pulsar when msgInReplay is missing. That's why everyone needs this and making it configurable with a feature flag wouldn't be sensible. |
…rove Key_Shared observability (apache#23224)
…rove Key_Shared observability (apache#23224) (cherry picked from commit 59424a8) (cherry picked from commit 766d2a4)
…rove Key_Shared observability (apache#23224) (cherry picked from commit 59424a8) (cherry picked from commit 766d2a4)
Main Issue: #23205
Motivation
Currently there aren't ways to observe the number of messages that are "in replay" with Key_Shared or Shared subscriptions. Having these metrics would make it easier to understand the behavior of issues such as #23200 .
Please check #23205 for more reasons.
Modifications
msgInReplay
in the similar way as the existingmsgDelayed
subscription stat and metric.Documentation
doc
doc-required
doc-not-needed
doc-complete