Skip to content

Spring @Async plugin support #2902

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
Merged
wu-sheng merged 24 commits into from
Jul 1, 2019
Merged

Spring @Async plugin support #2902

wu-sheng merged 24 commits into from
Jul 1, 2019

Conversation

@zhaoyuguang
Copy link
Member

@zhaoyuguang zhaoyuguang commented Jun 18, 2019

Please answer these questions before submitting pull request

@zhaoyuguang zhaoyuguang added agent Language agent related. feature New feature plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor. TBD To be decided later, need more discussion or input. labels Jun 18, 2019
@wu-sheng
Copy link
Member

wu-sheng commented Jun 18, 2019

Integration test cases are required.

wu-sheng
wu-sheng reviewed Jun 18, 2019
View reviewed changes
apm-sniffer/apm-sdk-plugin/spring-plugins/async-annotation-5.x-plugin/pom.xml Outdated
<artifactId>spring-aop</artifactId>
<version>${spring-aop.version}</version>
</dependency>
</dependencies>
Copy link
Member

@wu-sheng wu-sheng Jun 18, 2019

Choose a reason for hiding this comment

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

The pom format looks like very strange.

Copy link
Member Author

@zhaoyuguang zhaoyuguang Jun 19, 2019

Choose a reason for hiding this comment

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

Ok

@zhaoyuguang
Copy link
Member Author

zhaoyuguang commented Jun 19, 2019

Integration test cases are required.

Ok, I will support it soon

@apache apache deleted a comment from asfgit Jun 23, 2019
@apache apache deleted a comment from asfgit Jun 23, 2019
@asfgit
Copy link

asfgit commented Jun 24, 2019

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/skywalking-ci-it/64/

@zhaoyuguang zhaoyuguang mentioned this pull request Jun 24, 2019
Merged
@zhaoyuguang
Copy link
Member Author

zhaoyuguang commented Jun 25, 2019

@IanCao Please do testcase

IanCao
IanCao reviewed Jun 25, 2019
View reviewed changes
Copy link
Contributor

@IanCao IanCao left a comment

Choose a reason for hiding this comment

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

inline

@SkyWalkingRobot
Copy link

SkyWalkingRobot commented Jun 25, 2019

Here is the test report and validate logs

@SkyWalkingRobot
Copy link

SkyWalkingRobot commented Jun 25, 2019

Here is the test report and validate logs

@zhaoyuguang
Copy link
Member Author

zhaoyuguang commented Jul 1, 2019

@zhaoyuguang @IanCao What is the status of this PR?

I have completed the integration test and it looks good, is it possible to do CodeReview?

@kezhenxu94 kezhenxu94 self-requested a review July 1, 2019 11:25
@wu-sheng wu-sheng added this to the 6.3.0 milestone Jul 1, 2019
@wu-sheng
Copy link
Member

wu-sheng commented Jul 1, 2019 •
edited
Loading

@IanCao Have you finished your review?

kezhenxu94
kezhenxu94 previously approved these changes Jul 1, 2019
View reviewed changes
Copy link
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

lgtm, see others' opinions

IanCao
IanCao reviewed Jul 1, 2019
View reviewed changes
Copy link
Contributor

@IanCao IanCao left a comment

Choose a reason for hiding this comment

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

inline

IanCao
IanCao reviewed Jul 1, 2019
View reviewed changes
...nnotation-plugin/src/main/java/org/apache/skywalking/apm/plugin/spring/async/SWCallable.java Outdated
*/
public class SWCallable<V> implements Callable<V> {

private static String OPERATION_NAME = "SpringAsync";
Copy link
Contributor

@IanCao IanCao Jul 1, 2019

Choose a reason for hiding this comment

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

make it final is better

Copy link
Member Author

@zhaoyuguang zhaoyuguang Jul 1, 2019

Choose a reason for hiding this comment

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

ok

IanCao
IanCao previously approved these changes Jul 1, 2019
View reviewed changes
Copy link
Contributor

@IanCao IanCao left a comment •
edited
Loading

Choose a reason for hiding this comment

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

others code lgtm

@zhaoyuguang zhaoyuguang dismissed stale reviews from IanCao and kezhenxu94 via 23ceb63 July 1, 2019 11:54
IanCao
IanCao previously approved these changes Jul 1, 2019
View reviewed changes
wu-sheng
wu-sheng reviewed Jul 1, 2019
View reviewed changes
docs/en/setup/service-agent/java-agent/Supported-list.md Outdated
* Vert.x Ecosystem
* Vert.x Eventbus 3.2+
* Vert.x Web 3.x
* Multi-threaded Framework
Copy link
Member

@wu-sheng wu-sheng Jul 1, 2019

Choose a reason for hiding this comment

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

Thread Schedule Framework

I think we could add more threadpool plugin in this catalog.

Copy link
Member Author

@zhaoyuguang zhaoyuguang Jul 1, 2019

Choose a reason for hiding this comment

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

ok

wu-sheng
wu-sheng approved these changes Jul 1, 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

@wu-sheng
Copy link
Member

wu-sheng commented Jul 1, 2019 •
edited
Loading

This plugin breaks e2e?

@wu-sheng
Copy link
Member

wu-sheng commented Jul 1, 2019

@kezhenxu94 Please help on the first e2e check failure. :) Interesting

@wu-sheng
Copy link
Member

wu-sheng commented Jul 1, 2019

I click the re-run to see what happens.

@wu-sheng wu-sheng closed this Jul 1, 2019
@wu-sheng wu-sheng reopened this Jul 1, 2019
@wu-sheng
Copy link
Member

wu-sheng commented Jul 1, 2019

@asfgit rerun the e2e tests please

@wu-sheng
Copy link
Member

wu-sheng commented Jul 1, 2019

/run e2e

@wu-sheng wu-sheng merged commit 48ffd09 into apache:master Jul 1, 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

@kezhenxu94 kezhenxu94 kezhenxu94 left review comments

@IanCao IanCao IanCao left review comments

Assignees

No one assigned

Labels

agent Language agent related. feature New feature plugin Plugin for agent or collector. Be used to extend the capabilities of default implementor.

Projects

None yet

Milestone

6.3.0

Development

Successfully merging this pull request may close these issues.

6 participants

@zhaoyuguang @wu-sheng @asfgit @SkyWalkingRobot @kezhenxu94 @IanCao