Indy plugins

apm-agent-core/src/main/java/co/elastic/apm/agent/bci/ElasticApmAgent.java

@@ -52,6 +52,7 @@
 import net.bytebuddy.description.NamedElement;
 import net.bytebuddy.description.annotation.AnnotationDescription;
 import net.bytebuddy.description.method.MethodDescription;
+import net.bytebuddy.description.method.ParameterDescription;
 import net.bytebuddy.description.type.TypeDescription;
 import net.bytebuddy.dynamic.ClassFileLocator;
 import net.bytebuddy.dynamic.DynamicType;
@@ -91,14 +92,12 @@
 import static net.bytebuddy.asm.Advice.ExceptionHandler.Default.PRINTING;
 import static net.bytebuddy.matcher.ElementMatchers.any;
 import static net.bytebuddy.matcher.ElementMatchers.is;
-import static net.bytebuddy.matcher.ElementMatchers.isAnnotatedWith;
 import static net.bytebuddy.matcher.ElementMatchers.isAbstract;
+import static net.bytebuddy.matcher.ElementMatchers.isAnnotatedWith;
 import static net.bytebuddy.matcher.ElementMatchers.isInterface;
 import static net.bytebuddy.matcher.ElementMatchers.isStatic;
 import static net.bytebuddy.matcher.ElementMatchers.nameContains;
-import static net.bytebuddy.matcher.ElementMatchers.nameEndsWith;
 import static net.bytebuddy.matcher.ElementMatchers.nameStartsWith;
-import static net.bytebuddy.matcher.ElementMatchers.named;
 import static net.bytebuddy.matcher.ElementMatchers.none;
 import static net.bytebuddy.matcher.ElementMatchers.not;
 
