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

Added message retries and acquire timer metrics #1841

Merged
merged 4 commits into from
Apr 8, 2024

Conversation

szczygiel-m
Copy link
Contributor

@szczygiel-m szczygiel-m commented Apr 3, 2024

What this PR contains:

  1. subscriber latency metric indicates only the request itself, previously it also included blocking acquire on rate-limiter
  2. a separate metric has been added for measuring acquire time on rate-limiter
  3. added a metric for the number of retried messages on a subscription
  4. Removed unused metrics in the metrics panel on topic and subscription view, instead added the number of repeated messages and subscriber response codes

@szczygiel-m szczygiel-m marked this pull request as ready for review April 4, 2024 11:19
Diewa
Diewa previously approved these changes Apr 5, 2024
Copy link
Member

@Diewa Diewa left a comment

Choose a reason for hiding this comment

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

nice PR! 🎉

@@ -131,6 +136,9 @@ private int calculateMessageDelay(long publishingMessageTimestamp) {
*/
private void sendMessage(final Message message) {
loadRecorder.recordSingleOperation();
HermesTimerContext acquireTimer = rateLimiterAcquireTimer.time();
Copy link
Member

Choose a reason for hiding this comment

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

nit: maybe it should be moved to a separate method. What do you think?

private void acquireRateLimiter() {
  HermesTimerContext acquireTimer = rateLimiterAcquireTimer.time();
  rateLimiter.acquire();
  acquireTimer.close();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to a separate method

@@ -131,6 +136,9 @@ private int calculateMessageDelay(long publishingMessageTimestamp) {
*/
private void sendMessage(final Message message) {
loadRecorder.recordSingleOperation();
HermesTimerContext acquireTimer = rateLimiterAcquireTimer.time();
rateLimiter.acquire();
Copy link
Member

Choose a reason for hiding this comment

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

question: Does only ConsumerMessageSender use the send method from ResilientMessageSender?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually ConsumerMessageSender use the send method from JettyBroadCastMessageSender or SingleRecipientMessageSenderAdapter depends on MessageSender implementation provided by factory. True is, that those two classes which i have mentioned are the only usages of ResilientMessageSender.send()

piotrrzysko
piotrrzysko previously approved these changes Apr 5, 2024
@szczygiel-m szczygiel-m dismissed stale reviews from piotrrzysko and Diewa via 6f19099 April 5, 2024 14:39
@szczygiel-m szczygiel-m merged commit a58f62e into master Apr 8, 2024
27 checks passed
@szczygiel-m szczygiel-m deleted the improve-admins-observability branch April 8, 2024 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants