Exclude glassfish Executor which does not permit wrapped runnables

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<Executor> excluded = new WeakConcurrentSet<>(WeakConcurrentSet.Cleaner.THREAD);
+    @VisibleForAdvice
+    public static final Set<String> 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<? super NamedElement> getTypeMatcherPreFilter() {
@@ -61,21 +73,27 @@ public ElementMatcher<? super NamedElement> getTypeMatcherPreFilter() {
     public ElementMatcher<? super TypeDescription> 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")))
+            .and(not(named("org.glassfish.enterprise.concurrent.internal.ManagedThreadPoolExecutor")));
     }
 
     @Override
     public Collection<String> 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);
             }
@@ -117,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);
             }

apm-agent-plugins/apm-java-concurrent-plugin/src/test/java/co/elastic/apm/agent/concurrent/ExcludedExecutorClassTest.java

@@ -0,0 +1,56 @@
+/*-
+ * #%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;
+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();
+    }
+}