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

Use TopicPartition.topic for metrics #235

Merged
merged 3 commits into from
May 17, 2024
Merged

Conversation

tadashiya
Copy link
Contributor

  • I noticed that the metrics always use scope.topic() after Add virtual threads support #224.
  • I think it's useful to keep metrics separate between normal topic and retry topic. This change will use scope.topicPartition().topic() to use the retry topic name for metrics when handling the retry topic.

@CLAassistant
Copy link

CLAassistant commented May 16, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@kawamuray kawamuray left a comment

Choose a reason for hiding this comment

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

Oh very nice catch...! I overlooked that these two topic references may refer different topics in case of retry and indeed it should refer to the currently consuming topic rather than the origin subscription topic.

@@ -59,7 +59,7 @@ protected AbstractSubPartitions(PartitionScope scope, Processors<?> processors)
rateLimiter = new DynamicRateLimiter(rateProperty(scope));
Metrics metrics = Metrics.withTags(
"subscription", scope.subscriptionId(),
"topic", scope.topic(),
"topic", scope.topicPartition().topic(),
Copy link
Member

Choose a reason for hiding this comment

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

in addition to these changes I wanna make the following changes to ensure we won't repeat the same mistake in future. Are you happy to follow these up in this PR? Otherwise I'll cover that after merging this PR.

  • Let's use variable to assign scope.topicPartition() return and use it instead to avoid unnecessary object allocation.
  • Rename SubscriptionScope.topic to be SubscriptionScope.originTopic to distinguish it w/ PartitionScope.topicPartition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your review. I see, I'll update them as well.

@tadashiya tadashiya requested a review from kawamuray May 16, 2024 09:51
Copy link
Member

@kawamuray kawamuray left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@kawamuray kawamuray merged commit 91fde46 into line:master May 17, 2024
5 checks passed
@kawamuray
Copy link
Member

This has been released as 8.0.1. Thanks for your contribution :) https://github.com/line/decaton/releases/tag/v8.0.1

@tadashiya tadashiya deleted the topic-name branch May 17, 2024 07:29
@tadashiya
Copy link
Contributor Author

OK, thanks for your help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fix a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants