From af81bd744c6b292f061efa3039a069ec15e21e6c Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Wed, 17 Apr 2019 16:38:47 +0200 Subject: [PATCH 1/4] Exclude glassfish Executor which does not permit wrapped runnables fixes #589 --- .../apm/agent/concurrent/ExecutorInstrumentation.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/apm-agent-plugins/apm-java-concurrent-plugin/src/main/java/co/elastic/apm/agent/concurrent/ExecutorInstrumentation.java b/apm-agent-plugins/apm-java-concurrent-plugin/src/main/java/co/elastic/apm/agent/concurrent/ExecutorInstrumentation.java index b77e670e5b..d87f447fd8 100644 --- a/apm-agent-plugins/apm-java-concurrent-plugin/src/main/java/co/elastic/apm/agent/concurrent/ExecutorInstrumentation.java +++ b/apm-agent-plugins/apm-java-concurrent-plugin/src/main/java/co/elastic/apm/agent/concurrent/ExecutorInstrumentation.java @@ -61,7 +61,11 @@ public ElementMatcher getTypeMatcherPreFilter() { public ElementMatcher getTypeMatcher() { return hasSuperType(named("java.util.concurrent.Executor")) // hazelcast tries to serialize the Runnables/Callables to execute them on remote JVMs - .and(not(nameStartsWith("com.hazelcast"))); + .and(not(nameStartsWith("com.hazelcast"))) + // this pool relies on the task to be an instance of org.glassfish.enterprise.concurrent.internal.ManagedFutureTask + // the wrapping is done in org.glassfish.enterprise.concurrent.ManagedExecutorServiceImpl.execute + // so this pool only works when called directly from ManagedExecutorServiceImpl + .and(not(named("org.glassfish.enterprise.concurrent.internal.ManagedThreadPoolExecutor"))); } @Override From d2fb57cae1500bfddf04d2143058d3e953088644 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Thu, 18 Apr 2019 16:10:40 +0200 Subject: [PATCH 2/4] Exclude based on class name at runtime --- .../concurrent/ExecutorInstrumentation.java | 24 +++++++++--- .../concurrent/ExcludedExecutorClassTest.java | 37 +++++++++++++++++++ 2 files changed, 56 insertions(+), 5 deletions(-) create mode 100644 apm-agent-plugins/apm-java-concurrent-plugin/src/test/java/co/elastic/apm/agent/concurrent/ExcludedExecutorClassTest.java diff --git a/apm-agent-plugins/apm-java-concurrent-plugin/src/main/java/co/elastic/apm/agent/concurrent/ExecutorInstrumentation.java b/apm-agent-plugins/apm-java-concurrent-plugin/src/main/java/co/elastic/apm/agent/concurrent/ExecutorInstrumentation.java index d87f447fd8..5587e8d4b3 100644 --- a/apm-agent-plugins/apm-java-concurrent-plugin/src/main/java/co/elastic/apm/agent/concurrent/ExecutorInstrumentation.java +++ b/apm-agent-plugins/apm-java-concurrent-plugin/src/main/java/co/elastic/apm/agent/concurrent/ExecutorInstrumentation.java @@ -32,6 +32,8 @@ import javax.annotation.Nullable; import java.util.Arrays; import java.util.Collection; +import java.util.HashSet; +import java.util.Set; import java.util.concurrent.Callable; import java.util.concurrent.Executor; import java.util.concurrent.Future; @@ -48,6 +50,16 @@ public abstract class ExecutorInstrumentation extends ElasticApmInstrumentation @VisibleForAdvice public static final WeakConcurrentSet excluded = new WeakConcurrentSet<>(WeakConcurrentSet.Cleaner.THREAD); + @VisibleForAdvice + public static final Set excludedClasses = new HashSet<>(); + + static { + // this pool relies on the task to be an instance of org.glassfish.enterprise.concurrent.internal.ManagedFutureTask + // the wrapping is done in org.glassfish.enterprise.concurrent.ManagedExecutorServiceImpl.execute + // so this pool only works when called directly from ManagedExecutorServiceImpl + // excluding this class from instrumentation does not work as it inherits the execute and submit methods + excludedClasses.add("org.glassfish.enterprise.concurrent.internal.ManagedThreadPoolExecutor"); + } @Override public ElementMatcher getTypeMatcherPreFilter() { @@ -62,9 +74,6 @@ public ElementMatcher getTypeMatcher() { return hasSuperType(named("java.util.concurrent.Executor")) // hazelcast tries to serialize the Runnables/Callables to execute them on remote JVMs .and(not(nameStartsWith("com.hazelcast"))) - // this pool relies on the task to be an instance of org.glassfish.enterprise.concurrent.internal.ManagedFutureTask - // the wrapping is done in org.glassfish.enterprise.concurrent.ManagedExecutorServiceImpl.execute - // so this pool only works when called directly from ManagedExecutorServiceImpl .and(not(named("org.glassfish.enterprise.concurrent.internal.ManagedThreadPoolExecutor"))); } @@ -73,13 +82,18 @@ public Collection getInstrumentationGroupNames() { return Arrays.asList("concurrent", "executor"); } + @VisibleForAdvice + public static boolean isExcluded(@Advice.This Executor executor) { + return excluded.contains(executor) || excludedClasses.contains(executor.getClass().getName()); + } + public static class ExecutorRunnableInstrumentation extends ExecutorInstrumentation { @Advice.OnMethodEnter(suppress = Throwable.class) public static void onExecute(@Advice.This Executor thiz, @Advice.Argument(value = 0, readOnly = false) @Nullable Runnable runnable, @Advice.Local("original") Runnable original) { final TraceContextHolder active = ExecutorInstrumentation.getActive(); - if (active != null && runnable != null && !excluded.contains(thiz)) { + if (active != null && runnable != null && !isExcluded(thiz)) { original = runnable; runnable = active.withActive(runnable); } @@ -121,7 +135,7 @@ public static void onSubmit(@Advice.This Executor thiz, @Advice.Argument(value = 0, readOnly = false) @Nullable Callable callable, @Advice.Local("original") Callable original) { final TraceContextHolder active = ExecutorInstrumentation.getActive(); - if (active != null && callable != null && !excluded.contains(thiz)) { + if (active != null && callable != null && !isExcluded(thiz)) { original = callable; callable = active.withActive(callable); } diff --git a/apm-agent-plugins/apm-java-concurrent-plugin/src/test/java/co/elastic/apm/agent/concurrent/ExcludedExecutorClassTest.java b/apm-agent-plugins/apm-java-concurrent-plugin/src/test/java/co/elastic/apm/agent/concurrent/ExcludedExecutorClassTest.java new file mode 100644 index 0000000000..a2c3e7609f --- /dev/null +++ b/apm-agent-plugins/apm-java-concurrent-plugin/src/test/java/co/elastic/apm/agent/concurrent/ExcludedExecutorClassTest.java @@ -0,0 +1,37 @@ +package co.elastic.apm.agent.concurrent; + +import co.elastic.apm.agent.AbstractInstrumentationTest; +import co.elastic.apm.agent.impl.transaction.TraceContext; +import co.elastic.apm.agent.impl.transaction.Transaction; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; + +import static org.assertj.core.api.Assertions.assertThat; + +public class ExcludedExecutorClassTest extends AbstractInstrumentationTest { + + private ExecutorService executor; + private Transaction transaction; + + @Before + public void setUp() { + executor = new ExecutorServiceWrapper(Executors.newFixedThreadPool(1)); + ExecutorInstrumentation.excludedClasses.add(ExecutorServiceWrapper.class.getName()); + transaction = tracer.startTransaction(TraceContext.asRoot(), null, null).withName("Transaction").activate(); + } + + @After + public void tearDown() { + transaction.deactivate().end(); + ExecutorInstrumentation.excludedClasses.remove(ExecutorServiceWrapper.class.getName()); + } + + @Test + public void testExecutorExecute() throws Exception { + assertThat(executor.submit(tracer::getActive).get()).isNull(); + } +} From 8e0fc14b1e50cddcc5ba8a51d566681350a67a28 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Thu, 18 Apr 2019 17:57:27 +0200 Subject: [PATCH 3/4] Add license header --- .../concurrent/ExcludedExecutorClassTest.java | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/apm-agent-plugins/apm-java-concurrent-plugin/src/test/java/co/elastic/apm/agent/concurrent/ExcludedExecutorClassTest.java b/apm-agent-plugins/apm-java-concurrent-plugin/src/test/java/co/elastic/apm/agent/concurrent/ExcludedExecutorClassTest.java index a2c3e7609f..ae657ffa0c 100644 --- a/apm-agent-plugins/apm-java-concurrent-plugin/src/test/java/co/elastic/apm/agent/concurrent/ExcludedExecutorClassTest.java +++ b/apm-agent-plugins/apm-java-concurrent-plugin/src/test/java/co/elastic/apm/agent/concurrent/ExcludedExecutorClassTest.java @@ -1,3 +1,22 @@ +/*- + * #%L + * Elastic APM Java agent + * %% + * Copyright (C) 2018 - 2019 Elastic and contributors + * %% + * Licensed 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. + * #L% + */ package co.elastic.apm.agent.concurrent; import co.elastic.apm.agent.AbstractInstrumentationTest; From aa7f50a6090c4eb163ef94b05f34ec0bb4a2c1ab Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Tue, 23 Apr 2019 10:46:41 +0200 Subject: [PATCH 4/4] Remove type matcher, add changelog --- CHANGELOG.md | 1 + .../elastic/apm/agent/concurrent/ExecutorInstrumentation.java | 3 +-- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3048f31092..a64fe7a772 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ## Bug Fixes * Fixes transaction name for non-sampled transactions [#581](https://github.com/elastic/apm-agent-java/issues/581) + * Fixes exceptions when using WildFly managed executor services [#589](https://github.com/elastic/apm-agent-java/issues/589) # 1.6.0 diff --git a/apm-agent-plugins/apm-java-concurrent-plugin/src/main/java/co/elastic/apm/agent/concurrent/ExecutorInstrumentation.java b/apm-agent-plugins/apm-java-concurrent-plugin/src/main/java/co/elastic/apm/agent/concurrent/ExecutorInstrumentation.java index 348192bd1e..70cfe3bded 100644 --- a/apm-agent-plugins/apm-java-concurrent-plugin/src/main/java/co/elastic/apm/agent/concurrent/ExecutorInstrumentation.java +++ b/apm-agent-plugins/apm-java-concurrent-plugin/src/main/java/co/elastic/apm/agent/concurrent/ExecutorInstrumentation.java @@ -75,8 +75,7 @@ public ElementMatcher getTypeMatcher() { // executes on same thread, no need to wrap to activate again .and(not(named("org.apache.felix.resolver.ResolverImpl$DumbExecutor"))) // hazelcast tries to serialize the Runnables/Callables to execute them on remote JVMs - .and(not(nameStartsWith("com.hazelcast"))) - .and(not(named("org.glassfish.enterprise.concurrent.internal.ManagedThreadPoolExecutor"))); + .and(not(nameStartsWith("com.hazelcast"))); } @Override