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

Implement consumer part of rocketmq new client instrumentation #7019

Conversation

aaron-ai
Copy link
Member

@aaron-ai aaron-ai commented Nov 1, 2022

Fixes #6764 , This PR is about the consumer part.

@aaron-ai aaron-ai requested a review from a team November 1, 2022 08:48
@aaron-ai aaron-ai force-pushed the feature/support_rocketmq_consumer_instrumentation branch 2 times, most recently from 4550348 to 82d1cc3 Compare November 1, 2022 09:04
@aaron-ai
Copy link
Member Author

aaron-ai commented Nov 7, 2022

@laurit , I noticed this issue #6804.

It seems that the message consumption procedure of RocketMQ is more similar to Kafka rather than JMS, which means the process span is always there and the receive span is just an addition.

So I add the message receiving instrumentation flag and keep it consistent with Kafka.

@aaron-ai aaron-ai requested a review from laurit November 7, 2022 07:32
@aaron-ai aaron-ai force-pushed the feature/support_rocketmq_consumer_instrumentation branch from 43bb101 to 71ce4c0 Compare November 9, 2022 07:16
@aaron-ai aaron-ai requested review from trask and removed request for laurit November 9, 2022 08:10
@aaron-ai aaron-ai force-pushed the feature/support_rocketmq_consumer_instrumentation branch from 71ce4c0 to 59ab0f3 Compare November 10, 2022 02:04
@aaron-ai
Copy link
Member Author

@trask @laurit Any more feedback?

@mateuszrzeszutek
Copy link
Member

Hey @aaron-ai ,
I left a couple style/convention related comments - overall the instrumentation looks solid, thanks!

@aaron-ai aaron-ai force-pushed the feature/support_rocketmq_consumer_instrumentation branch 2 times, most recently from c5012d4 to 75ae828 Compare November 14, 2022 14:58
@aaron-ai aaron-ai force-pushed the feature/support_rocketmq_consumer_instrumentation branch from 75ae828 to 96b297b Compare November 14, 2022 15:10
@aaron-ai
Copy link
Member Author

@mateuszrzeszutek thanks for your opnions, all issues were fixed already. but there were a few unpassed tests not related to this PR seemingly, could you re-run the unpassed tests?

@trask
Copy link
Member

trask commented Nov 15, 2022

@aaron-ai heads up I merged main into your branch to fix the CI failures

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

thx @aaron-ai!

Copy link
Member

@mateuszrzeszutek mateuszrzeszutek 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 @aaron-ai !

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

Successfully merging this pull request may close these issues.

Implement instrumentation for the new client of Apache RocketMQ
4 participants