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

Add an agent plugin to support tomcat10.x(#7420) #286

Merged
merged 39 commits into from
Jul 23, 2022
Merged

Conversation

yswdqz
Copy link
Member

@yswdqz yswdqz commented Jul 21, 2022

Add an agent plugin to support tomcat10.x

@wu-sheng wu-sheng self-requested a review July 21, 2022 05:33
@yswdqz
Copy link
Member Author

yswdqz commented Jul 21, 2022

I couldn't find a more suitable way to configure witness class, most of the changes in tomcat10 are about javax to jakarta. So I can only think of a combination of jakarta.servlet.http.HttpServletResponse, org.apache.catalina.core.StandardHostValve to configure witness classes.
If it is OK, I am ready to open CI.

@wu-sheng
Copy link
Member

javax and jakarta are good points.

@yswdqz
Copy link
Member Author

yswdqz commented Jul 21, 2022

I don't know why some checks were not successful. Please give me some advice.

public class CaseServlet extends HttpServlet {

@Override
protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
Copy link
Member

Choose a reason for hiding this comment

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

You should add an HTTP client here to call the doPost method. Then we could check whether you handle the context propagation correctly in the plugin.

Copy link
Member Author

Choose a reason for hiding this comment

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

How can I change this? I didn't understand.

Copy link
Member

Choose a reason for hiding this comment

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

You need to change the case to
Tomcat(get) -> A HTTP client call -> Tomcat(post)

Then you would have two segments, with one reference created by post because its peer client gets traced too.

@wu-sheng
Copy link
Member

I don't know why some checks were not successful. Please give me some advice.

I think that is just a stable CI's fault. It is not relative to your codes. I just rerun and passed.

@wu-sheng wu-sheng added this to the 8.12.0 milestone Jul 22, 2022
@wu-sheng wu-sheng requested a review from kezhenxu94 July 22, 2022 00:45
kezhenxu94
kezhenxu94 previously approved these changes Jul 22, 2022
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.

I can confirm the image is with desired tomcat version 10.2.22

apm-sniffer/apm-sdk-plugin/pom.xml Outdated Show resolved Hide resolved
Co-authored-by: kezhenxu94 <kezhenxu94@apache.org>
@yswdqz
Copy link
Member Author

yswdqz commented Jul 22, 2022

@kezhenxu94 The approved was dismissed, maybe need a new approve

kezhenxu94
kezhenxu94 previously approved these changes Jul 22, 2022
@wu-sheng
Copy link
Member

@kezhenxu94 The approved was dismissed, maybe need a new approve

Don't worry. Every time, you commit a new one, it would dismiss the previous approval.
Once you resolve all comments(change or don't need to change), the maintainer would approve again.
And we can see other reviewers had approved before, like this,
image

@@ -34,6 +34,7 @@
<module>jedis-2.x-plugin</module>
<module>redisson-3.x-plugin</module>
<module>tomcat-7.x-8.x-plugin</module>
<module>tomcat-10x-plugin</module>
Copy link
Contributor

Choose a reason for hiding this comment

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

There are 2 tabs here, which should be indented with 4 spaces

@yswdqz
Copy link
Member Author

yswdqz commented Jul 22, 2022

It seems that there are still some problems in my expectedData.yaml. I will fix them tomorrow morning.

@wu-sheng wu-sheng merged commit 129f57c into apache:main Jul 23, 2022
private static final String GET_STATUS_METHOD = "getStatus";

static {
IS_SERVLET_GET_STATUS_METHOD_EXIST = MethodUtil.isMethodExist(
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to check this method for Tomcat 10. According to the spec, getStutus is introduced from servlet API 3.0. But the jakarta migration happened from 4.0.2.

I cannot see the possibility that this method is missing here. @kezhenxu94 @yswdqz @wu-sheng

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think it is safe to remove this.

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

Successfully merging this pull request may close these issues.

5 participants