You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2022/07/21 05:29:59 UTC
[GitHub] [skywalking-java] yswdqz opened a new pull request, #286: Add an agent plugin to support tomcat10.x(#7420)
yswdqz opened a new pull request, #286:
URL: https://github.com/apache/skywalking-java/pull/286
### Add an agent plugin to support tomcat10.x
- [x] Add a test case for the new plugin, refer to [the doc](https://github.com/apache/skywalking-java/blob/main/docs/en/setup/service-agent/java-agent/Plugin-test.md)
- [x] Add a component id in [the component-libraries.yml](https://github.com/apache/skywalking/blob/master/oap-server/server-starter/src/main/resources/component-libraries.yml)——(Already exists)
- [x] Add a logo in [the UI repo](https://github.com/apache/skywalking-rocketbot-ui/tree/master/src/views/components/topology/assets)——(Already exists)
- [x] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #7420.
- [x] Update the [`CHANGES` log](https://github.com/apache/skywalking-java/blob/main/CHANGES.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] lujiajing1126 commented on a diff in pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by "lujiajing1126 (via GitHub)" <gi...@apache.org>.
lujiajing1126 commented on code in PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#discussion_r1119753555
##########
apm-sniffer/apm-sdk-plugin/tomcat-10x-plugin/src/main/java/org/apache/skywalking/apm/plugin/tomcat10x/TomcatInvokeInterceptor.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.plugin.tomcat10x;
+
+import jakarta.servlet.http.HttpServletResponse;
+import org.apache.catalina.connector.Request;
+import org.apache.skywalking.apm.agent.core.context.CarrierItem;
+import org.apache.skywalking.apm.agent.core.context.ContextCarrier;
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.tag.Tags;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.context.trace.SpanLayer;
+
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+import org.apache.skywalking.apm.agent.core.util.CollectionUtil;
+import org.apache.skywalking.apm.agent.core.util.MethodUtil;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+import org.apache.skywalking.apm.util.StringUtil;
+import org.apache.tomcat.util.http.Parameters;
+
+import java.lang.reflect.Method;
+import java.util.Enumeration;
+import java.util.HashMap;
+import java.util.Map;
+
+public class TomcatInvokeInterceptor implements InstanceMethodsAroundInterceptor {
+
+ private static boolean IS_SERVLET_GET_STATUS_METHOD_EXIST;
+ private static final String SERVLET_RESPONSE_CLASS = "jakarta.servlet.http.HttpServletResponse";
+ private static final String GET_STATUS_METHOD = "getStatus";
+
+ static {
+ IS_SERVLET_GET_STATUS_METHOD_EXIST = MethodUtil.isMethodExist(
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#issuecomment-1191632066
`javax` and `jakarta` are good points.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#discussion_r926285123
##########
apm-sniffer/apm-sdk-plugin/tomcat-7.x-8.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/tomcat78x/define/TomcatInstrumentation.java:
##########
@@ -45,6 +45,11 @@ public class TomcatInstrumentation extends ClassInstanceMethodsEnhancePluginDefi
*/
private static final String EXCEPTION_INTERCEPT_CLASS = "org.apache.skywalking.apm.plugin.tomcat78x.TomcatExceptionInterceptor";
+ @Override
+ protected String[] witnessClasses() {
+ return new String[]{"javax.servlet.http.HttpServletResponse"};
Review Comment:
Does this only exist in Tomcat 10? This looks like a J2EE class. Why do you choose this as a witness class?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] yswdqz commented on pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#issuecomment-1191088788
Sorry,I will fix it soon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#issuecomment-1191093010
> Sorry,I will fix it soon.
Don't need to be sorry. You have been better than many, as you have finished most of this PR.
Review is a project maintainer's responsibility to help you go through all processes, just because we spent more time on this project than you. Although, you have spent more than many others.
Also, please notice, that my review could be wrong, as mostly I am relying on CI and reading codes. You are really running those codes step by step.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#discussion_r927253418
##########
test/plugin/scenarios/tomcat-10x-scenario/src/main/java/org/apache/skywalking/apm/testcase/tomcat10x/CaseServlet.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.testcase.tomcat10x;
+
+import jakarta.servlet.ServletException;
+import jakarta.servlet.http.HttpServlet;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
+
+import java.io.IOException;
+
+public class CaseServlet extends HttpServlet {
+
+ @Override
+ protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] yswdqz commented on pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#issuecomment-1191697583
I don't know why some checks were not successful. Please give me some advice.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#discussion_r926291895
##########
apm-sniffer/apm-sdk-plugin/tomcat-10x-plugin/src/main/java/org/apache/skywalking/apm/plugin/tomcat10x/define/TomcatInstrumentation.java:
##########
@@ -0,0 +1,100 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.plugin.tomcat10x.define;
+
+import net.bytebuddy.description.method.MethodDescription;
+import net.bytebuddy.matcher.ElementMatcher;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.ConstructorInterceptPoint;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.InstanceMethodsInterceptPoint;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.ClassInstanceMethodsEnhancePluginDefine;
+import org.apache.skywalking.apm.agent.core.plugin.match.ClassMatch;
+
+import static net.bytebuddy.matcher.ElementMatchers.named;
+import static org.apache.skywalking.apm.agent.core.plugin.match.NameMatch.byName;
+
+public class TomcatInstrumentation extends ClassInstanceMethodsEnhancePluginDefine {
+
+ /**
+ * Enhance class.
+ */
+ private static final String ENHANCE_CLASS = "org.apache.catalina.core.StandardHostValve";
Review Comment:
This class does also exist in Tomcat 7/8 plugin, you need witness class(es) at both sides to recognize versions.
Like I mentioned https://github.com/apache/skywalking-java/pull/286#discussion_r926285123, you may be wrong about the witness class setting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] yswdqz commented on a diff in pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#discussion_r926838084
##########
apm-sniffer/apm-sdk-plugin/tomcat-10x-plugin/src/main/java/org/apache/skywalking/apm/plugin/tomcat10x/define/ApplicationDispatcherInstrumentation.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.plugin.tomcat10x.define;
+
+import net.bytebuddy.description.method.MethodDescription;
+import net.bytebuddy.matcher.ElementMatcher;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.ConstructorInterceptPoint;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.InstanceMethodsInterceptPoint;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.ClassInstanceMethodsEnhancePluginDefine;
+import org.apache.skywalking.apm.agent.core.plugin.match.ClassMatch;
+
+import static net.bytebuddy.matcher.ElementMatchers.any;
+import static net.bytebuddy.matcher.ElementMatchers.named;
+import static org.apache.skywalking.apm.agent.core.plugin.match.NameMatch.byName;
+
+public class ApplicationDispatcherInstrumentation extends ClassInstanceMethodsEnhancePluginDefine {
+
+ private static final String ENHANCE_CLASS = "org.apache.catalina.core.ApplicationDispatcher";
+ private static final String ENHANCE_METHOD = "forward";
+ public static final String INTERCEPTOR_CLASS = "org.apache.skywalking.apm.plugin.tomcat10x.ForwardInterceptor";
+
+ @Override
+ protected String[] witnessClasses() {
+ return new String[]{"jakarta.servlet.http.HttpServletResponse", "org.apache.catalina.core.StandardHostValve"};
Review Comment:
OK, I will change it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#issuecomment-1191354154
The CI would be hold until you think this is ready.
In the future, it is better you could rebase git commit to make the message more meaningful.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] yswdqz commented on a diff in pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#discussion_r926832140
##########
apm-sniffer/apm-sdk-plugin/tomcat-10x-plugin/src/main/java/org/apache/skywalking/apm/plugin/tomcat10x/define/ApplicationDispatcherInstrumentation.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.plugin.tomcat10x.define;
+
+import net.bytebuddy.description.method.MethodDescription;
+import net.bytebuddy.matcher.ElementMatcher;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.ConstructorInterceptPoint;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.InstanceMethodsInterceptPoint;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.ClassInstanceMethodsEnhancePluginDefine;
+import org.apache.skywalking.apm.agent.core.plugin.match.ClassMatch;
+
+import static net.bytebuddy.matcher.ElementMatchers.any;
+import static net.bytebuddy.matcher.ElementMatchers.named;
+import static org.apache.skywalking.apm.agent.core.plugin.match.NameMatch.byName;
+
+public class ApplicationDispatcherInstrumentation extends ClassInstanceMethodsEnhancePluginDefine {
+
+ private static final String ENHANCE_CLASS = "org.apache.catalina.core.ApplicationDispatcher";
+ private static final String ENHANCE_METHOD = "forward";
+ public static final String INTERCEPTOR_CLASS = "org.apache.skywalking.apm.plugin.tomcat10x.ForwardInterceptor";
+
+ @Override
+ protected String[] witnessClasses() {
+ return new String[]{"jakarta.servlet.http.HttpServletResponse", "org.apache.catalina.core.StandardHostValve"};
Review Comment:
I thought this would be more efficient, by not starting when it's not need tomcat.
if it is not necessary, I will change it soon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#issuecomment-1192033289
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#discussion_r926333587
##########
apm-sniffer/apm-sdk-plugin/tomcat-10x-plugin/src/main/java/org/apache/skywalking/apm/plugin/tomcat10x/ForwardInterceptor.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ *
+ */
+
+package org.apache.skywalking.apm.plugin.tomcat10x;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceConstructorInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+import java.util.HashMap;
+import java.util.Map;
+
+public class ForwardInterceptor implements InstanceMethodsAroundInterceptor, InstanceConstructorInterceptor {
+
+ @Override
+ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes,
+ MethodInterceptResult result) throws Throwable {
+ if (ContextManager.isActive()) {
+ AbstractSpan abstractTracingSpan = ContextManager.activeSpan();
+ Map<String, String> eventMap = new HashMap<String, String>();
+ eventMap.put("forward-url", objInst.getSkyWalkingDynamicField() == null ? "" : String.valueOf(objInst.getSkyWalkingDynamicField()));
Review Comment:
Got it, thanks for the explanation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] yswdqz commented on a diff in pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#discussion_r927216140
##########
test/plugin/scenarios/tomcat-10x-scenario/src/main/java/org/apache/skywalking/apm/testcase/tomcat10x/CaseServlet.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.testcase.tomcat10x;
+
+import jakarta.servlet.ServletException;
+import jakarta.servlet.http.HttpServlet;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
+
+import java.io.IOException;
+
+public class CaseServlet extends HttpServlet {
+
+ @Override
+ protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
Review Comment:
How can I change this? I didn't understand.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#discussion_r926287915
##########
apm-sniffer/apm-sdk-plugin/tomcat-10x-plugin/src/main/java/org/apache/skywalking/apm/plugin/tomcat10x/ForwardInterceptor.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ *
+ */
+
+package org.apache.skywalking.apm.plugin.tomcat10x;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceConstructorInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+import java.util.HashMap;
+import java.util.Map;
+
+public class ForwardInterceptor implements InstanceMethodsAroundInterceptor, InstanceConstructorInterceptor {
+
+ @Override
+ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes,
+ MethodInterceptResult result) throws Throwable {
+ if (ContextManager.isActive()) {
+ AbstractSpan abstractTracingSpan = ContextManager.activeSpan();
+ Map<String, String> eventMap = new HashMap<String, String>();
+ eventMap.put("forward-url", objInst.getSkyWalkingDynamicField() == null ? "" : String.valueOf(objInst.getSkyWalkingDynamicField()));
+ abstractTracingSpan.log(System.currentTimeMillis(), eventMap);
+ ContextManager.getRuntimeContext().put(Constants.FORWARD_REQUEST_FLAG, true);
Review Comment:
I can't see any code that reads this. Only one `put` and two `remove`s? Why do we need this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#discussion_r926293591
##########
.github/workflows/plugins-tomcat10-test.0.yaml:
##########
@@ -0,0 +1,72 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+name: Test
+
+on:
+ pull_request:
+ paths:
+ - '.github/workflows/plugins-*.yaml'
+ - 'apm-application-toolkit/**'
+ - 'apm-commons/**'
+ - 'apm-protocol/**'
+ - 'apm-sniffer/**'
+ - 'test/plugin/**'
+ - '**/pom.xml'
+ - '!**.md'
+ push:
+ branches:
+ - test/ci/*
+
+concurrency:
+ group: plugins-tomcat10-${{ github.event.pull_request.number || github.ref }}
+ cancel-in-progress: true
+
+jobs:
+ build:
+ name: Build
+ runs-on: ubuntu-latest
+ timeout-minutes: 30
+ steps:
+ - uses: actions/checkout@v2
+ with:
+ submodules: true
+ - name: Build
+ uses: ./.github/actions/build
+ with:
+ base_image_tomcat: tomcat:10.0.22-jdk8
Review Comment:
@dmsolr Is this a correct way to control Tomcat base image for testing?
##########
test/plugin/scenarios/tomcat-10x-scenario/support-version.list:
##########
@@ -0,0 +1,17 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+10.0.22
Review Comment:
```suggestion
# Use `.github/workflows/plugins-tomcat10-test.0.yaml` to control Tomcat verion
# Current tested Tomcat 10.0.22
all
```
I don't think we could control the test case version through this file
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#discussion_r926289899
##########
apm-sniffer/apm-sdk-plugin/tomcat-10x-plugin/src/main/java/org/apache/skywalking/apm/plugin/tomcat10x/TomcatPluginConfig.java:
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.plugin.tomcat10x;
+
+import org.apache.skywalking.apm.agent.core.boot.PluginConfig;
+
+public class TomcatPluginConfig {
+ public static class Plugin {
+ @PluginConfig(root = TomcatPluginConfig.class)
+ public static class Tomcat {
+ /**
+ * This config item controls that whether the Tomcat plugin should collect the parameters of the request.
+ */
+ public static boolean COLLECT_HTTP_PARAMS = false;
+ }
+
+ @PluginConfig(root = TomcatPluginConfig.class)
+ public static class Http {
+ /**
+ * When either {@link Tomcat#COLLECT_HTTP_PARAMS} is enabled, how many characters to keep and send to the
+ * OAP backend, use negative values to keep and send the complete parameters, NB. this config item is added
+ * for the sake of performance
+ */
+ public static int HTTP_PARAMS_LENGTH_THRESHOLD = 1024;
Review Comment:
This setting is not declared in `agent.config` and `configurations.md`. I know it exists for Tomcat 8/0 plugin too, it is missed before. Please fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] yswdqz commented on pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#issuecomment-1191625611
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] yswdqz commented on a diff in pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#discussion_r926469128
##########
apm-sniffer/apm-sdk-plugin/tomcat-10x-plugin/src/main/java/org/apache/skywalking/apm/plugin/tomcat10x/TomcatPluginConfig.java:
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.plugin.tomcat10x;
+
+import org.apache.skywalking.apm.agent.core.boot.PluginConfig;
+
+public class TomcatPluginConfig {
+ public static class Plugin {
+ @PluginConfig(root = TomcatPluginConfig.class)
+ public static class Tomcat {
+ /**
+ * This config item controls that whether the Tomcat plugin should collect the parameters of the request.
+ */
+ public static boolean COLLECT_HTTP_PARAMS = false;
+ }
+
+ @PluginConfig(root = TomcatPluginConfig.class)
+ public static class Http {
+ /**
+ * When either {@link Tomcat#COLLECT_HTTP_PARAMS} is enabled, how many characters to keep and send to the
+ * OAP backend, use negative values to keep and send the complete parameters, NB. this config item is added
+ * for the sake of performance
+ */
+ public static int HTTP_PARAMS_LENGTH_THRESHOLD = 1024;
Review Comment:
I found this setting is already exist in
https://github.com/apache/skywalking-java/blob/main/apm-sniffer/config/agent.config#L215
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#discussion_r927178596
##########
test/plugin/scenarios/tomcat-10x-scenario/src/main/java/org/apache/skywalking/apm/testcase/tomcat10x/CaseServlet.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.testcase.tomcat10x;
+
+import jakarta.servlet.ServletException;
+import jakarta.servlet.http.HttpServlet;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
+
+import java.io.IOException;
+
+public class CaseServlet extends HttpServlet {
+
+ @Override
+ protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#discussion_r927202380
##########
.github/workflows/plugins-tomcat10-test.0.yaml:
##########
@@ -0,0 +1,72 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+name: Test
+
+on:
+ pull_request:
+ paths:
+ - '.github/workflows/plugins-*.yaml'
+ - 'apm-application-toolkit/**'
+ - 'apm-commons/**'
+ - 'apm-protocol/**'
+ - 'apm-sniffer/**'
+ - 'test/plugin/**'
+ - '**/pom.xml'
+ - '!**.md'
+ push:
+ branches:
+ - test/ci/*
+
+concurrency:
+ group: plugins-tomcat10-${{ github.event.pull_request.number || github.ref }}
+ cancel-in-progress: true
+
+jobs:
+ build:
+ name: Build
+ runs-on: ubuntu-latest
+ timeout-minutes: 30
+ steps:
+ - uses: actions/checkout@v2
+ with:
+ submodules: true
+ - name: Build
+ uses: ./.github/actions/build
+ with:
+ base_image_tomcat: tomcat:10.0.22-jdk8
Review Comment:
@kezhenxu94 Please help on confirming, the image is correct.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#issuecomment-1192209718
> @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,
<img width="531" alt="image" src="https://user-images.githubusercontent.com/5441976/180372337-3a7fc6fc-e0ea-4359-9f76-7d2777d8cb74.png">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#discussion_r926831963
##########
apm-sniffer/apm-sdk-plugin/tomcat-7.x-8.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/tomcat78x/define/ApplicationDispatcherInstrumentation.java:
##########
@@ -35,6 +39,16 @@ public class ApplicationDispatcherInstrumentation extends ClassInstanceMethodsEn
private static final String ENHANCE_METHOD = "forward";
public static final String INTERCEPTOR_CLASS = "org.apache.skywalking.apm.plugin.tomcat78x.ForwardInterceptor";
+ @Override
+ protected String[] witnessClasses() {
+ return new String[]{"org.apache.catalina.core.StandardHostValve"};
Review Comment:
I think you should put the key `javax`.xxx class here as the witness class.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#discussion_r926829599
##########
apm-sniffer/apm-sdk-plugin/tomcat-7.x-8.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/tomcat78x/define/TomcatInstrumentation.java:
##########
@@ -45,6 +49,16 @@ public class TomcatInstrumentation extends ClassInstanceMethodsEnhancePluginDefi
*/
private static final String EXCEPTION_INTERCEPT_CLASS = "org.apache.skywalking.apm.plugin.tomcat78x.TomcatExceptionInterceptor";
+ @Override
+ protected String[] witnessClasses() {
+ return new String[]{"org.apache.catalina.core.StandardHostValve"};
Review Comment:
I think you should put the key `javax`.xxx class here as the witness class.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] yswdqz commented on pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#issuecomment-1192104978
@kezhenxu94 The approved was dismissed, maybe need a new approve
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] kezhenxu94 commented on a diff in pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#discussion_r927210618
##########
apm-sniffer/apm-sdk-plugin/pom.xml:
##########
@@ -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>
Review Comment:
```suggestion
<module>tomcat-10x-plugin</module>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wuwen5 commented on a diff in pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
wuwen5 commented on code in PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#discussion_r927331018
##########
apm-sniffer/apm-sdk-plugin/pom.xml:
##########
@@ -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>
Review Comment:
There are 2 tabs here, which should be indented with 4 spaces
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] yswdqz commented on a diff in pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
yswdqz commented on code in PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#discussion_r926298869
##########
apm-sniffer/apm-sdk-plugin/tomcat-10x-plugin/src/main/java/org/apache/skywalking/apm/plugin/tomcat10x/ForwardInterceptor.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ *
+ */
+
+package org.apache.skywalking.apm.plugin.tomcat10x;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceConstructorInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+import java.util.HashMap;
+import java.util.Map;
+
+public class ForwardInterceptor implements InstanceMethodsAroundInterceptor, InstanceConstructorInterceptor {
+
+ @Override
+ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes,
+ MethodInterceptResult result) throws Throwable {
+ if (ContextManager.isActive()) {
+ AbstractSpan abstractTracingSpan = ContextManager.activeSpan();
+ Map<String, String> eventMap = new HashMap<String, String>();
+ eventMap.put("forward-url", objInst.getSkyWalkingDynamicField() == null ? "" : String.valueOf(objInst.getSkyWalkingDynamicField()));
Review Comment:
I reference the code from tomcat78 plugin. if url has redirect . it will log forward url.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] yswdqz commented on pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#issuecomment-1191600731
I'm having some problems with my compiler, so I can only use github to edit files today, it won't happen in the future.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#discussion_r926834859
##########
apm-sniffer/apm-sdk-plugin/tomcat-10x-plugin/src/main/java/org/apache/skywalking/apm/plugin/tomcat10x/define/ApplicationDispatcherInstrumentation.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.plugin.tomcat10x.define;
+
+import net.bytebuddy.description.method.MethodDescription;
+import net.bytebuddy.matcher.ElementMatcher;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.ConstructorInterceptPoint;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.InstanceMethodsInterceptPoint;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.ClassInstanceMethodsEnhancePluginDefine;
+import org.apache.skywalking.apm.agent.core.plugin.match.ClassMatch;
+
+import static net.bytebuddy.matcher.ElementMatchers.any;
+import static net.bytebuddy.matcher.ElementMatchers.named;
+import static org.apache.skywalking.apm.agent.core.plugin.match.NameMatch.byName;
+
+public class ApplicationDispatcherInstrumentation extends ClassInstanceMethodsEnhancePluginDefine {
+
+ private static final String ENHANCE_CLASS = "org.apache.catalina.core.ApplicationDispatcher";
+ private static final String ENHANCE_METHOD = "forward";
+ public static final String INTERCEPTOR_CLASS = "org.apache.skywalking.apm.plugin.tomcat10x.ForwardInterceptor";
+
+ @Override
+ protected String[] witnessClasses() {
+ return new String[]{"jakarta.servlet.http.HttpServletResponse", "org.apache.catalina.core.StandardHostValve"};
Review Comment:
The plugin would only work when and only when `ENHANCE_CLASS`(`ApplicationDispatcher`) exists. The witness helps the plugins when ENHANCE_CLASS exists across versions, but behaves differently.
So, if `jakarta.servlet.http.HttpServletResponse` is enough to determine version, then one should be good. `ApplicationDispatcher` and `StandardHostValve` would exist one only, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#discussion_r926828445
##########
apm-sniffer/apm-sdk-plugin/tomcat-10x-plugin/src/main/java/org/apache/skywalking/apm/plugin/tomcat10x/define/ApplicationDispatcherInstrumentation.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.plugin.tomcat10x.define;
+
+import net.bytebuddy.description.method.MethodDescription;
+import net.bytebuddy.matcher.ElementMatcher;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.ConstructorInterceptPoint;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.InstanceMethodsInterceptPoint;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.ClassInstanceMethodsEnhancePluginDefine;
+import org.apache.skywalking.apm.agent.core.plugin.match.ClassMatch;
+
+import static net.bytebuddy.matcher.ElementMatchers.any;
+import static net.bytebuddy.matcher.ElementMatchers.named;
+import static org.apache.skywalking.apm.agent.core.plugin.match.NameMatch.byName;
+
+public class ApplicationDispatcherInstrumentation extends ClassInstanceMethodsEnhancePluginDefine {
+
+ private static final String ENHANCE_CLASS = "org.apache.catalina.core.ApplicationDispatcher";
+ private static final String ENHANCE_METHOD = "forward";
+ public static final String INTERCEPTOR_CLASS = "org.apache.skywalking.apm.plugin.tomcat10x.ForwardInterceptor";
+
+ @Override
+ protected String[] witnessClasses() {
+ return new String[]{"jakarta.servlet.http.HttpServletResponse", "org.apache.catalina.core.StandardHostValve"};
Review Comment:
If `org.apache.catalina.core.StandardHostValve` always exists, you don't need this in the witness class.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#discussion_r926287157
##########
apm-sniffer/apm-sdk-plugin/tomcat-10x-plugin/src/main/java/org/apache/skywalking/apm/plugin/tomcat10x/ForwardInterceptor.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ *
+ */
+
+package org.apache.skywalking.apm.plugin.tomcat10x;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceConstructorInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+import java.util.HashMap;
+import java.util.Map;
+
+public class ForwardInterceptor implements InstanceMethodsAroundInterceptor, InstanceConstructorInterceptor {
+
+ @Override
+ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes,
+ MethodInterceptResult result) throws Throwable {
+ if (ContextManager.isActive()) {
+ AbstractSpan abstractTracingSpan = ContextManager.activeSpan();
+ Map<String, String> eventMap = new HashMap<String, String>();
+ eventMap.put("forward-url", objInst.getSkyWalkingDynamicField() == null ? "" : String.valueOf(objInst.getSkyWalkingDynamicField()));
Review Comment:
Why do you put a `forward-url` into an event to log?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#issuecomment-1191087929
There are 3 jars in the source codes(such as [test/plugin/scenarios/tomcat-10x-scenario/WEB-INF/lib/jakarta.servlet-api-5.0.0.jar](https://github.com/apache/skywalking-java/pull/286/files#diff-e5d900740cd2e827d91359de6b8dc43411e466f344d8bd2de44169a67d870b44)).
The binary codes are not allowed, we should download from maven.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#issuecomment-1191085295
`Supported-list.md` doc should be update as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] yswdqz commented on pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
yswdqz commented on PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#issuecomment-1192794490
It seems that there are still some problems in my expectedData.yaml. I will fix them tomorrow morning.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng merged pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by GitBox <gi...@apache.org>.
wu-sheng merged PR #286:
URL: https://github.com/apache/skywalking-java/pull/286
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #286: Add an agent plugin to support tomcat10.x(#7420)
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #286:
URL: https://github.com/apache/skywalking-java/pull/286#discussion_r1119756263
##########
apm-sniffer/apm-sdk-plugin/tomcat-10x-plugin/src/main/java/org/apache/skywalking/apm/plugin/tomcat10x/TomcatInvokeInterceptor.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.plugin.tomcat10x;
+
+import jakarta.servlet.http.HttpServletResponse;
+import org.apache.catalina.connector.Request;
+import org.apache.skywalking.apm.agent.core.context.CarrierItem;
+import org.apache.skywalking.apm.agent.core.context.ContextCarrier;
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.tag.Tags;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.context.trace.SpanLayer;
+
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+import org.apache.skywalking.apm.agent.core.util.CollectionUtil;
+import org.apache.skywalking.apm.agent.core.util.MethodUtil;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+import org.apache.skywalking.apm.util.StringUtil;
+import org.apache.tomcat.util.http.Parameters;
+
+import java.lang.reflect.Method;
+import java.util.Enumeration;
+import java.util.HashMap;
+import java.util.Map;
+
+public class TomcatInvokeInterceptor implements InstanceMethodsAroundInterceptor {
+
+ private static boolean IS_SERVLET_GET_STATUS_METHOD_EXIST;
+ private static final String SERVLET_RESPONSE_CLASS = "jakarta.servlet.http.HttpServletResponse";
+ private static final String GET_STATUS_METHOD = "getStatus";
+
+ static {
+ IS_SERVLET_GET_STATUS_METHOD_EXIST = MethodUtil.isMethodExist(
Review Comment:
Yes, I think it is safe to remove this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org