@@ -350,32 +349,46 @@ public boolean matches(MethodDescription target) {
 
     /**
      * Validates invariants explained in {@link ElasticApmInstrumentation#indyPlugin()}
+     *
      * @param adviceClassName the name of the advice class
      */
     private static void validateAdvice(String adviceClassName) {
-        validateAdviceIsInlined(adviceClassName);
-        if (adviceClassName.startsWith("co.elastic.apm.agent.") && adviceClassName.split("\\.").length > 6) {
-            throw new IllegalStateException("Indy-dispatched advice class must be at the root of the instrumentation plugin.");
-        }
-    }
-
-    private static void validateAdviceIsInlined(String adviceClassName) {
         TypePool pool = new TypePool.Default(TypePool.CacheProvider.NoOp.INSTANCE, ClassFileLocator.ForClassLoader.ofSystemLoader(), TypePool.Default.ReaderMode.FAST);
         TypeDescription typeDescription = pool.describe(adviceClassName).resolve();
         for (MethodDescription.InDefinedShape enterAdvice : typeDescription.getDeclaredMethods().filter(isStatic().and(isAnnotatedWith(Advice.OnMethodEnter.class)))) {
+            validateAdviceReturnAndParameterTypes(enterAdvice);
+
             for (AnnotationDescription enter : enterAdvice.getDeclaredAnnotations().filter(ElementMatchers.annotationType(Advice.OnMethodEnter.class))) {
                 if (enter.prepare(Advice.OnMethodEnter.class).load().inline()) {
                     throw new IllegalStateException(String.format("Indy-dispatched advice %s#%s has to be declared with inline=false", adviceClassName, enterAdvice.getName()));
                 }
             }
         }
         for (MethodDescription.InDefinedShape exitAdvice : typeDescription.getDeclaredMethods().filter(isStatic().and(isAnnotatedWith(Advice.OnMethodExit.class)))) {
+            validateAdviceReturnAndParameterTypes(exitAdvice);
+            if (exitAdvice.getReturnType().getTypeName().startsWith("co.elastic.apm")) {
+                throw new IllegalStateException("Advice return type must be visible from the bootstrap class loader and must not be an agent type.");
+            }
             for (AnnotationDescription exit : exitAdvice.getDeclaredAnnotations().filter(ElementMatchers.annotationType(Advice.OnMethodExit.class))) {
                 if (exit.prepare(Advice.OnMethodExit.class).load().inline()) {
                     throw new IllegalStateException(String.format("Indy-dispatched advice %s#%s has to be declared with inline=false", adviceClassName, exitAdvice.getName()));
                 }
             }
         }
+        if (adviceClassName.startsWith("co.elastic.apm.agent.") && adviceClassName.split("\\.").length > 6) {
+            throw new IllegalStateException("Indy-dispatched advice class must be at the root of the instrumentation plugin.");
+        }
+    }
+
+    private static void validateAdviceReturnAndParameterTypes(MethodDescription.InDefinedShape advice) {
+        if (advice.getReturnType().getTypeName().startsWith("co.elastic.apm")) {
+            throw new IllegalStateException("Advice return type must not be an agent type: " + advice.toGenericString());
+        }
+        for (ParameterDescription.InDefinedShape parameter : advice.getParameters()) {
+            if (parameter.getName().startsWith("co.elastic.apm")) {
+                throw new IllegalStateException("Advice parameters must not contain an agent type: " + advice.toGenericString());
+            }
+        }
     }
 
     private static MatcherTimer getOrCreateTimer(Class<? extends ElasticApmInstrumentation> adviceClass) {
@@ -493,32 +506,6 @@ public Iterable<? extends List<Class<?>>> onError(int index, List<Class<?>> batc
             // avoids instrumenting classes from helper class loaders
             .or(any(), classLoaderWithName(ByteArrayClassLoader.ChildFirst.class.getName()))
             .or(any(), classLoaderWithName("org.codehaus.groovy.runtime.callsite.CallSiteClassLoader"))
-            // ideally, those bootstrap classpath inclusions should be set at plugin level, see issue #952
-            .or(nameStartsWith("java.")
-                .and(
-                    not(
-                        nameEndsWith("URLConnection")
-                            .or(nameStartsWith("java.util.concurrent."))
-                            .or(named("java.lang.ProcessBuilder"))
-                            .or(named("java.lang.ProcessImpl"))
-                            .or(named("java.lang.Process"))
-                            .or(named("java.lang.UNIXProcess"))
-                    )
-                )
-            )
-            .or(nameStartsWith("com.sun.")
-                .and(
-                    not(
-                        nameStartsWith("com.sun.faces.")
-                            .or(nameEndsWith("URLConnection"))
-                    )
-                )
-            )
-            .or(nameStartsWith("sun")
-                .and(
-                    not(nameEndsWith("URLConnection"))
-                )
-            )
             .or(nameStartsWith("co.elastic.apm.agent.shaded"))
             .or(nameStartsWith("org.aspectj."))
             .or(nameStartsWith("org.groovy."))

apm-agent-core/src/main/java/co/elastic/apm/agent/bci/ElasticApmInstrumentation.java

@@ -206,8 +206,25 @@ public void onTypeMatch(TypeDescription typeDescription, ClassLoader classLoader
      * </p>
      * <p>
      * Things to watch out for when using indy plugins:
+     * </p>
      * <ul>
      *     <li>
+     *         Set {@link Advice.OnMethodEnter#inline()} and {@link Advice.OnMethodExit#inline()} to {@code false} on all advices.
+     *         As the {@code readOnly} flag in Byte Buddy annotations such as {@link Advice.Return#readOnly()} cannot be used with non
+     *         {@linkplain Advice.OnMethodEnter#inline() inlined advices},
+     *         use {@link co.elastic.apm.agent.bci.bytebuddy.postprocessor.AssignTo} and friends.
+     *     </li>
+     *     <li>
+     *         Both the return type and the arguments of advice methods must no contain types from the agent.
+     *         If you'd like to return a {@link Span} from an advice, for example, return an {@link Object} instead.
+     *         When using an {@link net.bytebuddy.asm.Advice.Enter} argument on the
+     *         {@linkplain net.bytebuddy.asm.Advice.OnMethodExit exit advice},
+     *         that argument als has to be of type {@link Object} and you have to cast in within the method body.
+     *         The reason is that the return value will become a local variable in the instrumented method.
+     *         Due to OSGi, those methods may not have access to agent types.
+     *         Another case is when the instrumented class is inside the bootstrap classloader.
+     *     </li>
+     *     <li>
      *         When an advice instruments classes in multiple class loaders, the plugin classes will be loaded form multiple class loaders.
      *         In order to still share state across those plugin class loaders, use {@link co.elastic.apm.agent.util.GlobalVariables} or {@link GlobalState}.
      *         That's necessary as a static variables are scoped to the class loader they are defined in.
@@ -222,14 +239,7 @@ public void onTypeMatch(TypeDescription typeDescription, ClassLoader classLoader
      *         As the package of the {@link #getAdviceClass()} is used as the root,
      *         all advices have to be at the top level of the plugin.
      *     </li>
-     *     <li>
-     *         Set {@link Advice.OnMethodEnter#inline()} and {@link Advice.OnMethodExit#inline()} to {@code false} on all advices.
-     *         As the {@code readOnly} flag in Byte Buddy annotations such as {@link Advice.Return#readOnly()} cannot be used with non
-     *         {@linkplain Advice.OnMethodEnter#inline() inlined advices},
-     *         use {@link co.elastic.apm.agent.bci.bytebuddy.postprocessor.AssignTo} and friends.
-     *     </li>
      * </ul>
-     * </p>
      *
      * @return whether to load the classes of this plugin in dedicated plugin class loaders (one for each unique class loader)
      * and dispatch to the {@linkplain #getAdviceClass() advice} via an {@code INVOKEDYNAMIC} instruction.

apm-agent-core/src/main/java/co/elastic/apm/agent/bci/IndyBootstrap.java

@@ -42,6 +42,7 @@
 import java.util.concurrent.ConcurrentMap;
 
 import static net.bytebuddy.matcher.ElementMatchers.isAnnotatedWith;
+import static net.bytebuddy.matcher.ElementMatchers.nameStartsWith;
 import static net.bytebuddy.matcher.ElementMatchers.named;
 
 /**
@@ -236,7 +237,10 @@ public static ConstantCallSite bootstrap(MethodHandles.Lookup lookup,
         ClassLoader pluginClassLoader = HelperClassManager.ForIndyPlugin.getOrCreatePluginClassLoader(
             lookup.lookupClass().getClassLoader(),
             pluginClasses,
-            isAnnotatedWith(named(GlobalState.class.getName())));
+            isAnnotatedWith(named(GlobalState.class.getName()))
+                // no plugin CL necessary as all types are available form bootstrap CL
+                // also, this plugin is used as a dependency in other plugins
+                .or(nameStartsWith("co.elastic.apm.agent.concurrent")));
         Class<?> adviceInPluginCL = pluginClassLoader.loadClass(adviceClassName);
         MethodHandle methodHandle = MethodHandles.lookup().findStatic(adviceInPluginCL, adviceMethodName, adviceMethodType);
         return new ConstantCallSite(methodHandle);

apm-agent-core/src/main/java/co/elastic/apm/agent/bci/methodmatching/TraceMethodInstrumentation.java

@@ -51,6 +51,7 @@
 import static net.bytebuddy.matcher.ElementMatchers.isSynthetic;
 import static net.bytebuddy.matcher.ElementMatchers.isTypeInitializer;
 import static net.bytebuddy.matcher.ElementMatchers.nameContains;
+import static net.bytebuddy.matcher.ElementMatchers.nameStartsWith;
 import static net.bytebuddy.matcher.ElementMatchers.not;
 import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
 import static net.bytebuddy.matcher.ElementMatchers.takesArguments;
@@ -110,6 +111,10 @@ public ElementMatcher<? super TypeDescription> getTypeMatcher() {
             .and(not(nameContains("CGLIB")))
             .and(not(nameContains("EnhancerBy")))
             .and(not(nameContains("$Proxy")))
+            .and(not(nameStartsWith("java.")))
+            .and(not(nameStartsWith("com.sun.")))
+            .and(not(nameStartsWith("sun.")))
+            .and(not(nameStartsWith("jdk.")))
             .and(declaresMethod(matches(methodMatcher.getMethodMatcher())));
     }
 

apm-agent-core/src/main/resources/bootstrap/IndyBootstrapDispatcher.java

@@ -15,7 +15,7 @@ public class IndyBootstrapDispatcher {
 
     static {
         try {
-            VOID_NOOP = MethodHandles.lookup().findStatic(IndyBootstrapDispatcher.class, "voidNoop", MethodType.methodType(void.class));
+            VOID_NOOP = MethodHandles.publicLookup().findStatic(IndyBootstrapDispatcher.class, "voidNoop", MethodType.methodType(void.class));
         } catch (Exception e) {
             throw new RuntimeException(e);
         }

apm-agent-core/src/test/java/co/elastic/apm/agent/util/CallDepthTest.java

@@ -1,3 +1,27 @@
+/*-
+ * #%L
+ * Elastic APM Java agent
+ * %%
+ * Copyright (C) 2018 - 2020 Elastic and contributors
+ * %%
+ * Licensed to Elasticsearch B.V. under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch B.V. 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.
+ * #L%
+ */
 package co.elastic.apm.agent.util;
 
 import org.junit.jupiter.api.AfterEach;

apm-agent-core/src/test/resources/elasticapm.properties

@@ -1,6 +1,4 @@
 # experimental instrumentations should be active in tests
 disable_instrumentations=
-# in unit tests, the agent is loaded by the system class loader so we can't instrument bootstrap classes
-classes_excluded_from_instrumentation=java.*,com.sun.*,sun.*
 application_packages=co.elastic.apm
 ship_agent_logs=false

apm-agent-plugins/apm-dubbo-plugin/src/test/java/co/elastic/apm/agent/dubbo/ApacheDubboInstrumentationTest.java

@@ -37,7 +37,6 @@
 import org.apache.dubbo.config.RegistryConfig;
 import org.apache.dubbo.config.ServiceConfig;
 import org.apache.dubbo.rpc.RpcContext;
-import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 
 import java.util.LinkedList;
@@ -150,9 +149,6 @@ public void testAsyncException() throws Exception {
     }
 
     @Test
-    @Disabled("The new executor instrumentation doesn't wrap Runnables anymore. " +
-        "In this case, the Runnable is java.util.concurrent.CompletableFuture.AsyncSupply. " +
-        "Currently, we can't instrument java.* classes in unit tests (this will change with indy plugins).")
     public void testAsyncByFuture() throws Exception {
         DubboTestApi dubboTestApi = getDubboTestApi();
         String arg = "hello";
@@ -168,9 +164,6 @@ public void testAsyncByFuture() throws Exception {
     }
 
     @Test
-    @Disabled("The new executor instrumentation doesn't wrap Runnables anymore. " +
-        "In this case, the Runnable is java.util.concurrent.CompletableFuture.AsyncSupply. " +
-        "Currently, we can't instrument java.* classes in unit tests (this will change with indy plugins).")
     public void testAsyncByFutureException() throws Exception {
         DubboTestApi dubboTestApi = getDubboTestApi();
         String arg = "error";

apm-agent-plugins/apm-dubbo-plugin/src/test/java/co/elastic/apm/agent/dubbo/api/impl/DubboTestApiImpl.java

@@ -24,7 +24,6 @@
  */
 package co.elastic.apm.agent.dubbo.api.impl;
 
-import co.elastic.apm.agent.dubbo.ExecutorServiceWrapper;
 import co.elastic.apm.agent.dubbo.api.DubboTestApi;
 import co.elastic.apm.agent.dubbo.api.exception.BizException;
 import com.github.tomakehurst.wiremock.WireMockServer;
@@ -64,7 +63,7 @@ public class DubboTestApiImpl implements DubboTestApi {
 
     public DubboTestApiImpl() {
         client = new OkHttpClient();
-        executorService = new ExecutorServiceWrapper(Executors.newSingleThreadExecutor());
+        executorService = Executors.newSingleThreadExecutor();
     }
 
     @Override