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/04/07 09:09:42 UTC
[GitHub] [skywalking-java] Cool-Coding opened a new pull request, #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Cool-Coding opened a new pull request, #146:
URL: https://github.com/apache/skywalking-java/pull/146
### Add an agent plugin to support JDK ThreadPoolExecutor and its subclasses
- [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)
### <Feature description>
- [x] Update the documentation to include this new feature.
- [x] Tests(including UT, IT, E2E) are added to verify the new feature.
- [x] If it's UI related, attach the screenshots below.
*ThreadPoolExecutor.submit(Callable)*
*ThreadPoolExecutor.execute(Runnable)*
*ThreadPoolExecutor.submit(Runnable)*
*ScheduledThreadPoolExecutor.submit(Callable)*
*ScheduledThreadPoolExecutor.execute(Runnable)*
*ScheduledThreadPoolExecutor.submit(Runnable)*
*submit(Callable) exception*
*execute(Runnable) exception*
- [x] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes https://github.com/apache/skywalking/issues/8743.
- [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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r845136231
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
> And, have you known there are meter plugins for thread pool, such as `UndertowWorkerThreadPoolConstructorIntercept`. I think if this plugin covers the thread pool case too, it should report the thread pool metrics too.
I've seen the issue before. I seen it again just now. it also instructs `ThreadPoolExecutor` with the match rule that package name is started with "org.xnio". If my `ThreadPoolExecutor` plugin covers this, the `Undertow ThreadPool` plugin is redundant I think. I will take it into account.
Please take a look at [the reply](#discussion_r845092153) and give me a reply. Thank you!
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#issuecomment-1094691902
Are you going to create a new one?
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#issuecomment-1095062515
It seems some test cases fail, please recheck.
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r847892607
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
Got it, make sense. Yes, maybe we have to use the class name as the thread pool name.
I think we could make trace supported in this PR and do meter collecting in another?
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848042762
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/ThreadPoolExecuteMethodInterceptor.java:
##########
@@ -0,0 +1,45 @@
+/*
+ * 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;
+
+import org.apache.skywalking.apm.plugin.wrapper.SwRunnableWrapper;
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import java.util.concurrent.RunnableFuture;
+
+public class ThreadPoolExecuteMethodInterceptor extends AbstractThreadingPoolInterceptor {
+
+ @Override
+ public Object wrap(Object param) {
+ if (param instanceof SwRunnableWrapper) {
+ return param;
+ }
+
+ if (param instanceof RunnableFuture) {
+ return null;
Review Comment:
> OK, please add comments on `AbstractThreadingPoolInterceptor#wrap`.
>
> Also consider to add more comments on codes too.
Ok!
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848082194
##########
apm-sniffer/apm-agent/src/main/java/org/apache/skywalking/apm/agent/SkyWalkingAgent.java:
##########
@@ -130,6 +130,8 @@ public static void premain(String agentArgs, Instrumentation instrumentation) th
.with(new Listener())
.installOn(instrumentation);
+ SnifferConfigInitializer.pluginInitCompleted();
Review Comment:
>
`PluginFinder` is responsible for loading plugins and matching classes. `AgentBuilder` is responsible for transforming classes. `IS_PLUGIN_INIT_COMPLETED` must be set true after `AgentBuilder. installOn(instrumentation)`. Maybe the variable field name is not very appropriate. ` IS_PLUGIN_INSTALLED_COMPLETED` maybe more appropriate. WDYT?
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848167297
##########
apm-sniffer/apm-agent/src/main/java/org/apache/skywalking/apm/agent/SkyWalkingAgent.java:
##########
@@ -130,6 +130,8 @@ public static void premain(String agentArgs, Instrumentation instrumentation) th
.with(new Listener())
.installOn(instrumentation);
+ SnifferConfigInitializer.pluginInitCompleted();
Review Comment:
I think you still don't follow all requirements. Rename is required, and move to `SkyWalkingAgent#IS_PLUGIN_INSTALLED_COMPLETED`.
> > Putting it in the Agent makes sense.
>
> Ok, Then keep it like this.
I think I said putting it in the Agent class, not the way it is right now. You somehow misunderstood the point.
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848222864
##########
apm-sniffer/apm-agent/src/main/java/org/apache/skywalking/apm/agent/SkyWalkingAgent.java:
##########
@@ -130,6 +130,8 @@ public static void premain(String agentArgs, Instrumentation instrumentation) th
.with(new Listener())
.installOn(instrumentation);
+ SnifferConfigInitializer.pluginInitCompleted();
Review Comment:
It is fine, I just want to make sure the agent's source codes are still well organized. After all, it is already super complex.
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng merged PR #146:
URL: https://github.com/apache/skywalking-java/pull/146
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848062573
##########
apm-sniffer/apm-agent/src/main/java/org/apache/skywalking/apm/agent/SkyWalkingAgent.java:
##########
@@ -130,6 +130,8 @@ public static void premain(String agentArgs, Instrumentation instrumentation) th
.with(new Listener())
.installOn(instrumentation);
+ SnifferConfigInitializer.pluginInitCompleted();
Review Comment:
I think `pluginInitCompleted` should belong to `SnifferConfigInitializer`.
the IS_PLUGIN_INIT_COMPLETED field is not a flag that indicates the plugins are loaded. It's the flag that ByteBuddy has loaded all plugins. Before this, `ThreadPoolExecutor` can't be created.
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r845011495
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/AbstractThreadingPoolInterceptor.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * 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;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+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 java.lang.reflect.Method;
+
+public abstract class AbstractThreadingPoolInterceptor implements InstanceMethodsAroundInterceptor {
+ @Override
+ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
+ if (!ContextManager.isActive()) {
+ return;
+ }
+
+ if (allArguments == null || allArguments.length != 1) {
+ return;
+ }
+ Object argument = allArguments[0];
+
+ Object updateResult = updateOnlyParam(argument);
+ if (updateResult != null) {
+ allArguments[0] = updateResult;
+ }
+ }
+
+ public abstract Object updateOnlyParam(Object param);
Review Comment:
Then we should name it `wrapExecutor`?
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r845039466
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
>
To be honest, My focus is `ThreadPoolExecutor`, not all kinds of `TheadPool`s. if we want to cover all the `crossThread` situations, I will think it over include `ScheduledThreadPool` and `ForkJoinPool`. And the instructed point is not `java.util.concurrent.ThreadPoolExecutor`, it will be `java.util.concurrent.Executor` and `java.util.concurrent.ExecutorService`. it is complex. The cost is relatively large and The risk is relatively high. so I chose the `ThreadPoolExecutor` commonly used by users. WDYT?
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848018873
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
> One more scenario to discussion,
>
> The ThreadPoolExecutor could be used to run an endless loop task, like a timer. And `run` method would never end. As you mentioned using the class name to filter in meter collection, should we add this kind of mechanism in the tracing part too? (Have to back about caller detection, as user could use APIs rather than inherit from this)
Do you mean we do not trace some `Runnable` or `Callable` task ?
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848091537
##########
apm-sniffer/apm-agent/src/main/java/org/apache/skywalking/apm/agent/SkyWalkingAgent.java:
##########
@@ -130,6 +130,8 @@ public static void premain(String agentArgs, Instrumentation instrumentation) th
.with(new Listener())
.installOn(instrumentation);
+ SnifferConfigInitializer.pluginInitCompleted();
Review Comment:
> Member
Ok, Then keep it like 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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848036208
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/define/ThreadPoolExecutorInstrumentation.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.define;
+
+import net.bytebuddy.description.method.MethodDescription;
+import net.bytebuddy.matcher.ElementMatcher;
+import net.bytebuddy.matcher.ElementMatchers;
+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.StaticMethodsInterceptPoint;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.ClassEnhancePluginDefine;
+import org.apache.skywalking.apm.agent.core.plugin.match.ClassMatch;
+import org.apache.skywalking.apm.agent.core.plugin.match.HierarchyMatch;
+import org.apache.skywalking.apm.agent.core.plugin.match.MultiClassNameMatch;
+import org.apache.skywalking.apm.agent.core.plugin.match.logical.LogicalMatchOperation;
+
+public class ThreadPoolExecutorInstrumentation extends ClassEnhancePluginDefine {
+
+ protected static final String ENHANCE_CLASS = "java.util.concurrent.ThreadPoolExecutor";
+
+ private static final String INTERCEPT_EXECUTE_METHOD = "execute";
+
+ private static final String INTERCEPT_SUBMIT_METHOD = "submit";
+
+ private static final String INTERCEPT_EXECUTE_METHOD_HANDLE = "org.apache.skywalking.apm.plugin.ThreadPoolExecuteMethodInterceptor";
+
+ private static final String INTERCEPT_SUBMIT_METHOD_HANDLE = "org.apache.skywalking.apm.plugin.ThreadPoolSubmitMethodInterceptor";
+
+ @Override
+ public boolean isBootstrapInstrumentation() {
+ return true;
+ }
+
+ @Override
+ protected ClassMatch enhanceClass() {
+ return LogicalMatchOperation.or(HierarchyMatch.byHierarchyMatch(ENHANCE_CLASS), MultiClassNameMatch.byMultiClassMatch(ENHANCE_CLASS));
+ }
+
+ @Override
+ public ConstructorInterceptPoint[] getConstructorsInterceptPoints() {
+ return new ConstructorInterceptPoint[0];
+ }
+
+ @Override
+ public InstanceMethodsInterceptPoint[] getInstanceMethodsInterceptPoints() {
+ return new InstanceMethodsInterceptPoint[]{
+ new InstanceMethodsInterceptPoint() {
+ @Override
+ public ElementMatcher<MethodDescription> getMethodsMatcher() {
+ return ElementMatchers.named(INTERCEPT_EXECUTE_METHOD);
+ }
+
+ @Override
+ public String getMethodsInterceptor() {
+ return INTERCEPT_EXECUTE_METHOD_HANDLE;
+ }
+
+ @Override
+ public boolean isOverrideArgs() {
+ return true;
+ }
+ },
+ new InstanceMethodsInterceptPoint() {
+ @Override
+ public ElementMatcher<MethodDescription> getMethodsMatcher() {
+ return ElementMatchers.named(INTERCEPT_SUBMIT_METHOD);
+ }
+
+ @Override
+ public String getMethodsInterceptor() {
+ return INTERCEPT_SUBMIT_METHOD_HANDLE;
+ }
+
+ @Override
+ public boolean isOverrideArgs() {
+ return true;
+ }
+ }
+ };
+ }
+
+ @Override
+ public StaticMethodsInterceptPoint[] getStaticMethodsInterceptPoints() {
+ return new StaticMethodsInterceptPoint[0];
+ }
Review Comment:
```suggestion
```
ClassInstanceMethodsEnhancePluginDefine has covered 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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r844946438
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/AbstractThreadingPoolInterceptor.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * 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;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+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 java.lang.reflect.Method;
+
+public abstract class AbstractThreadingPoolInterceptor implements InstanceMethodsAroundInterceptor {
+ @Override
+ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
+ if (!ContextManager.isActive()) {
+ return;
+ }
+
+ if (allArguments == null || allArguments.length != 1) {
+ return;
+ }
+ Object argument = allArguments[0];
+
+ Object updateResult = updateOnlyParam(argument);
+ if (updateResult != null) {
+ allArguments[0] = updateResult;
+ }
+ }
+
+ public abstract Object updateOnlyParam(Object param);
Review Comment:
>
it will change the param of `execute` and `submit` method wrapping it to `SWCallableWrapper` or `SWRunnableWrapper`.
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r845074898
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/AbstractThreadingPoolInterceptor.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * 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;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+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 java.lang.reflect.Method;
+
+public abstract class AbstractThreadingPoolInterceptor implements InstanceMethodsAroundInterceptor {
+ @Override
+ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
+ if (!ContextManager.isActive()) {
+ return;
+ }
+
+ if (allArguments == null || allArguments.length != 1) {
+ return;
+ }
+ Object argument = allArguments[0];
+
+ Object updateResult = updateOnlyParam(argument);
+ if (updateResult != null) {
+ allArguments[0] = updateResult;
+ }
+ }
+
+ public abstract Object updateOnlyParam(Object param);
Review Comment:
Just `wrap` should be enough, I think.
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r845092153
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
> Yes, I know. But Executors.newSingleThreadScheduledExecutor covers this case, right? Otherwise, FileWriter should not be an issue.
Yes, `ScheduledThreadPool` is inherited from `ThreadPoolExecutor,` also has execute and submit methods. so I take all the subclasses of `ThreadPoolExecutor` into account, but only execute and submit methods. Do you think we just take `scheduleAtFixedRate` and `scheduleWithFixedDelay` into account in addition to `execute` and `submit`, or cover all kinds of threadpools?
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r844914267
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/conf/SnifferConfigInitializer.java:
##########
@@ -234,4 +235,12 @@ static void configureLogger() {
LogManager.setLogResolver(new PatternLogResolver());
}
}
+
+ public static void pluginInitCompleted() {
+ IS_PLUGIN_INIT_COMPLETED = true;
+ }
+
+ public static boolean isPluginInitCompleted() {
+ return IS_PLUGIN_INIT_COMPLETED;
+ }
Review Comment:
But simply change like this, you would make a lot of logs disappear from `agent.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 a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848031738
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/conf/SnifferConfigInitializer.java:
##########
@@ -234,4 +235,12 @@ static void configureLogger() {
LogManager.setLogResolver(new PatternLogResolver());
}
}
+
+ public static void pluginInitCompleted() {
+ IS_PLUGIN_INIT_COMPLETED = true;
+ }
+
+ public static boolean isPluginInitCompleted() {
+ return IS_PLUGIN_INIT_COMPLETED;
+ }
Review Comment:
> Also, if you need a new flag of plugin initialization, it looks like the flag should be in PluginFinder? SnifferConfigInitializer.java seems a wrong place to add.
I mean which class should own the flag. `SnifferConfigInitializer` has nothing related to the plugin mechanism.
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848062573
##########
apm-sniffer/apm-agent/src/main/java/org/apache/skywalking/apm/agent/SkyWalkingAgent.java:
##########
@@ -130,6 +130,8 @@ public static void premain(String agentArgs, Instrumentation instrumentation) th
.with(new Listener())
.installOn(instrumentation);
+ SnifferConfigInitializer.pluginInitCompleted();
Review Comment:
I think `pluginInitCompleted` show belong to `SnifferConfigInitializer`.
the IS_PLUGIN_INIT_COMPLETED field is not a flag that indicates the plugins are loaded. It's the flag that ByteBuddy has loaded all plugins. Before this, ThreadPoolExecutor can't be created.
##########
apm-sniffer/apm-agent/src/main/java/org/apache/skywalking/apm/agent/SkyWalkingAgent.java:
##########
@@ -130,6 +130,8 @@ public static void premain(String agentArgs, Instrumentation instrumentation) th
.with(new Listener())
.installOn(instrumentation);
+ SnifferConfigInitializer.pluginInitCompleted();
Review Comment:
I think `pluginInitCompleted` should belong to `SnifferConfigInitializer`.
the IS_PLUGIN_INIT_COMPLETED field is not a flag that indicates the plugins are loaded. It's the flag that ByteBuddy has loaded all plugins. Before this, ThreadPoolExecutor can't be created.
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848057766
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/conf/SnifferConfigInitializer.java:
##########
@@ -234,4 +235,12 @@ static void configureLogger() {
LogManager.setLogResolver(new PatternLogResolver());
}
}
+
+ public static void pluginInitCompleted() {
+ IS_PLUGIN_INIT_COMPLETED = true;
+ }
+
+ public static boolean isPluginInitCompleted() {
+ return IS_PLUGIN_INIT_COMPLETED;
+ }
Review Comment:
>
the `IS_PLUGIN_INIT_COMPLETED ` field is not a flag that indicates the plugins are loaded. It's the flag that `ByteBuddy` has loaded all plugins. Before this, `ThreadPoolExecutor` can't be created.
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r846982138
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
@wu-sheng `ScheduledThreadPoolExecutor.scheduleAtFixedRate` has been supported. but the trace is getting longer and longer over the time. I think it is a bad sense. WDYT?
Besides, `ScheduledThreadPoolExecutor.scheduleAtFixedRate` in `JVMService` doesn't show in the trace ui. I don't identify the caller. I don't know the reason.
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848084427
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/AbstractThreadingPoolInterceptor.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * 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;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+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 java.lang.reflect.Method;
+
+public abstract class AbstractThreadingPoolInterceptor implements InstanceMethodsAroundInterceptor {
+ @Override
+ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
+ if (!ContextManager.isActive()) {
+ return;
+ }
+
+ if (allArguments == null || allArguments.length < 1) {
+ return;
+ }
+
+ Object argument = allArguments[0];
+
+ Object updateResult = wrap(argument);
+ if (updateResult != null) {
+ allArguments[0] = updateResult;
+ }
+ }
+
+ public abstract Object wrap(Object param);
+
+ @Override
+ public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Object ret) throws Throwable {
+ return ret;
+ }
+
+ @Override
+ public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Throwable t) {
+ if (ContextManager.isActive()) {
Review Comment:
> Then, please remove all these checks. An exception is better than nothing.
Ok.
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#issuecomment-1096206451
I think this is mostly done. Once this is merged, I will try to release agent 8.10.0 if you don't have more to add in this release.
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848127505
##########
apm-sniffer/apm-agent/src/main/java/org/apache/skywalking/apm/agent/SkyWalkingAgent.java:
##########
@@ -130,6 +130,8 @@ public static void premain(String agentArgs, Instrumentation instrumentation) th
.with(new Listener())
.installOn(instrumentation);
+ SnifferConfigInitializer.pluginInitCompleted();
Review Comment:
Please move and rename this flag, I think others are good to me.
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r847025124
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
> > And, have you known there are meter plugins for thread pool, such as `UndertowWorkerThreadPoolConstructorIntercept`. I think if this plugin covers the thread pool case too, it should report the thread pool metrics too.
>
> I've seen the issue before. I seen it again just now. it also instructs `ThreadPoolExecutor` with the match rule that package name is started with "org.xnio". If my `ThreadPoolExecutor` plugin covers this, the `Undertow ThreadPool` plugin is redundant I think. I will take it into account.
>
> Please take a look at [reply1](#discussion_r845039466), [reply2 ](#discussion_r845092153). I want to know your point of view. Thank you!
@wu-sheng I am adding `ThreadPoolExecutor` metrics. But How to distinguish different thread pools？That is, how to determine the thread pool name. so far, I haven't thought of a meaningful name yet. Do you have any good suggestions?
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r845076502
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
2 general ways.
1. Ask SkyWalking core to use a specific interface(extends Runnable/Callable) only. Then the plugin is easy to resolve the caller.
2. Get the stack and try to find `org.apache.skywalking`.
The (2) is easier but it costs performance due to getting stack elements.
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848062237
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/conf/SnifferConfigInitializer.java:
##########
@@ -234,4 +235,12 @@ static void configureLogger() {
LogManager.setLogResolver(new PatternLogResolver());
}
}
+
+ public static void pluginInitCompleted() {
+ IS_PLUGIN_INIT_COMPLETED = true;
+ }
+
+ public static boolean isPluginInitCompleted() {
+ return IS_PLUGIN_INIT_COMPLETED;
+ }
Review Comment:
SnifferConfigInitializer class takes the responsibility to initialize config only. Nothing more.
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848087872
##########
apm-sniffer/apm-agent/src/main/java/org/apache/skywalking/apm/agent/SkyWalkingAgent.java:
##########
@@ -130,6 +130,8 @@ public static void premain(String agentArgs, Instrumentation instrumentation) th
.with(new Listener())
.installOn(instrumentation);
+ SnifferConfigInitializer.pluginInitCompleted();
Review Comment:
Putting it in the Agent makes sense.
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r847191002
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
@wu-sheng Usually, an application includes many thread pools. If we collect all thread pool meters, but users don't pay attention to these meters, then these collected meters are garbage data. I think we can let users configure thread pools they care about in `agent.config`. the rough format is `class1:name1,name2;class2:name1,name2`. Then we can match it and name `ThreadFactory` in `ThreadPoolExecutor` plugin through `thread stack`. Although it consumes a little performance when the application starts, I think it's worth it. WDYT?
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r844919090
##########
apm-sniffer/apm-agent/src/main/java/org/apache/skywalking/apm/agent/SkyWalkingAgent.java:
##########
@@ -138,6 +138,8 @@ public static void premain(String agentArgs, Instrumentation instrumentation) th
Runtime.getRuntime()
.addShutdownHook(new Thread(ServiceManager.INSTANCE::shutdown, "skywalking service shutdown thread"));
+
+ SnifferConfigInitializer.pluginInitCompleted();
Review Comment:
Also, `Executors.newSingleThreadScheduledExecutor` is not just used in **FileWriter**, others like `JVMService` service used them too. I think put `SnifferConfigInitializer.pluginInitCompleted();` here would make more logs disappeared. You just need this at L131, right after `installOn(instrumentation)`
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r845005282
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
But `scheduleAtFixedRate` is also could be used by users, which would be missed by this 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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r845092153
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
> Yes, I know. But Executors.newSingleThreadScheduledExecutor covers this case, right? Otherwise, FileWriter should not be an issue.
Yes, `ScheduledThreadPool` is inherited from `ThreadPoolExecutor,` also has execute and submit methods. so I take all the subclasses of T`hreadPoolExecutor` into account, but only execute and submit methods. Do you think we just take `scheduleAtFixedRate` and `scheduleWithFixedDelay` into account in addition to `execute` and `submit`, or cover all kinds of threadpools?
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r845100071
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
Once this is `ThreadPoolExecutor` plugin, it should cover all known cases, which is why we should detect caller one way or another.
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r845073266
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
> I think the better way is to identify the caller and exclude the agent package. WDYT? How to identify could be discussed later.
Ok, I will think another better way. Keep in touch!
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r844914254
##########
test/plugin/scenarios/jdk-threadpool-scenario/bin/startup.sh:
##########
@@ -0,0 +1,21 @@
+#!/bin/bash
+#
+# 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.
+
+home="$(cd "$(dirname $0)"; pwd)"
+
+java -jar ${agent_opts} ${home}/../libs/jdk-threadpool-scenario.jar &
Review Comment:
> GHA
I don't ever develop a plugin. How to add it into GHA? Can you teach me? Thanks!
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r844907265
##########
test/plugin/scenarios/jdk-threadpool-scenario/bin/startup.sh:
##########
@@ -0,0 +1,21 @@
+#!/bin/bash
+#
+# 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.
+
+home="$(cd "$(dirname $0)"; pwd)"
+
+java -jar ${agent_opts} ${home}/../libs/jdk-threadpool-scenario.jar &
Review Comment:
Your new test is not added into GHA.
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r845014078
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
I think the better way is to identify the caller and exclude the agent package. WDYT?
How to identify could be discussed later.
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r847014945
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
OK, I think we should not support this for now. It makes the trace strange.
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848018242
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
> Got it, make sense. Yes, maybe we have to use the class name as the thread pool name.
>
> I think we could make trace supported in this PR and do meter collecting in another?
ok, agree!
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848065799
##########
apm-sniffer/apm-agent/src/main/java/org/apache/skywalking/apm/agent/SkyWalkingAgent.java:
##########
@@ -130,6 +130,8 @@ public static void premain(String agentArgs, Instrumentation instrumentation) th
.with(new Listener())
.installOn(instrumentation);
+ SnifferConfigInitializer.pluginInitCompleted();
Review Comment:
No, you have a misunderstanding with the codes. PluginFinder is the one working with bytebuddy. SnifferConfigInitializer is just about `Config.class`.
Please read the codes again.
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848034017
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/ThreadPoolExecuteMethodInterceptor.java:
##########
@@ -0,0 +1,45 @@
+/*
+ * 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;
+
+import org.apache.skywalking.apm.plugin.wrapper.SwRunnableWrapper;
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import java.util.concurrent.RunnableFuture;
+
+public class ThreadPoolExecuteMethodInterceptor extends AbstractThreadingPoolInterceptor {
+
+ @Override
+ public Object wrap(Object param) {
+ if (param instanceof SwRunnableWrapper) {
+ return param;
+ }
+
+ if (param instanceof RunnableFuture) {
+ return null;
Review Comment:
Would not `return null` cause an NPE? Same as below.
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r847042006
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
If it is not set, I think we could use pool's object ID as the name? It is meaningless, yes, but it is better than nothing.
After all, the thread pool metric is at instance level only.
--
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] Cool-Coding commented on pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#issuecomment-1094733636
> Are you going to create a new one?
No, I will still use this one. There is a new PR before my PR is merged. I reset my commit just now and pulled the new PR just now. `GitHub` closed my PR automatically. I have reopened 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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r847020506
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
> OK, I think we should not support this for now. It makes the trace strange.
Agree.
--
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] Cool-Coding closed pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding closed pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
URL: https://github.com/apache/skywalking-java/pull/146
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r844991022
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
>
I have considered this case. the instructed point are just`execute` and `submit` methods in `ThreadPoolExecutor` and its subclasses. `FileWriter` and `JVMService` use `scheduleAtFixedRate` method.
If the `execute` method in `ThreadPoolExecutor` subclass calls its super class `execute` method, this would result in `Runnable` will be wrapped twice, but it won't form a `spin-tracing`.
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r844927891
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/conf/SnifferConfigInitializer.java:
##########
@@ -234,4 +235,12 @@ static void configureLogger() {
LogManager.setLogResolver(new PatternLogResolver());
}
}
+
+ public static void pluginInitCompleted() {
+ IS_PLUGIN_INIT_COMPLETED = true;
+ }
+
+ public static boolean isPluginInitCompleted() {
+ return IS_PLUGIN_INIT_COMPLETED;
+ }
Review Comment:
Also, if you need a new flag of plugin initialization, it looks like the flag should be in `PluginFinder`? [SnifferConfigInitializer.java](https://github.com/apache/skywalking-java/pull/146/files/22e85142ff56493cee90adf49d123b71ff3e0089#diff-3f4c9264a89197350b7eb2ca0e2f2daa494306980a00142edef7f5abfc941951) seems a wrong place to add.
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848195349
##########
apm-sniffer/apm-agent/src/main/java/org/apache/skywalking/apm/agent/SkyWalkingAgent.java:
##########
@@ -130,6 +130,8 @@ public static void premain(String agentArgs, Instrumentation instrumentation) th
.with(new Listener())
.installOn(instrumentation);
+ SnifferConfigInitializer.pluginInitCompleted();
Review Comment:
> Please move and rename this flag, I think others are good to me.
Ok, I understand it. We were not on the same channel just now. I will move it to the `PluginFinder`.
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848017887
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/conf/SnifferConfigInitializer.java:
##########
@@ -234,4 +235,12 @@ static void configureLogger() {
LogManager.setLogResolver(new PatternLogResolver());
}
}
+
+ public static void pluginInitCompleted() {
+ IS_PLUGIN_INIT_COMPLETED = true;
+ }
+
+ public static boolean isPluginInitCompleted() {
+ return IS_PLUGIN_INIT_COMPLETED;
+ }
Review Comment:
> @Cool-Coding I think this part of codes has not been polished yet.
I have polished the `WriterFactoryTest` UT adding the code ` BDDMockito.given(SnifferConfigInitializer.isPluginInitCompleted()).willReturn(true)`. The UT passed.
And the statement `SnifferConfigInitializer.pluginInitCompleted()` has been moved after `installOn(instrumentation)`.
```
agentBuilder.type(pluginFinder.buildMatch())
.transform(new Transformer(pluginFinder))
.with(AgentBuilder.RedefinitionStrategy.RETRANSFORMATION)
.with(new RedefinitionListener())
.with(new Listener())
.installOn(instrumentation);
SnifferConfigInitializer.pluginInitCompleted();
try {
ServiceManager.INSTANCE.boot();
} catch (Exception e) {
LOGGER.error(e, "Skywalking agent boot failure.");
}
```
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848043070
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/AbstractThreadingPoolInterceptor.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * 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;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+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 java.lang.reflect.Method;
+
+public abstract class AbstractThreadingPoolInterceptor implements InstanceMethodsAroundInterceptor {
+ @Override
+ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
+ if (!ContextManager.isActive()) {
+ return;
+ }
+
+ if (allArguments == null || allArguments.length < 1) {
+ return;
+ }
+
+ Object argument = allArguments[0];
+
+ Object updateResult = wrap(argument);
+ if (updateResult != null) {
+ allArguments[0] = updateResult;
+ }
+ }
+
+ public abstract Object wrap(Object param);
+
+ @Override
+ public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Object ret) throws Throwable {
+ return ret;
+ }
+
+ @Override
+ public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Throwable t) {
+ if (ContextManager.isActive()) {
Review Comment:
Is any chance of not active? Besides bug.
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848039534
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/ThreadPoolExecuteMethodInterceptor.java:
##########
@@ -0,0 +1,45 @@
+/*
+ * 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;
+
+import org.apache.skywalking.apm.plugin.wrapper.SwRunnableWrapper;
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import java.util.concurrent.RunnableFuture;
+
+public class ThreadPoolExecuteMethodInterceptor extends AbstractThreadingPoolInterceptor {
+
+ @Override
+ public Object wrap(Object param) {
+ if (param instanceof SwRunnableWrapper) {
+ return param;
+ }
+
+ if (param instanceof RunnableFuture) {
+ return null;
Review Comment:
it won't cause NPE. Let's see the caller code in 'beforeMethod'.
```
Object updateResult = wrap(argument);
if (updateResult != null) {
allArguments[0] = updateResult;
}
```
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848042762
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/ThreadPoolExecuteMethodInterceptor.java:
##########
@@ -0,0 +1,45 @@
+/*
+ * 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;
+
+import org.apache.skywalking.apm.plugin.wrapper.SwRunnableWrapper;
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import java.util.concurrent.RunnableFuture;
+
+public class ThreadPoolExecuteMethodInterceptor extends AbstractThreadingPoolInterceptor {
+
+ @Override
+ public Object wrap(Object param) {
+ if (param instanceof SwRunnableWrapper) {
+ return param;
+ }
+
+ if (param instanceof RunnableFuture) {
+ return null;
Review Comment:
> OK, please add comments on `AbstractThreadingPoolInterceptor#wrap`.
>
> Also consider to add more comments on codes too.
Ok!
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r847835493
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/conf/SnifferConfigInitializer.java:
##########
@@ -234,4 +235,12 @@ static void configureLogger() {
LogManager.setLogResolver(new PatternLogResolver());
}
}
+
+ public static void pluginInitCompleted() {
+ IS_PLUGIN_INIT_COMPLETED = true;
+ }
+
+ public static boolean isPluginInitCompleted() {
+ return IS_PLUGIN_INIT_COMPLETED;
+ }
Review Comment:
@Cool-Coding I think this part of codes has not been polished yet.
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848055787
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/AbstractThreadingPoolInterceptor.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * 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;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+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 java.lang.reflect.Method;
+
+public abstract class AbstractThreadingPoolInterceptor implements InstanceMethodsAroundInterceptor {
+ @Override
+ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
+ if (!ContextManager.isActive()) {
+ return;
+ }
+
+ if (allArguments == null || allArguments.length < 1) {
+ return;
+ }
+
+ Object argument = allArguments[0];
+
+ Object updateResult = wrap(argument);
+ if (updateResult != null) {
+ allArguments[0] = updateResult;
+ }
+ }
+
+ public abstract Object wrap(Object param);
+
+ @Override
+ public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Object ret) throws Throwable {
+ return ret;
+ }
+
+ @Override
+ public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Throwable t) {
+ if (ContextManager.isActive()) {
Review Comment:
Then, please remove all these checks. An exception is better than nothing.
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r844924895
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/conf/SnifferConfigInitializer.java:
##########
@@ -234,4 +235,12 @@ static void configureLogger() {
LogManager.setLogResolver(new PatternLogResolver());
}
}
+
+ public static void pluginInitCompleted() {
+ IS_PLUGIN_INIT_COMPLETED = true;
+ }
+
+ public static boolean isPluginInitCompleted() {
+ return IS_PLUGIN_INIT_COMPLETED;
+ }
Review Comment:
Yeah, Before plugins are loaded completed, logs will be printed by system.out.
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r844940055
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/AbstractThreadingPoolInterceptor.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * 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;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+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 java.lang.reflect.Method;
+
+public abstract class AbstractThreadingPoolInterceptor implements InstanceMethodsAroundInterceptor {
+ @Override
+ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
+ if (!ContextManager.isActive()) {
+ return;
+ }
+
+ if (allArguments == null || allArguments.length != 1) {
+ return;
+ }
+ Object argument = allArguments[0];
+
+ Object updateResult = updateOnlyParam(argument);
+ if (updateResult != null) {
+ allArguments[0] = updateResult;
+ }
+ }
+
+ public abstract Object updateOnlyParam(Object param);
Review Comment:
What does this method name mean? `onlyParam` means?
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r845050169
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
Yes, I know. But `Executors.newSingleThreadScheduledExecutor` covers this case, right? Otherwise, FileWriter should not be an issue.
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r847191002
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
@wu-sheng Usually, an application includes many thread pools. If we collect all thread pool meters, but users don't pay attention to these indicators, then these collected meters are garbage data. I think we can let users configure thread pools they care about in `agent.config`. the rough format is `class1:name1,name2;class2:name1,name2`. Then we can match it and name `ThreadFactory` in `ThreadPoolExecutor` plugin through `thread stack`. Although it consumes a little performance when the application starts, I think it's worth it. WDYT?
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#issuecomment-1096163241
> > Many NPEs happen in the UT, I think you break something. Please recheck.
>
> I have rechecked it again. There is still one failed that has nothing to do with my modification.
>
> ```
> ERROR: 1 dead links found!
> [✖] https://twitter.com/ASFSkyWalking → Status: 404
> ```
Ignore this. Don't worry.
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r847094811
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
> Thread pool supports setting a custom name by users. AFAIK.
User can custom thread's name, not threadpool's name. They are not equivalent. I will use object ID or other name that seems 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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848205981
##########
apm-sniffer/apm-agent/src/main/java/org/apache/skywalking/apm/agent/SkyWalkingAgent.java:
##########
@@ -130,6 +130,8 @@ public static void premain(String agentArgs, Instrumentation instrumentation) th
.with(new Listener())
.installOn(instrumentation);
+ SnifferConfigInitializer.pluginInitCompleted();
Review Comment:
@wu-sheng This is a simple design principle, Each class has its own responsibilities. I'm so sorry to let you spend a lot of time explaining.
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r847039462
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
Thread pool supports setting a custom name by users. AFAIK.
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r847915609
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/define/HierarchyThreadPoolExecutorInstrumentation.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * 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.define;
+
+import org.apache.skywalking.apm.agent.core.plugin.match.ClassMatch;
+import org.apache.skywalking.apm.agent.core.plugin.match.HierarchyMatch;
+
+public class HierarchyThreadPoolExecutorInstrumentation extends AbstractThreadPoolExecutorInstrumentation {
Review Comment:
> Consider to use `LogicalOrMatch` to replace this and `ThreadPoolExecutorInstrumentation`?
I have tried to use `LogicalMatchOperation.or` before. but it accepts `IndirectMatch` class type, however, `NameMatch` is not subclass of `IndirectMatch`. I replaced `NameMatch` with `MultiClassNameMatch` and verified it just now ,it works.
`LogicalMatchOperation.or(HierarchyMatch.byHierarchyMatch(ENHANCE_CLASS), MultiClassNameMatch.byMultiClassMatch(ENHANCE_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 pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#issuecomment-1095935482
Many NPEs happen in the UT, I think you break something. Please recheck.
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848061270
##########
apm-sniffer/apm-agent/src/main/java/org/apache/skywalking/apm/agent/SkyWalkingAgent.java:
##########
@@ -130,6 +130,8 @@ public static void premain(String agentArgs, Instrumentation instrumentation) th
.with(new Listener())
.installOn(instrumentation);
+ SnifferConfigInitializer.pluginInitCompleted();
Review Comment:
```suggestion
PluginFinder.pluginInitCompleted();
```
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r844910046
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/conf/SnifferConfigInitializer.java:
##########
@@ -234,4 +235,12 @@ static void configureLogger() {
LogManager.setLogResolver(new PatternLogResolver());
}
}
+
+ public static void pluginInitCompleted() {
+ IS_PLUGIN_INIT_COMPLETED = true;
+ }
+
+ public static boolean isPluginInitCompleted() {
+ return IS_PLUGIN_INIT_COMPLETED;
+ }
Review Comment:
the `FileWriter` class in Skywalking agent uses `ThreadPoolExecutor`. If plugins are not loaded, FileWriter is created. Then `ThreadPoolExecutor won't be instructed`. so we must make sure that after plugins are loaded, FileWriter can be created.
```
private FileWriter() {
logBuffer = new ArrayBlockingQueue(1024);
final ArrayList<String> outputLogs = new ArrayList<String>(200);
Executors.newSingleThreadScheduledExecutor(new DefaultNamedThreadFactory("LogFileWriter"))
.scheduleAtFixedRate(new RunnableWithExceptionProtection(new Runnable() {
@Override
public void run() {
try {
logBuffer.drainTo(outputLogs);
for (String log : outputLogs) {
writeToFile(log + Constants.LINE_SEPARATOR);
}
try {
fileOutputStream.flush();
} catch (IOException e) {
e.printStackTrace();
}
} finally {
outputLogs.clear();
}
}
}, new RunnableWithExceptionProtection.CallbackWhenException() {
@Override
public void handle(Throwable t) {
}
}), 0, 1, TimeUnit.SECONDS);
}
```
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r844910046
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/conf/SnifferConfigInitializer.java:
##########
@@ -234,4 +235,12 @@ static void configureLogger() {
LogManager.setLogResolver(new PatternLogResolver());
}
}
+
+ public static void pluginInitCompleted() {
+ IS_PLUGIN_INIT_COMPLETED = true;
+ }
+
+ public static boolean isPluginInitCompleted() {
+ return IS_PLUGIN_INIT_COMPLETED;
+ }
Review Comment:
the `FileWriter` class in Skywalking agent uses `ThreadPoolExecutor`. If plugins are not loaded, FileWriter is created. Then `ThreadPoolExecutor` won't be instructed. so we must make sure that after plugins are loaded, FileWriter can be created.
```
private FileWriter() {
logBuffer = new ArrayBlockingQueue(1024);
final ArrayList<String> outputLogs = new ArrayList<String>(200);
Executors.newSingleThreadScheduledExecutor(new DefaultNamedThreadFactory("LogFileWriter"))
.scheduleAtFixedRate(new RunnableWithExceptionProtection(new Runnable() {
@Override
public void run() {
try {
logBuffer.drainTo(outputLogs);
for (String log : outputLogs) {
writeToFile(log + Constants.LINE_SEPARATOR);
}
try {
fileOutputStream.flush();
} catch (IOException e) {
e.printStackTrace();
}
} finally {
outputLogs.clear();
}
}
}, new RunnableWithExceptionProtection.CallbackWhenException() {
@Override
public void handle(Throwable t) {
}
}), 0, 1, TimeUnit.SECONDS);
}
```
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r847898118
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
One more scenario to discussion,
The ThreadPoolExecutor could be used to run an endless loop task, like a timer. And `run` method would never end.
As you mentioned using the class name to filter in meter collection, should we add this kind of mechanism in the tracing part too? (Have to back about caller detection, as user could use APIs rather than inherit from 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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848054872
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/AbstractThreadingPoolInterceptor.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * 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;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+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 java.lang.reflect.Method;
+
+public abstract class AbstractThreadingPoolInterceptor implements InstanceMethodsAroundInterceptor {
+ @Override
+ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
+ if (!ContextManager.isActive()) {
+ return;
+ }
+
+ if (allArguments == null || allArguments.length < 1) {
+ return;
+ }
+
+ Object argument = allArguments[0];
+
+ Object updateResult = wrap(argument);
+ if (updateResult != null) {
+ allArguments[0] = updateResult;
+ }
+ }
+
+ public abstract Object wrap(Object param);
+
+ @Override
+ public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Object ret) throws Throwable {
+ return ret;
+ }
+
+ @Override
+ public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Throwable t) {
+ if (ContextManager.isActive()) {
Review Comment:
>
Normally, There must be an context.
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848039534
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/ThreadPoolExecuteMethodInterceptor.java:
##########
@@ -0,0 +1,45 @@
+/*
+ * 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;
+
+import org.apache.skywalking.apm.plugin.wrapper.SwRunnableWrapper;
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import java.util.concurrent.RunnableFuture;
+
+public class ThreadPoolExecuteMethodInterceptor extends AbstractThreadingPoolInterceptor {
+
+ @Override
+ public Object wrap(Object param) {
+ if (param instanceof SwRunnableWrapper) {
+ return param;
+ }
+
+ if (param instanceof RunnableFuture) {
+ return null;
Review Comment:
it won't cause NPE. Let's see the caller code in `beforeMethod`.
```
Object updateResult = wrap(argument);
if (updateResult != null) {
allArguments[0] = updateResult;
}
```
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848034668
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/ThreadPoolSubmitMethodInterceptor.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * 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;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.plugin.wrapper.SwCallableWrapper;
+import org.apache.skywalking.apm.plugin.wrapper.SwRunnableWrapper;
+
+import java.util.concurrent.Callable;
+
+public class ThreadPoolSubmitMethodInterceptor extends AbstractThreadingPoolInterceptor {
+
+ @Override
+ public Object wrap(Object param) {
+ if (param instanceof SwRunnableWrapper || param instanceof SwCallableWrapper) {
+ return param;
+ }
+
+ if (param instanceof Callable) {
+ Callable callable = (Callable) param;
+ return new SwCallableWrapper(callable, ContextManager.capture());
+ }
+
+ if (param instanceof Runnable) {
+ Runnable runnable = (Runnable) param;
+ return new SwRunnableWrapper(runnable, ContextManager.capture());
+ }
+
+ return null;
Review Comment:
Same question about `return null`.
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/ThreadPoolExecuteMethodInterceptor.java:
##########
@@ -0,0 +1,45 @@
+/*
+ * 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;
+
+import org.apache.skywalking.apm.plugin.wrapper.SwRunnableWrapper;
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import java.util.concurrent.RunnableFuture;
+
+public class ThreadPoolExecuteMethodInterceptor extends AbstractThreadingPoolInterceptor {
+
+ @Override
+ public Object wrap(Object param) {
+ if (param instanceof SwRunnableWrapper) {
+ return param;
+ }
+
+ if (param instanceof RunnableFuture) {
+ return null;
+ }
+
+ if (!(param instanceof Runnable)) {
+ return null;
Review Comment:
Same question about `return null`.
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848032212
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/define/ThreadPoolExecutorInstrumentation.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.define;
+
+import net.bytebuddy.description.method.MethodDescription;
+import net.bytebuddy.matcher.ElementMatcher;
+import net.bytebuddy.matcher.ElementMatchers;
+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.StaticMethodsInterceptPoint;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.ClassEnhancePluginDefine;
+import org.apache.skywalking.apm.agent.core.plugin.match.ClassMatch;
+import org.apache.skywalking.apm.agent.core.plugin.match.HierarchyMatch;
+import org.apache.skywalking.apm.agent.core.plugin.match.MultiClassNameMatch;
+import org.apache.skywalking.apm.agent.core.plugin.match.logical.LogicalMatchOperation;
+
+public class ThreadPoolExecutorInstrumentation extends ClassEnhancePluginDefine {
+
+ protected static final String ENHANCE_CLASS = "java.util.concurrent.ThreadPoolExecutor";
Review Comment:
```suggestion
private static final String ENHANCE_CLASS = "java.util.concurrent.ThreadPoolExecutor";
```
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r845144553
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
> My focus is ThreadPoolExecutor, not all kinds of TheadPools.
This makes perfect sense to me. We don't need to cover all threads.
> Do you think we just take scheduleAtFixedRate and scheduleWithFixedDelay into account in addition to execute and submit, or cover all kinds of threadpools?
Not all thread pool, just these two popular scheduling ways.
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r845126495
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
<img width="449" alt="image" src="https://user-images.githubusercontent.com/5441976/162208090-da5268fa-95d2-4970-bef7-9c6b90165324.png">
This is available on v9 `General Service/Instance/JVM` panel.
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r845092153
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
> Yes, `ScheduledThreadPool` is inherited from `ThreadPoolExecutor`, also has `execute` and `submit` methods. so I take all the subclasses of `ThreadPoolExecutor` into account, but only `execute` and `submit` methods. I'll reconsider it all
Yes, `ScheduledThreadPool` is inherited from `ThreadPoolExecutor,` also has execute and submit methods. so I take all the subclasses of T`hreadPoolExecutor` into account, but only execute and submit methods. Do you think we just take `scheduleAtFixedRate` and `scheduleWithFixedDelay` into account in addition to `execute` and `submit`, or cover all kinds of threadpools?
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r844920619
##########
test/plugin/scenarios/jdk-threadpool-scenario/bin/startup.sh:
##########
@@ -0,0 +1,21 @@
+#!/bin/bash
+#
+# 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.
+
+home="$(cd "$(dirname $0)"; pwd)"
+
+java -jar ${agent_opts} ${home}/../libs/jdk-threadpool-scenario.jar &
Review Comment:
`plugin-tests-*.yaml` in https://github.com/apache/skywalking-java/tree/main/.github/workflows. Last step in the doc, https://github.com/apache/skywalking-java/blob/main/docs/en/setup/service-agent/java-agent/Plugin-test.md#local-test-and-pull-request-to-the-upstream
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r844991022
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
>
I have considered this case. the instructed point are just`execute` and `submit` methods in `ThreadPoolExecutor` and its subclasses. `FileWriter` and `JVMService` use `scheduleAtFixedRate` method.
If the `execute` method in `ThreadPoolExecutor` subclass calls its super class `execute` method, this will result in `Runnable` will be wrapped twice, but it won't form a `spin-tracing`.
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r845039466
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
>
To be honest, My focus is `ThreadPoolExecutor`, not all kinds of `TheadPool`s. if we want to cover all the `crossThread` situations, I will think it over include `ScheduledThreadPool` ,`ForkJoinPool` and any other customed threadpools. And the instructed point won't be `java.util.concurrent.ThreadPoolExecutor`, it will be `java.util.concurrent.Executor` and `java.util.concurrent.ExecutorService`. it is complex. The cost is relatively large and The risk is relatively high. so I chose the `ThreadPoolExecutor` commonly used by users. WDYT?
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r845104525
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
And, have you known there are meter plugins for thread pool, such as `UndertowWorkerThreadPoolConstructorIntercept`. I think if this plugin covers the thread pool case too, it should report the thread pool metrics 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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r845045453
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/AbstractThreadingPoolInterceptor.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * 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;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+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 java.lang.reflect.Method;
+
+public abstract class AbstractThreadingPoolInterceptor implements InstanceMethodsAroundInterceptor {
+ @Override
+ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
+ if (!ContextManager.isActive()) {
+ return;
+ }
+
+ if (allArguments == null || allArguments.length != 1) {
+ return;
+ }
+ Object argument = allArguments[0];
+
+ Object updateResult = updateOnlyParam(argument);
+ if (updateResult != null) {
+ allArguments[0] = updateResult;
+ }
+ }
+
+ public abstract Object updateOnlyParam(Object param);
Review Comment:
>
it doesn't wrap executor. it just wraps the `Runnable` or `Callable`. we could name it `wrapCommand` or `wrapTask`. WDYT?
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r845039466
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
>
To be honest, My focus is `ThreadPoolExecutor`, not all kinds of 'TheadPool`s
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r844905611
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/conf/SnifferConfigInitializer.java:
##########
@@ -234,4 +235,12 @@ static void configureLogger() {
LogManager.setLogResolver(new PatternLogResolver());
}
}
+
+ public static void pluginInitCompleted() {
+ IS_PLUGIN_INIT_COMPLETED = true;
+ }
+
+ public static boolean isPluginInitCompleted() {
+ return IS_PLUGIN_INIT_COMPLETED;
+ }
Review Comment:
What are these about?
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r847896560
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/define/HierarchyThreadPoolExecutorInstrumentation.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * 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.define;
+
+import org.apache.skywalking.apm.agent.core.plugin.match.ClassMatch;
+import org.apache.skywalking.apm.agent.core.plugin.match.HierarchyMatch;
+
+public class HierarchyThreadPoolExecutorInstrumentation extends AbstractThreadPoolExecutorInstrumentation {
Review Comment:
Consider to use `LogicalOrMatch` to replace this and `ThreadPoolExecutorInstrumentation`?
--
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] Cool-Coding commented on pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#issuecomment-1096161645
> Many NPEs happen in the UT, I think you break something. Please recheck.
I have rechecked it again. There is still one failed that has nothing to do with my modification.
```
ERROR: 1 dead links found!
[✖] https://twitter.com/ASFSkyWalking → Status: 404
```
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848082194
##########
apm-sniffer/apm-agent/src/main/java/org/apache/skywalking/apm/agent/SkyWalkingAgent.java:
##########
@@ -130,6 +130,8 @@ public static void premain(String agentArgs, Instrumentation instrumentation) th
.with(new Listener())
.installOn(instrumentation);
+ SnifferConfigInitializer.pluginInitCompleted();
Review Comment:
>
`PluginFinder` is responsible for loadplugins and matching classes. `AgentBuilder` is responsible for transforming classes. `IS_PLUGIN_INIT_COMPLETED` must be set true after `AgentBuilder. installOn(instrumentation)`. Maybe the variable field name is not very appropriate. ` IS_PLUGIN_INSTALLED_COMPLETED` maybe more appropriate. WDYT?
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848091537
##########
apm-sniffer/apm-agent/src/main/java/org/apache/skywalking/apm/agent/SkyWalkingAgent.java:
##########
@@ -130,6 +130,8 @@ public static void premain(String agentArgs, Instrumentation instrumentation) th
.with(new Listener())
.installOn(instrumentation);
+ SnifferConfigInitializer.pluginInitCompleted();
Review Comment:
> Putting it in the Agent makes sense.
Ok, Then keep it like 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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848195349
##########
apm-sniffer/apm-agent/src/main/java/org/apache/skywalking/apm/agent/SkyWalkingAgent.java:
##########
@@ -130,6 +130,8 @@ public static void premain(String agentArgs, Instrumentation instrumentation) th
.with(new Listener())
.installOn(instrumentation);
+ SnifferConfigInitializer.pluginInitCompleted();
Review Comment:
> Please move and rename this flag, I think others are good to me.
Ok, I understand it. `SnifferConfigInitializer` is just responsible for configuration. I will move it to the `PluginFinder`.
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848040356
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/ThreadPoolExecuteMethodInterceptor.java:
##########
@@ -0,0 +1,45 @@
+/*
+ * 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;
+
+import org.apache.skywalking.apm.plugin.wrapper.SwRunnableWrapper;
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import java.util.concurrent.RunnableFuture;
+
+public class ThreadPoolExecuteMethodInterceptor extends AbstractThreadingPoolInterceptor {
+
+ @Override
+ public Object wrap(Object param) {
+ if (param instanceof SwRunnableWrapper) {
+ return param;
+ }
+
+ if (param instanceof RunnableFuture) {
+ return null;
Review Comment:
OK, please add comments on `AbstractThreadingPoolInterceptor#wrap`.
Also consider to add more comments on codes 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] wu-sheng commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r844941686
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
Are you considering `spin-tracing`? The agent is going to trace its own internal methods? And meanwhile, this method could print logs. You will face endless executions.
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r845092153
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
> Yes, `ScheduledThreadPool` is inherited from `ThreadPoolExecutor`, also has `execute` and `submit` methods. so I take all the subclasses of `ThreadPoolExecutor` into account, but only `execute` and `submit` methods. I'll reconsider it all
Yes, ScheduledThreadPool is inherited from ThreadPoolExecutor, also has execute and submit methods. so I take all the subclasses of ThreadPoolExecutor into account, but only execute and submit methods. Do you think we just take `scheduleAtFixedRate` and `scheduleWithFixedDelay` into account in addition to `execute` and `submit`, or cover all kinds of threadpools?
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r845085294
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
>
Yes, `ScheduledThreadPool` is inherited from `ThreadPoolExecutor`, also has `execute` and `submit` methods. so I take all the subclasses of `ThreadPoolExecutor` into account, but only `execute` and `submit` methods. I'll reconsider it all
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r844932071
##########
test/plugin/scenarios/jdk-threadpool-scenario/bin/startup.sh:
##########
@@ -0,0 +1,21 @@
+#!/bin/bash
+#
+# 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.
+
+home="$(cd "$(dirname $0)"; pwd)"
+
+java -jar ${agent_opts} ${home}/../libs/jdk-threadpool-scenario.jar &
Review Comment:
> `plugin-tests-*.yaml` in https://github.com/apache/skywalking-java/tree/main/.github/workflows. Last step in the doc, https://github.com/apache/skywalking-java/blob/main/docs/en/setup/service-agent/java-agent/Plugin-test.md#local-test-and-pull-request-to-the-upstream
Ok, I will see it. Thank you!
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r845121548
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/AbstractThreadingPoolInterceptor.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * 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;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+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 java.lang.reflect.Method;
+
+public abstract class AbstractThreadingPoolInterceptor implements InstanceMethodsAroundInterceptor {
+ @Override
+ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
+ if (!ContextManager.isActive()) {
+ return;
+ }
+
+ if (allArguments == null || allArguments.length != 1) {
+ return;
+ }
+ Object argument = allArguments[0];
+
+ Object updateResult = updateOnlyParam(argument);
+ if (updateResult != null) {
+ allArguments[0] = updateResult;
+ }
+ }
+
+ public abstract Object updateOnlyParam(Object param);
Review Comment:
> Just `wrap` should be enough, I think.
ok
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r845136231
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
> And, have you known there are meter plugins for thread pool, such as `UndertowWorkerThreadPoolConstructorIntercept`. I think if this plugin covers the thread pool case too, it should report the thread pool metrics too.
I've seen the issue before. I seen it again just now. it also instructs `ThreadPoolExecutor` with the match rule that package name is started with "org.xnio". If my `ThreadPoolExecutor` plugin covers this, the `Undertow ThreadPool` plugin is redundant I think. I will take it into account.
Please take a look at [reply1](#discussion_r845039466), [reply2 ](#discussion_r845092153). I want to know your point of view. Thank you!
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r847916980
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/define/HierarchyThreadPoolExecutorInstrumentation.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * 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.define;
+
+import org.apache.skywalking.apm.agent.core.plugin.match.ClassMatch;
+import org.apache.skywalking.apm.agent.core.plugin.match.HierarchyMatch;
+
+public class HierarchyThreadPoolExecutorInstrumentation extends AbstractThreadPoolExecutorInstrumentation {
Review Comment:
Make sense.
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848018873
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
> One more scenario to discussion,
>
> The ThreadPoolExecutor could be used to run an endless loop task, like a timer. And `run` method would never end. As you mentioned using the class name to filter in meter collection, should we add this kind of mechanism in the tracing part too? (Have to back about caller detection, as user could use APIs rather than inherit from this)
Do you mean we do not trace some `Runnable` or `Callable` tasks if `skywalking` matchs the class name that user config in `agent.config`?
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848062573
##########
apm-sniffer/apm-agent/src/main/java/org/apache/skywalking/apm/agent/SkyWalkingAgent.java:
##########
@@ -130,6 +130,8 @@ public static void premain(String agentArgs, Instrumentation instrumentation) th
.with(new Listener())
.installOn(instrumentation);
+ SnifferConfigInitializer.pluginInitCompleted();
Review Comment:
I think `pluginInitCompleted` should belong to `SnifferConfigInitializer`.
the `IS_PLUGIN_INIT_COMPLETED` field is not a flag that indicates the plugins are loaded. It's the flag that ByteBuddy has loaded all plugins. Before this, `ThreadPoolExecutor` can't be created.
--
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] Cool-Coding commented on pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#issuecomment-1095827353
> It seems some test cases fail, please recheck.
I have rechecked it. It seems that it has nothing to do with my modification. I will recheck it again. please take a look at [my reply](#discussion_r847191002).
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848035917
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/define/ThreadPoolExecutorInstrumentation.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.define;
+
+import net.bytebuddy.description.method.MethodDescription;
+import net.bytebuddy.matcher.ElementMatcher;
+import net.bytebuddy.matcher.ElementMatchers;
+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.StaticMethodsInterceptPoint;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.ClassEnhancePluginDefine;
+import org.apache.skywalking.apm.agent.core.plugin.match.ClassMatch;
+import org.apache.skywalking.apm.agent.core.plugin.match.HierarchyMatch;
+import org.apache.skywalking.apm.agent.core.plugin.match.MultiClassNameMatch;
+import org.apache.skywalking.apm.agent.core.plugin.match.logical.LogicalMatchOperation;
+
+public class ThreadPoolExecutorInstrumentation extends ClassEnhancePluginDefine {
Review Comment:
```suggestion
public class ThreadPoolExecutorInstrumentation extends ClassInstanceMethodsEnhancePluginDefine {
```
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848061742
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/conf/SnifferConfigInitializer.java:
##########
@@ -234,4 +235,12 @@ static void configureLogger() {
LogManager.setLogResolver(new PatternLogResolver());
}
}
+
+ public static void pluginInitCompleted() {
+ IS_PLUGIN_INIT_COMPLETED = true;
+ }
+
+ public static boolean isPluginInitCompleted() {
+ return IS_PLUGIN_INIT_COMPLETED;
+ }
Review Comment:
Same. I am not arguing about the process flow, I am talking about where to hold this flag.
I mean moving this flag to https://github.com/apache/skywalking-java/pull/146#discussion_r848061270
--
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 #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848047512
##########
apm-sniffer/bootstrap-plugins/jdk-threadpool-plugin/src/main/java/org/apache/skywalking/apm/plugin/wrapper/SwRunnableWrapper.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.wrapper;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.ContextSnapshot;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+
+public class SwRunnableWrapper implements Runnable {
+
+ private Runnable runnable;
+
+ private ContextSnapshot contextSnapshot;
+
+ public SwRunnableWrapper(Runnable runnable, ContextSnapshot contextSnapshot) {
+ this.runnable = runnable;
+ this.contextSnapshot = contextSnapshot;
+ }
+
+ @Override
+ public void run() {
+ AbstractSpan span = ContextManager.createLocalSpan(getOperationName());
Review Comment:
No need, please address others. This is good for me now.
--
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] Cool-Coding commented on a diff in pull request #146: [Feature] Add JDK ThreadPoolExecutor plugin(#8743)
Posted by GitBox <gi...@apache.org>.
Cool-Coding commented on code in PR #146:
URL: https://github.com/apache/skywalking-java/pull/146#discussion_r848195349
##########
apm-sniffer/apm-agent/src/main/java/org/apache/skywalking/apm/agent/SkyWalkingAgent.java:
##########
@@ -130,6 +130,8 @@ public static void premain(String agentArgs, Instrumentation instrumentation) th
.with(new Listener())
.installOn(instrumentation);
+ SnifferConfigInitializer.pluginInitCompleted();
Review Comment:
> Please move and rename this flag, I think others are good to me.
Ok, I understand. We were not on the same channel just now. I will move it to the `PluginFinder`.
--
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