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.

Sign up for GitHub

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

Jump to bottom

Rabbitmq instrument consumer class correctly to get duration reported… #3761

Merged
merged 4 commits into from
Nov 16, 2019
Merged

Rabbitmq instrument consumer class correctly to get duration reported… #3761

merged 4 commits into from
Nov 16, 2019

Conversation

viswaramamoorthy
Copy link
Contributor

… accurately

Please answer these questions before submitting pull request

  • Why submit this pull request?

  • Bug fix

  • New feature provided

  • Improve performance

  • Related issues

Bug fix

  • Bug description.
    RabbitMQ consumer duration is reported incorrectly due to instrumenting com.rabbitmq.client.impl.ConsumerDispatcher.handleDelivery method which spins up a thread to deliver message to consuming method

  • How to fix?
    Instrumenting implementations of com.rabbitmq.client.Consumer to resolve

New feature or improvement

  • Describe the details and related test reports.

@viswaramamoorthy viswaramamoorthy mentioned this pull request Oct 31, 2019
Closed
4 tasks
@wu-sheng
Copy link
Member

wu-sheng commented Nov 1, 2019

Your test should be added through jenkins file. Find the fastest workload and group, add your new case there. And make sure test passed.

@wu-sheng wu-sheng added agent Language agent related. bug Something isn't working and you are sure it's a bug! plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor. labels Nov 1, 2019
@dmsolr
Copy link
Member

dmsolr commented Nov 1, 2019

After that, please add rabbitmq-scenario task into the Jenkinsfile.

@viswaramamoorthy
Copy link
Contributor Author

@dmsolr @wu-sheng I have answered why we should make changes to core api. once you review and get back, i can make changes to test case suggestions to get ready for final merge. let me know

@viswaramamoorthy viswaramamoorthy force-pushed the 3753-rabbitmq-fix branch 2 times, most recently from 88b1f6f to 44e78fa Compare November 4, 2019 21:45
@wu-sheng
Copy link
Member

wu-sheng commented Nov 5, 2019

@viswaramamoorthy Have you checked? Is the way I proposed working for your inherit scenario?

@dmsolr
Copy link
Member

dmsolr commented Nov 5, 2019

@viswaramamoorthy Hi, Please rename the package of testcase. And then add it as a Jenkinsfile task to run.

@wu-sheng
Copy link
Member

wu-sheng commented Nov 5, 2019

@dmsolr Which workload and group should be added? Or should we start workload 4 for now? @kezhenxu94

@wu-sheng
Copy link
Member

wu-sheng commented Nov 5, 2019

The plugin test framework doc is ready, #3786. Please take a look.

@viswaramamoorthy After you confirm the latest codes are working for all your cases.
You could move the RabbitMQ test case in another PR. After that is merged, then we recheck this.

@kezhenxu94
Copy link
Member

@dmsolr Which workload and group should be added? Or should we start workload 4 for now? @kezhenxu94

Workload4 has been set up, use file name Jenkinsfile-Agent-Test-4 and ping me, and I'll enable this job,

@dmsolr
Copy link
Member

dmsolr commented Nov 5, 2019

@kezhenxu94 Hi, I pushed the Jenkinsfile on #3787 . Please review and enable it.

@viswaramamoorthy
Copy link
Contributor Author

Changes in this PR are verified and works for the bug reported.

I have raised a separate PR for RabbitMQ e2e test case migration with package renamed. it is #3788

@wu-sheng
Copy link
Member

@viswaramamoorthy The test case has been merged, let's work on this one.

@wu-sheng wu-sheng added this to the 6.6.0 milestone Nov 16, 2019
dmsolr
dmsolr approved these changes Nov 16, 2019
View reviewed changes
Copy link
Member

@dmsolr dmsolr left a comment

Choose a reason for hiding this comment

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

It is good for me. Note that it uses HierarchyMatch

wu-sheng
wu-sheng approved these changes Nov 16, 2019
View reviewed changes
Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

LGTM. com.rabbitmq.client.Consumer HierarchyMatch should be safe from my perspective.

@wu-sheng wu-sheng merged commit 77019ba into apache:master Nov 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wu-sheng wu-sheng wu-sheng approved these changes

@dmsolr dmsolr dmsolr approved these changes

@ascrutae ascrutae Awaiting requested review from ascrutae

@kezhenxu94 kezhenxu94 Awaiting requested review from kezhenxu94

Assignees
No one assigned
Labels
agent Language agent related. bug Something isn't working and you are sure it's a bug! plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor.
Projects
None yet
Milestone
6.6.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@viswaramamoorthy @wu-sheng @dmsolr @kezhenxu94 @arugal