Require separate advice class (#2101)

Avoids class loading issues when the advice refers to library classes.

This allows to validate the advice class when the instrumentations get initialized
as opposed to on indy bootstrap.
That caches more errors and fails faster.

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

@@ -428,6 +428,7 @@ private static AgentBuilder.Transformer.ForAdvice getTransformer(final ElasticAp
         // external plugins are always indy plugins
         if (!(instrumentation instanceof TracerAwareInstrumentation)
             || ((TracerAwareInstrumentation) instrumentation).indyPlugin()) {
+            validateAdvice(instrumentation);
             withCustomMapping = withCustomMapping.bootstrap(IndyBootstrap.getIndyBootstrapMethod(logger));
         }
         return new AgentBuilder.Transformer.ForAdvice(withCustomMapping)
@@ -458,25 +459,25 @@ public boolean matches(MethodDescription target) {
     }
 
     /**
-     * Validates invariants explained in {@link TracerAwareInstrumentation#indyPlugin()}
-     *
-     * @param adviceClass the advice class
+     * Validates invariants explained in {@link TracerAwareInstrumentation#indyPlugin()} and {@link ElasticApmInstrumentation#getAdviceClassName()}
      */
-    public static void validateAdvice(Class<?> adviceClass) {
-        String adviceClassName = adviceClass.getName();
-        ClassLoader classLoader = adviceClass.getClassLoader();
-        if (classLoader == null) {
-            classLoader = ClassLoader.getSystemClassLoader();
-        }
-
-        int adviceModifiers = adviceClass.getModifiers();
+    public static void validateAdvice(ElasticApmInstrumentation instrumentation) {
+        String adviceClassName = instrumentation.getAdviceClassName();
+        if (instrumentation.getClass().getName().equals(adviceClassName)) {
+            throw new IllegalStateException("The advice must be declared in a separate class: " + adviceClassName);
+        }
+        ClassLoader adviceClassLoader = instrumentation.getClass().getClassLoader();
+        if (adviceClassLoader == null) {
+            // the bootstrap class loader can't do resource lookup
+            // if classes are added via java.lang.instrument.Instrumentation.appendToBootstrapClassLoaderSearch
+            adviceClassLoader = ClassLoader.getSystemClassLoader();
+        }
+        TypePool pool = new TypePool.Default.WithLazyResolution(TypePool.CacheProvider.NoOp.INSTANCE, ClassFileLocator.ForClassLoader.of(adviceClassLoader), TypePool.Default.ReaderMode.FAST);
+        TypeDescription typeDescription = pool.describe(adviceClassName).resolve();
+        int adviceModifiers = typeDescription.getModifiers();
         if (!Modifier.isPublic(adviceModifiers)) {
             throw new IllegalStateException(String.format("advice class %s should be public", adviceClassName));
         }
-
-        TypePool pool = new TypePool.Default.WithLazyResolution(TypePool.CacheProvider.NoOp.INSTANCE, ClassFileLocator.ForClassLoader.of(classLoader), 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, adviceClassName);
 
@@ -493,7 +494,7 @@ public static void validateAdvice(Class<?> adviceClass) {
                 checkInline(exitAdvice, adviceClassName, exit.prepare(Advice.OnMethodExit.class).load().inline());
             }
         }
-        if (!(classLoader instanceof ExternalPluginClassLoader) && !adviceClassName.startsWith("co.elastic.apm.agent.")) {
+        if (!(adviceClassLoader instanceof ExternalPluginClassLoader) && !adviceClassName.startsWith("co.elastic.apm.agent.")) {
             throw new IllegalStateException(String.format(
                 "Invalid Advice class - %s - Indy-dispatched advice class must be in a sub-package of 'co.elastic.apm.agent'.",
                 adviceClassName)

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

@@ -355,7 +355,6 @@ public static ConstantCallSite bootstrap(MethodHandles.Lookup lookup,
                     // tracer.getConfig(Config.class) would return null when called from an advice as the classes are not the same
                     .or(nameContains("Config").and(hasSuperType(is(ConfigurationOptionProvider.class)))));
             Class<?> adviceInPluginCL = pluginClassLoader.loadClass(adviceClassName);
-            ElasticApmAgent.validateAdvice(adviceInPluginCL);
             Class<LookupExposer> lookupExposer = (Class<LookupExposer>) pluginClassLoader.loadClass(LOOKUP_EXPOSER_CLASS_NAME);
             // can't use MethodHandle.lookup(), see also https://github.com/elastic/apm-agent-java/issues/1450
             MethodHandles.Lookup indyLookup = (MethodHandles.Lookup) lookupExposer.getMethod("getLookup").invoke(null);

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

@@ -28,7 +28,6 @@
  */
 public abstract class TracerAwareInstrumentation extends ElasticApmInstrumentation {
 
-    @VisibleForAdvice
     public static final Tracer tracer = GlobalTracer.get();
 
     /**

apm-agent-core/src/main/java/co/elastic/apm/agent/configuration/MessagingConfiguration.java

@@ -18,7 +18,6 @@
  */
 package co.elastic.apm.agent.configuration;
 
-import co.elastic.apm.agent.bci.VisibleForAdvice;
 import co.elastic.apm.agent.matcher.WildcardMatcher;
 import co.elastic.apm.agent.matcher.WildcardMatcherValueConverter;
 import org.stagemonitor.configuration.ConfigurationOption;
@@ -88,17 +87,14 @@ public List<WildcardMatcher> getIgnoreMessageQueues() {
         return ignoreMessageQueues.get();
     }
 
-    @VisibleForAdvice
     public boolean shouldCollectQueueAddress() {
         return collectQueueAddress.get();
     }
 
-    @VisibleForAdvice
     public boolean shouldEndMessagingTransactionOnPoll() {
         return endMessagingTransactionOnPoll.get();
     }
 
-    @VisibleForAdvice
     public enum Strategy {
         POLLING,
         HANDLING,

apm-agent-core/src/main/java/co/elastic/apm/agent/util/IOUtils.java

@@ -18,7 +18,6 @@
  */
 package co.elastic.apm.agent.util;
 
-import co.elastic.apm.agent.bci.VisibleForAdvice;
 
 import java.io.File;
 import java.io.FileInputStream;
@@ -38,7 +37,6 @@
 import java.security.MessageDigest;
 import java.security.NoSuchAlgorithmException;
 
-@VisibleForAdvice
 public class IOUtils {
     protected static final int BYTE_BUFFER_CAPACITY = 2048;
     protected static final ThreadLocal<ByteBuffer> threadLocalByteBuffer = new ThreadLocal<ByteBuffer>() {
@@ -69,7 +67,6 @@ protected CharsetDecoder initialValue() {
      * @return {@code true}, if the input stream could be decoded with the UTF-8 charset, {@code false} otherwise.
      * @throws IOException in case of errors reading from the provided {@link InputStream}
      */
-    @VisibleForAdvice
     public static boolean readUtf8Stream(final InputStream is, final CharBuffer charBuffer) throws IOException {
         // to be compatible with Java 8, we have to cast to buffer because of different return types
         final ByteBuffer buffer = threadLocalByteBuffer.get();
@@ -119,7 +116,6 @@ public static boolean readUtf8Stream(final InputStream is, final CharBuffer char
      * @param charBuffer the {@link CharBuffer} the {@link InputStream} should be written into
      * @return a {@link CoderResult}, indicating the success or failure of the decoding
      */
-    @VisibleForAdvice
     public static CoderResult decodeUtf8Bytes(final byte[] bytes, final CharBuffer charBuffer) {
         return decodeUtf8Bytes(bytes, 0, bytes.length, charBuffer);
     }
@@ -146,7 +142,6 @@ public static CoderResult decodeUtf8Bytes(final byte[] bytes, final CharBuffer c
      * @param length     the maximum number of bytes to read
      * @return a {@link CoderResult}, indicating the success or failure of the decoding
      */
-    @VisibleForAdvice
     public static CoderResult decodeUtf8Bytes(final byte[] bytes, final int offset, final int length, final CharBuffer charBuffer) {
         // to be compatible with Java 8, we have to cast to buffer because of different return types
         final ByteBuffer buffer;
@@ -182,7 +177,6 @@ public static CoderResult decodeUtf8Bytes(final byte[] bytes, final int offset,
      * @param charBuffer the {@link CharBuffer} the {@link InputStream} should be written into
      * @return a {@link CoderResult}, indicating the success or failure of the decoding
      */
-    @VisibleForAdvice
     public static CoderResult decodeUtf8Byte(final byte b, final CharBuffer charBuffer) {
         // to be compatible with Java 8, we have to cast to buffer because of different return types
         final ByteBuffer buffer = threadLocalByteBuffer.get();