make servlet3 + spring webmvc + jaxrs 2.0 indy compatible (#12432)

open-telemetry · Nov 30, 2024 · 7c82dc4 · 7c82dc4
1 parent 219149f
commit 7c82dc4
Show file tree

Hide file tree

Showing 21 changed files with 371 additions and 64 deletions.
diff --git a/...reflection/javaagent-integration-tests/src/main/java/instrumentation/TestHelperClass.java b/...reflection/javaagent-integration-tests/src/main/java/instrumentation/TestHelperClass.java
@@ -0,0 +1,15 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package instrumentation;
+
+/**
+ * Class that will be injected in target classloader with inline instrumentation and proxied with
+ * indy instrumentation
+ */
+public class TestHelperClass {
+
+  public TestHelperClass() {}
+}
diff --git a/.../javaagent-integration-tests/src/main/java/instrumentation/TestInstrumentationModule.java b/.../javaagent-integration-tests/src/main/java/instrumentation/TestInstrumentationModule.java
@@ -10,10 +10,15 @@
 import com.google.auto.service.AutoService;
 import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
+import io.opentelemetry.javaagent.extension.instrumentation.internal.ExperimentalInstrumentationModule;
+import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.ClassInjector;
+import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.InjectionMode;
+import java.util.Arrays;
 import java.util.List;
 
 @AutoService(InstrumentationModule.class)
-public class TestInstrumentationModule extends InstrumentationModule {
+public class TestInstrumentationModule extends InstrumentationModule
+    implements ExperimentalInstrumentationModule {
   public TestInstrumentationModule() {
     super("test-instrumentation");
   }
@@ -22,4 +27,17 @@ public TestInstrumentationModule() {
   public List<TypeInstrumentation> typeInstrumentations() {
     return singletonList(new TestTypeInstrumentation());
   }
+
+  @Override
+  public List<String> getAdditionalHelperClassNames() {
+    // makes the class from instrumentation from the instrumented class with inlined advice
+    return Arrays.asList("instrumentation.TestHelperClass");
+  }
+
+  @Override
+  public void injectClasses(ClassInjector injector) {
+    injector
+        .proxyBuilder("instrumentation.TestHelperClass")
+        .inject(InjectionMode.CLASS_AND_RESOURCE);
+  }
 }
diff --git a/...nternal/internal-reflection/javaagent-integration-tests/src/test/java/ReflectionTest.java b/...nternal/internal-reflection/javaagent-integration-tests/src/test/java/ReflectionTest.java
@@ -5,6 +5,8 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 
+import instrumentation.TestHelperClass;
+import io.opentelemetry.javaagent.bootstrap.InstrumentationProxy;
 import io.opentelemetry.javaagent.bootstrap.VirtualFieldAccessorMarker;
 import io.opentelemetry.javaagent.bootstrap.VirtualFieldInstalledMarker;
 import java.io.ObjectStreamClass;
@@ -42,7 +44,45 @@ void testOurFieldsAndMethodsAreNotVisibleWithReflection() {
   void testGeneratedSerialVersionUid() {
     // expected value is computed with serialver utility that comes with jdk
     assertThat(ObjectStreamClass.lookup(TestClass.class).getSerialVersionUID())
-        .isEqualTo(-1508684692096503670L);
+        .isEqualTo(-4292813100633930936L);
     assertThat(TestClass.class.getDeclaredFields().length).isEqualTo(0);
   }
+
+  @Test
+  void testInjectedClassProxyUnwrap() throws Exception {
+    TestClass testClass = new TestClass();
+    Class<?> helperType = testClass.testHelperClass();
+    assertThat(helperType)
+        .describedAs("unable to resolve injected class from instrumented class")
+        .isNotNull();
+
+    Object instance = helperType.getConstructor().newInstance();
+    if (InstrumentationProxy.class.isAssignableFrom(helperType)) {
+      // indy advice: must be an indy proxy
+
+      for (Method method : helperType.getMethods()) {
+        assertThat(method.getName())
+            .describedAs("proxy method must be hidden from reflection")
+            .isNotEqualTo("__getIndyProxyDelegate");
+      }
+
+      for (Class<?> interfaceType : helperType.getInterfaces()) {
+        assertThat(interfaceType)
+            .describedAs("indy proxy interface must be hidden from reflection")
+            .isNotEqualTo(InstrumentationProxy.class);
+      }
+
+      assertThat(instance).isInstanceOf(InstrumentationProxy.class);
+
+      Object proxyDelegate = ((InstrumentationProxy) instance).__getIndyProxyDelegate();
+      assertThat(proxyDelegate).isNotInstanceOf(InstrumentationProxy.class);
+
+    } else {
+      // inline advice: must be of the expected type
+      assertThat(helperType).isEqualTo(TestHelperClass.class);
+      assertThat(instance)
+          .isInstanceOf(TestHelperClass.class)
+          .isNotInstanceOf(InstrumentationProxy.class);
+    }
+  }
 }
diff --git a/...ion/internal/internal-reflection/javaagent-integration-tests/src/test/java/TestClass.java b/...ion/internal/internal-reflection/javaagent-integration-tests/src/test/java/TestClass.java
@@ -17,4 +17,13 @@ public String testMethod() {
   public String testMethod2() {
     return "not instrumented";
   }
+
+  public Class<?> testHelperClass() {
+    try {
+      return Class.forName(
+          "instrumentation.TestHelperClass", false, TestClass.class.getClassLoader());
+    } catch (ClassNotFoundException e) {
+      return null;
+    }
+  }
 }
diff --git a/...java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ReflectionHelper.java b/...java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ReflectionHelper.java
@@ -5,6 +5,7 @@
 
 package io.opentelemetry.javaagent.instrumentation.internal.reflection;
 
+import io.opentelemetry.javaagent.bootstrap.InstrumentationProxy;
 import io.opentelemetry.javaagent.bootstrap.VirtualFieldAccessorMarker;
 import io.opentelemetry.javaagent.bootstrap.VirtualFieldDetector;
 import io.opentelemetry.javaagent.bootstrap.VirtualFieldInstalledMarker;
@@ -37,18 +38,20 @@ public static Field[] filterFields(Class<?> containingClass, Field[] fields) {
   }
 
   public static Method[] filterMethods(Class<?> containingClass, Method[] methods) {
-    if (methods.length == 0
-        || !VirtualFieldInstalledMarker.class.isAssignableFrom(containingClass)) {
-      // nothing to filter when class does not have any added virtual fields
+    // nothing to filter when class does not have any added virtual fields or is not a proxy
+    if (methods.length == 0 || noInterfaceToHide(containingClass)) {
       return methods;
     }
     List<Method> result = new ArrayList<>(methods.length);
     for (Method method : methods) {
-      // virtual field accessor methods are marked as synthetic
-      if (method.isSynthetic()
-          && (method.getName().startsWith("__get__opentelemetryVirtualField$")
-              || method.getName().startsWith("__set__opentelemetryVirtualField$"))) {
-        continue;
+      // virtual field accessor or proxy methods are marked as synthetic
+      if (method.isSynthetic()) {
+        String name = method.getName();
+        if ((name.startsWith("__get__opentelemetryVirtualField$")
+            || name.startsWith("__set__opentelemetryVirtualField$")
+            || name.equals("__getIndyProxyDelegate"))) {
+          continue;
+        }
       }
       result.add(method);
     }
@@ -57,9 +60,8 @@ public static Method[] filterMethods(Class<?> containingClass, Method[] methods)
 
   @SuppressWarnings("unused")
   public static Class<?>[] filterInterfaces(Class<?>[] interfaces, Class<?> containingClass) {
-    if (interfaces.length == 0
-        || !VirtualFieldInstalledMarker.class.isAssignableFrom(containingClass)) {
-      // nothing to filter when class does not have any added virtual fields
+    // nothing to filter when class does not have any added virtual fields or is not a proxy
+    if (interfaces.length == 0 || noInterfaceToHide(containingClass)) {
       return interfaces;
     }
     List<Class<?>> result = new ArrayList<>(interfaces.length);
@@ -69,6 +71,8 @@ public static Class<?>[] filterInterfaces(Class<?>[] interfaces, Class<?> contai
       // filter out virtual field marker and accessor interfaces
       if (interfaceClass == VirtualFieldInstalledMarker.class) {
         continue;
+      } else if (interfaceClass == InstrumentationProxy.class) {
+        continue;
       } else if (VirtualFieldAccessorMarker.class.isAssignableFrom(interfaceClass)
           && interfaceClass.isSynthetic()
           && interfaceClass.getName().contains("VirtualFieldAccessor$")) {
@@ -77,11 +81,14 @@ public static Class<?>[] filterInterfaces(Class<?>[] interfaces, Class<?> contai
       }
       result.add(interfaceClass);
     }
-
     if (!virtualFieldClassNames.isEmpty()) {
       VirtualFieldDetector.markVirtualFields(containingClass, virtualFieldClassNames);
     }
-
     return result.toArray(new Class<?>[0]);
   }
+
+  private static boolean noInterfaceToHide(Class<?> containingClass) {
+    return !VirtualFieldInstalledMarker.class.isAssignableFrom(containingClass)
+        && !InstrumentationProxy.class.isAssignableFrom(containingClass);
+  }
 }
diff --git a/...va/io/opentelemetry/javaagent/instrumentation/jaxrs/v2_0/JerseyInstrumentationModule.java b/...va/io/opentelemetry/javaagent/instrumentation/jaxrs/v2_0/JerseyInstrumentationModule.java
@@ -11,11 +11,13 @@
 import com.google.auto.service.AutoService;
 import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
+import io.opentelemetry.javaagent.extension.instrumentation.internal.ExperimentalInstrumentationModule;
 import java.util.List;
 import net.bytebuddy.matcher.ElementMatcher;
 
 @AutoService(InstrumentationModule.class)
-public class JerseyInstrumentationModule extends InstrumentationModule {
+public class JerseyInstrumentationModule extends InstrumentationModule
+    implements ExperimentalInstrumentationModule {
   public JerseyInstrumentationModule() {
     super("jaxrs", "jaxrs-2.0", "jersey", "jersey-2.0");
   }
@@ -26,11 +28,9 @@ public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
   }
 
   @Override
-  public boolean isIndyModule() {
-    // net.bytebuddy.pool.TypePool$Resolution$NoSuchTypeException: Cannot resolve type description
-    // for
-    // io.opentelemetry.javaagent.instrumentation.servlet.v3_0.snippet.Servlet3SnippetInjectingResponseWrapper
-    return false;
+  public String getModuleGroup() {
+    // depends on Servlet3SnippetInjectingResponseWrapper
+    return "servlet";
   }
 
   @Override

diff --git a/...o/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3InstrumentationModule.java b/...o/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3InstrumentationModule.java
@@ -11,6 +11,7 @@
 import com.google.auto.service.AutoService;
 import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
+import io.opentelemetry.javaagent.extension.instrumentation.internal.ExperimentalInstrumentationModule;
 import io.opentelemetry.javaagent.instrumentation.servlet.common.async.AsyncContextInstrumentation;
 import io.opentelemetry.javaagent.instrumentation.servlet.common.async.AsyncContextStartInstrumentation;
 import io.opentelemetry.javaagent.instrumentation.servlet.common.async.AsyncStartInstrumentation;
@@ -21,7 +22,8 @@
 import net.bytebuddy.matcher.ElementMatcher;
 
 @AutoService(InstrumentationModule.class)
-public class Servlet3InstrumentationModule extends InstrumentationModule {
+public class Servlet3InstrumentationModule extends InstrumentationModule
+    implements ExperimentalInstrumentationModule {
   private static final String BASE_PACKAGE = "javax.servlet";
 
   public Servlet3InstrumentationModule() {
@@ -34,9 +36,9 @@ public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
   }
 
   @Override
-  public boolean isIndyModule() {
-    // GrailsTest fails
-    return false;
+  public String getModuleGroup() {
+    // depends on servlet instrumentation
+    return "servlet";
   }
 
   @Override

diff --git a/...spring/security/config/v6_0/servlet/SpringSecurityConfigServletInstrumentationModule.java b/...spring/security/config/v6_0/servlet/SpringSecurityConfigServletInstrumentationModule.java
@@ -12,13 +12,15 @@
 import io.opentelemetry.javaagent.bootstrap.internal.AgentCommonConfig;
 import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
+import io.opentelemetry.javaagent.extension.instrumentation.internal.ExperimentalInstrumentationModule;
 import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties;
 import java.util.List;
 import net.bytebuddy.matcher.ElementMatcher;
 
 /** Instrumentation module for servlet-based applications that use spring-security-config. */
 @AutoService(InstrumentationModule.class)
-public class SpringSecurityConfigServletInstrumentationModule extends InstrumentationModule {
+public class SpringSecurityConfigServletInstrumentationModule extends InstrumentationModule
+    implements ExperimentalInstrumentationModule {
   public SpringSecurityConfigServletInstrumentationModule() {
     super("spring-security-config-servlet", "spring-security-config-servlet-6.0");
   }
@@ -47,6 +49,12 @@ public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
         "org.springframework.security.authentication.ObservationAuthenticationManager");
   }
 
+  @Override
+  public String getModuleGroup() {
+    // depends on servlet instrumentation
+    return "servlet";
+  }
+
   @Override
   public List<TypeInstrumentation> typeInstrumentations() {
     return singletonList(new HttpSecurityInstrumentation());

diff --git a/...pentelemetry/javaagent/instrumentation/springweb/v3_1/SpringWebInstrumentationModule.java b/...pentelemetry/javaagent/instrumentation/springweb/v3_1/SpringWebInstrumentationModule.java
@@ -13,11 +13,13 @@
 import io.opentelemetry.javaagent.extension.instrumentation.HelperResourceBuilder;
 import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
+import io.opentelemetry.javaagent.extension.instrumentation.internal.ExperimentalInstrumentationModule;
 import java.util.List;
 import net.bytebuddy.matcher.ElementMatcher;
 
 @AutoService(InstrumentationModule.class)
-public class SpringWebInstrumentationModule extends InstrumentationModule {
+public class SpringWebInstrumentationModule extends InstrumentationModule
+    implements ExperimentalInstrumentationModule {
   public SpringWebInstrumentationModule() {
     super("spring-web", "spring-web-3.1");
   }
@@ -32,12 +34,23 @@ public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
 
   @Override
   public void registerHelperResources(HelperResourceBuilder helperResourceBuilder) {
-    // make the filter class file loadable by ClassPathResource - in some cases (e.g. spring-guice,
-    // see https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/7428) Spring
-    // might want to read the class file metadata; this line will make the filter class file visible
-    // to the bean class loader
-    helperResourceBuilder.register(
-        "org/springframework/web/servlet/v3_1/OpenTelemetryHandlerMappingFilter.class");
+    if (!isIndyModule()) {
+      // make the filter class file loadable by ClassPathResource - in some cases (e.g.
+      // spring-guice,
+      // see https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/7428)
+      // Spring
+      // might want to read the class file metadata; this line will make the filter class file
+      // visible
+      // to the bean class loader
+      helperResourceBuilder.register(
+          "org/springframework/web/servlet/v3_1/OpenTelemetryHandlerMappingFilter.class");
+    }
+  }
+
+  @Override
+  public String getModuleGroup() {
+    // depends on OpenTelemetryHandlerMappingFilter
+    return "servlet";
   }
 
   @Override

diff --git a/...emetry/javaagent/instrumentation/springweb/v3_1/WebApplicationContextInstrumentation.java b/...emetry/javaagent/instrumentation/springweb/v3_1/WebApplicationContextInstrumentation.java
@@ -61,6 +61,9 @@ public static class FilterInjectingAdvice {
     public static void onEnter(@Advice.Argument(0) ConfigurableListableBeanFactory beanFactory) {
       if (beanFactory instanceof BeanDefinitionRegistry
           && !beanFactory.containsBean("otelAutoDispatcherFilter")) {
+
+        // Explicitly loading classes allows to catch any class-loading issue or deal with cases
+        // where the class is not visible.
         try {
           // Firstly check whether DispatcherServlet is present. We need to load an instrumented
           // class from spring-webmvc to trigger injection that makes

diff --git a/...entelemetry/javaagent/instrumentation/spring/web/v6_0/SpringWebInstrumentationModule.java b/...entelemetry/javaagent/instrumentation/spring/web/v6_0/SpringWebInstrumentationModule.java
@@ -12,11 +12,13 @@
 import io.opentelemetry.javaagent.extension.instrumentation.HelperResourceBuilder;
 import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
+import io.opentelemetry.javaagent.extension.instrumentation.internal.ExperimentalInstrumentationModule;
 import java.util.List;
 import net.bytebuddy.matcher.ElementMatcher;
 
 @AutoService(InstrumentationModule.class)
-public class SpringWebInstrumentationModule extends InstrumentationModule {
+public class SpringWebInstrumentationModule extends InstrumentationModule
+    implements ExperimentalInstrumentationModule {
 
   public SpringWebInstrumentationModule() {
     super("spring-web", "spring-web-6.0");
@@ -30,12 +32,23 @@ public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
 
   @Override
   public void registerHelperResources(HelperResourceBuilder helperResourceBuilder) {
-    // make the filter class file loadable by ClassPathResource - in some cases (e.g. spring-guice,
-    // see https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/7428) Spring
-    // might want to read the class file metadata; this line will make the filter class file visible
-    // to the bean class loader
-    helperResourceBuilder.register(
-        "org/springframework/web/servlet/v6_0/OpenTelemetryHandlerMappingFilter.class");
+    if (!isIndyModule()) {
+      // make the filter class file loadable by ClassPathResource - in some cases (e.g.
+      // spring-guice,
+      // see https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/7428)
+      // Spring
+      // might want to read the class file metadata; this line will make the filter class file
+      // visible
+      // to the bean class loader
+      helperResourceBuilder.register(
+          "org/springframework/web/servlet/v6_0/OpenTelemetryHandlerMappingFilter.class");
+    }
+  }
+
+  @Override
+  public String getModuleGroup() {
+    // depends on OpenTelemetryHandlerMappingFilter
+    return "servlet";
   }
 
   @Override

diff --git a/...metry/javaagent/instrumentation/spring/web/v6_0/WebApplicationContextInstrumentation.java b/...metry/javaagent/instrumentation/spring/web/v6_0/WebApplicationContextInstrumentation.java
@@ -61,6 +61,8 @@ public static class FilterInjectingAdvice {
     public static void onEnter(@Advice.Argument(0) ConfigurableListableBeanFactory beanFactory) {
       if (beanFactory instanceof BeanDefinitionRegistry
           && !beanFactory.containsBean("otelAutoDispatcherFilter")) {
+        // Explicitly loading classes allows to catch any class-loading issue or deal with cases
+        // where the class is not visible.
         try {
           // Firstly check whether DispatcherServlet is present. We need to load an instrumented
           // class from spring-webmvc to trigger injection that makes