diff --git a/instrumentation/internal/internal-reflection/javaagent-integration-tests/src/main/java/instrumentation/TestHelperClass.java b/instrumentation/internal/internal-reflection/javaagent-integration-tests/src/main/java/instrumentation/TestHelperClass.java new file mode 100644 index 000000000000..fa310587f74f --- /dev/null +++ b/instrumentation/internal/internal-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/instrumentation/internal/internal-reflection/javaagent-integration-tests/src/main/java/instrumentation/TestInstrumentationModule.java b/instrumentation/internal/internal-reflection/javaagent-integration-tests/src/main/java/instrumentation/TestInstrumentationModule.java index d9e406633db8..67a74f26e7ad 100644 --- a/instrumentation/internal/internal-reflection/javaagent-integration-tests/src/main/java/instrumentation/TestInstrumentationModule.java +++ b/instrumentation/internal/internal-reflection/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 typeInstrumentations() { return singletonList(new TestTypeInstrumentation()); } + + @Override + public List 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/instrumentation/internal/internal-reflection/javaagent-integration-tests/src/test/java/ReflectionTest.java b/instrumentation/internal/internal-reflection/javaagent-integration-tests/src/test/java/ReflectionTest.java index 5d142fbd4d0c..7293136a21bb 100644 --- a/instrumentation/internal/internal-reflection/javaagent-integration-tests/src/test/java/ReflectionTest.java +++ b/instrumentation/internal/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/instrumentation/internal/internal-reflection/javaagent-integration-tests/src/test/java/TestClass.java b/instrumentation/internal/internal-reflection/javaagent-integration-tests/src/test/java/TestClass.java index 20d1fef8e12e..96f304b6b3a2 100644 --- a/instrumentation/internal/internal-reflection/javaagent-integration-tests/src/test/java/TestClass.java +++ b/instrumentation/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/instrumentation/internal/internal-reflection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ReflectionHelper.java b/instrumentation/internal/internal-reflection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ReflectionHelper.java index 241c731f1469..e38e67de2a71 100644 --- a/instrumentation/internal/internal-reflection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ReflectionHelper.java +++ b/instrumentation/internal/internal-reflection/javaagent/src/main/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 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> 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/instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-jersey-2.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jaxrs/v2_0/JerseyInstrumentationModule.java b/instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-jersey-2.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jaxrs/v2_0/JerseyInstrumentationModule.java index 1e2a1e296bc0..8ab480006922 100644 --- a/instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-jersey-2.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jaxrs/v2_0/JerseyInstrumentationModule.java +++ b/instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-jersey-2.0/javaagent/src/main/java/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 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/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3InstrumentationModule.java b/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3InstrumentationModule.java index 46cc5596a9a8..f7a45a270f8b 100644 --- a/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3InstrumentationModule.java +++ b/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/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 classLoaderMatcher() { } @Override - public boolean isIndyModule() { - // GrailsTest fails - return false; + public String getModuleGroup() { + // depends on servlet instrumentation + return "servlet"; } @Override diff --git a/instrumentation/spring/spring-security-config-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/security/config/v6_0/servlet/SpringSecurityConfigServletInstrumentationModule.java b/instrumentation/spring/spring-security-config-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/security/config/v6_0/servlet/SpringSecurityConfigServletInstrumentationModule.java index 926bd6d94103..85c3e0bc00d0 100644 --- a/instrumentation/spring/spring-security-config-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/security/config/v6_0/servlet/SpringSecurityConfigServletInstrumentationModule.java +++ b/instrumentation/spring/spring-security-config-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/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 classLoaderMatcher() { "org.springframework.security.authentication.ObservationAuthenticationManager"); } + @Override + public String getModuleGroup() { + // depends on servlet instrumentation + return "servlet"; + } + @Override public List typeInstrumentations() { return singletonList(new HttpSecurityInstrumentation()); diff --git a/instrumentation/spring/spring-web/spring-web-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springweb/v3_1/SpringWebInstrumentationModule.java b/instrumentation/spring/spring-web/spring-web-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springweb/v3_1/SpringWebInstrumentationModule.java index 53e67dcd6e34..f257f527177a 100644 --- a/instrumentation/spring/spring-web/spring-web-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springweb/v3_1/SpringWebInstrumentationModule.java +++ b/instrumentation/spring/spring-web/spring-web-3.1/javaagent/src/main/java/io/opentelemetry/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 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/instrumentation/spring/spring-web/spring-web-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springweb/v3_1/WebApplicationContextInstrumentation.java b/instrumentation/spring/spring-web/spring-web-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springweb/v3_1/WebApplicationContextInstrumentation.java index fb9cbd7c3bbe..fa15d4c912a9 100644 --- a/instrumentation/spring/spring-web/spring-web-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springweb/v3_1/WebApplicationContextInstrumentation.java +++ b/instrumentation/spring/spring-web/spring-web-3.1/javaagent/src/main/java/io/opentelemetry/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/instrumentation/spring/spring-web/spring-web-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/web/v6_0/SpringWebInstrumentationModule.java b/instrumentation/spring/spring-web/spring-web-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/web/v6_0/SpringWebInstrumentationModule.java index f246e678c3b3..6a66e70cd866 100644 --- a/instrumentation/spring/spring-web/spring-web-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/web/v6_0/SpringWebInstrumentationModule.java +++ b/instrumentation/spring/spring-web/spring-web-6.0/javaagent/src/main/java/io/opentelemetry/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 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/instrumentation/spring/spring-web/spring-web-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/web/v6_0/WebApplicationContextInstrumentation.java b/instrumentation/spring/spring-web/spring-web-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/web/v6_0/WebApplicationContextInstrumentation.java index 3ecad4e9d748..69fbc36e1e70 100644 --- a/instrumentation/spring/spring-web/spring-web-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/web/v6_0/WebApplicationContextInstrumentation.java +++ b/instrumentation/spring/spring-web/spring-web-6.0/javaagent/src/main/java/io/opentelemetry/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 diff --git a/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webmvc/v3_1/DispatcherServletInstrumentation.java b/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webmvc/v3_1/DispatcherServletInstrumentation.java index 0ecea21b5e21..2b35744d63ed 100644 --- a/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webmvc/v3_1/DispatcherServletInstrumentation.java +++ b/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webmvc/v3_1/DispatcherServletInstrumentation.java @@ -15,6 +15,7 @@ import io.opentelemetry.context.Context; import io.opentelemetry.context.Scope; +import io.opentelemetry.javaagent.bootstrap.InstrumentationProxyHelper; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; import java.util.List; @@ -61,13 +62,14 @@ public static class HandlerMappingAdvice { public static void afterRefresh( @Advice.Argument(0) ApplicationContext springCtx, @Advice.FieldValue("handlerMappings") List handlerMappings) { - if (springCtx.containsBean("otelAutoDispatcherFilter")) { - OpenTelemetryHandlerMappingFilter filter = - (OpenTelemetryHandlerMappingFilter) springCtx.getBean("otelAutoDispatcherFilter"); - if (handlerMappings != null && filter != null) { - filter.setHandlerMappings(handlerMappings); - } + + if (handlerMappings == null || !springCtx.containsBean("otelAutoDispatcherFilter")) { + return; } + Object bean = springCtx.getBean("otelAutoDispatcherFilter"); + OpenTelemetryHandlerMappingFilter filter = + InstrumentationProxyHelper.unwrapIfNeeded(bean, OpenTelemetryHandlerMappingFilter.class); + filter.setHandlerMappings(handlerMappings); } } diff --git a/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webmvc/v3_1/SpringWebMvcInstrumentationModule.java b/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webmvc/v3_1/SpringWebMvcInstrumentationModule.java index 4fcececf341c..0a11d2e8d109 100644 --- a/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webmvc/v3_1/SpringWebMvcInstrumentationModule.java +++ b/instrumentation/spring/spring-webmvc/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webmvc/v3_1/SpringWebMvcInstrumentationModule.java @@ -11,11 +11,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.List; import net.bytebuddy.matcher.ElementMatcher; @AutoService(InstrumentationModule.class) -public class SpringWebMvcInstrumentationModule extends InstrumentationModule { +public class SpringWebMvcInstrumentationModule extends InstrumentationModule + implements ExperimentalInstrumentationModule { public SpringWebMvcInstrumentationModule() { super("spring-webmvc", "spring-webmvc-3.1"); @@ -28,13 +32,21 @@ public ElementMatcher.Junction classLoaderMatcher() { @Override public boolean isHelperClass(String className) { + // filter on prefix due to inner classes return className.startsWith( "org.springframework.web.servlet.v3_1.OpenTelemetryHandlerMappingFilter"); } @Override - public boolean isIndyModule() { - return false; + public void injectClasses(ClassInjector injector) { + injector + .proxyBuilder("org.springframework.web.servlet.v3_1.OpenTelemetryHandlerMappingFilter") + .inject(InjectionMode.CLASS_AND_RESOURCE); + } + + @Override + public String getModuleGroup() { + return "servlet"; } @Override diff --git a/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webmvc/v6_0/DispatcherServletInstrumentation.java b/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webmvc/v6_0/DispatcherServletInstrumentation.java index 60d663663bea..826a1a174803 100644 --- a/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webmvc/v6_0/DispatcherServletInstrumentation.java +++ b/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webmvc/v6_0/DispatcherServletInstrumentation.java @@ -15,6 +15,7 @@ import io.opentelemetry.context.Context; import io.opentelemetry.context.Scope; +import io.opentelemetry.javaagent.bootstrap.InstrumentationProxyHelper; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; import java.util.List; @@ -61,13 +62,15 @@ public static class HandlerMappingAdvice { public static void afterRefresh( @Advice.Argument(0) ApplicationContext springCtx, @Advice.FieldValue("handlerMappings") List handlerMappings) { - if (springCtx.containsBean("otelAutoDispatcherFilter")) { - OpenTelemetryHandlerMappingFilter filter = - (OpenTelemetryHandlerMappingFilter) springCtx.getBean("otelAutoDispatcherFilter"); - if (handlerMappings != null) { - filter.setHandlerMappings(handlerMappings); - } + + if (handlerMappings == null || !springCtx.containsBean("otelAutoDispatcherFilter")) { + return; } + + Object bean = springCtx.getBean("otelAutoDispatcherFilter"); + OpenTelemetryHandlerMappingFilter filter = + InstrumentationProxyHelper.unwrapIfNeeded(bean, OpenTelemetryHandlerMappingFilter.class); + filter.setHandlerMappings(handlerMappings); } } diff --git a/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webmvc/v6_0/SpringWebMvcInstrumentationModule.java b/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webmvc/v6_0/SpringWebMvcInstrumentationModule.java index a3d7cef051c4..d48a2d697f35 100644 --- a/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webmvc/v6_0/SpringWebMvcInstrumentationModule.java +++ b/instrumentation/spring/spring-webmvc/spring-webmvc-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/webmvc/v6_0/SpringWebMvcInstrumentationModule.java @@ -11,11 +11,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.List; import net.bytebuddy.matcher.ElementMatcher; @AutoService(InstrumentationModule.class) -public class SpringWebMvcInstrumentationModule extends InstrumentationModule { +public class SpringWebMvcInstrumentationModule extends InstrumentationModule + implements ExperimentalInstrumentationModule { public SpringWebMvcInstrumentationModule() { super("spring-webmvc", "spring-webmvc-6.0"); @@ -28,13 +32,21 @@ public ElementMatcher.Junction classLoaderMatcher() { @Override public boolean isHelperClass(String className) { + // filter on prefix due to inner classes return className.startsWith( "org.springframework.web.servlet.v6_0.OpenTelemetryHandlerMappingFilter"); } @Override - public boolean isIndyModule() { - return false; + public void injectClasses(ClassInjector injector) { + injector + .proxyBuilder("org.springframework.web.servlet.v6_0.OpenTelemetryHandlerMappingFilter") + .inject(InjectionMode.CLASS_AND_RESOURCE); + } + + @Override + public String getModuleGroup() { + return "servlet"; } @Override diff --git a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/InstrumentationProxy.java b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/InstrumentationProxy.java new file mode 100644 index 000000000000..b92f0994dd09 --- /dev/null +++ b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/InstrumentationProxy.java @@ -0,0 +1,20 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.bootstrap; + +/** Interface added to indy proxies to allow unwrapping the proxy object */ +public interface InstrumentationProxy { + + /** + * Unwraps the proxy delegate instance + * + * @return wrapped object instance + */ + // Method name does not fit common naming practices on purpose + // Also, when modifying this make sure to also update string references. + @SuppressWarnings("all") + Object __getIndyProxyDelegate(); +} diff --git a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/InstrumentationProxyHelper.java b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/InstrumentationProxyHelper.java new file mode 100644 index 000000000000..2a369ab9a224 --- /dev/null +++ b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/InstrumentationProxyHelper.java @@ -0,0 +1,36 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.bootstrap; + +public class InstrumentationProxyHelper { + + private InstrumentationProxyHelper() {} + + /** + * Unwraps and casts an indy proxy, or just casts if it's not an indy proxy. + * + * @param type of object to return + * @param o object to unwrap + * @param type expected object type + * @return unwrapped proxy instance or the original object (if not a proxy) cast to the expected + * type + * @throws IllegalArgumentException if the provided object the proxied object can't be cast to the + * expected type + */ + public static T unwrapIfNeeded(Object o, Class type) { + if (o instanceof InstrumentationProxy) { + Object delegate = ((InstrumentationProxy) o).__getIndyProxyDelegate(); + if (type.isAssignableFrom(delegate.getClass())) { + return type.cast(delegate); + } + } + if (type.isAssignableFrom(o.getClass())) { + return type.cast(o); + } + + throw new IllegalArgumentException("unexpected object type"); + } +} diff --git a/javaagent-bootstrap/src/test/java/io/opentelemetry/javaagent/bootstrap/InstrumentationProxyHelperTest.java b/javaagent-bootstrap/src/test/java/io/opentelemetry/javaagent/bootstrap/InstrumentationProxyHelperTest.java new file mode 100644 index 000000000000..996e414ea857 --- /dev/null +++ b/javaagent-bootstrap/src/test/java/io/opentelemetry/javaagent/bootstrap/InstrumentationProxyHelperTest.java @@ -0,0 +1,40 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.bootstrap; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import org.junit.jupiter.api.Test; + +class InstrumentationProxyHelperTest { + + @Test + void wrongType() { + assertThrows( + IllegalArgumentException.class, + () -> InstrumentationProxyHelper.unwrapIfNeeded("", Integer.class)); + assertThrows( + IllegalArgumentException.class, + () -> InstrumentationProxyHelper.unwrapIfNeeded(proxy(""), Integer.class)); + } + + @Test + void unwrap() { + + // no wrapping + Number number = InstrumentationProxyHelper.unwrapIfNeeded(42, Number.class); + assertThat(number).isEqualTo(42); + + // unwrap needed + String string = InstrumentationProxyHelper.unwrapIfNeeded(proxy("hello"), String.class); + assertThat(string).isEqualTo("hello"); + } + + private static InstrumentationProxy proxy(Object delegate) { + return () -> delegate; + } +} diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyProxyFactory.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyProxyFactory.java index 0a1b774ba60f..9cca710ccda8 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyProxyFactory.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyProxyFactory.java @@ -5,12 +5,17 @@ package io.opentelemetry.javaagent.tooling.instrumentation.indy; +import io.opentelemetry.javaagent.bootstrap.InstrumentationProxy; import java.lang.reflect.Method; import java.lang.reflect.Modifier; +import java.util.ArrayList; import java.util.List; import net.bytebuddy.ByteBuddy; import net.bytebuddy.description.method.MethodDescription; import net.bytebuddy.description.method.ParameterDescription; +import net.bytebuddy.description.modifier.SyntheticState; +import net.bytebuddy.description.modifier.Visibility; +import net.bytebuddy.description.type.TypeDefinition; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.dynamic.DynamicType; import net.bytebuddy.dynamic.scaffold.subclass.ConstructorStrategy; @@ -68,6 +73,9 @@ List getBootstrapArgsForMethod( private static final String DELEGATE_FIELD_NAME = "delegate"; + // Matches the single method of IndyProxy interface + static final String PROXY_DELEGATE_NAME = "__getIndyProxyDelegate"; + private final MethodDescription.InDefinedShape indyBootstrapMethod; private final BootstrapArgsProvider bootstrapArgsProvider; @@ -87,10 +95,12 @@ public IndyProxyFactory(Method bootstrapMethod, BootstrapArgsProvider bootstrapA public DynamicType.Unloaded generateProxy( TypeDescription classToProxy, String proxyClassName) { TypeDescription.Generic superClass = classToProxy.getSuperClass(); + List interfaces = new ArrayList<>(classToProxy.getInterfaces()); + interfaces.add(TypeDescription.ForLoadedType.of(InstrumentationProxy.class)); DynamicType.Builder builder = new ByteBuddy() .subclass(superClass, ConstructorStrategy.Default.NO_CONSTRUCTORS) - .implement(classToProxy.getInterfaces()) + .implement(interfaces) .name(proxyClassName) .annotateType(classToProxy.getDeclaredAnnotations()) .defineField(DELEGATE_FIELD_NAME, Object.class, Modifier.PRIVATE | Modifier.FINAL); @@ -108,6 +118,14 @@ public DynamicType.Unloaded generateProxy( } } } + + // Implement IndyProxy class and return the delegate field + builder = + builder + .defineMethod( + PROXY_DELEGATE_NAME, Object.class, Visibility.PUBLIC, SyntheticState.SYNTHETIC) + .intercept(FieldAccessor.ofField(DELEGATE_FIELD_NAME)); + return builder.make(); } diff --git a/javaagent-tooling/src/test/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyProxyFactoryTest.java b/javaagent-tooling/src/test/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyProxyFactoryTest.java index c5d1c287013b..9bd9ecb78917 100644 --- a/javaagent-tooling/src/test/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyProxyFactoryTest.java +++ b/javaagent-tooling/src/test/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyProxyFactoryTest.java @@ -7,6 +7,7 @@ import static org.assertj.core.api.Assertions.assertThat; +import io.opentelemetry.javaagent.bootstrap.InstrumentationProxy; import io.opentelemetry.javaagent.tooling.instrumentation.indy.dummies.DummyAnnotation; import java.lang.invoke.CallSite; import java.lang.invoke.ConstantCallSite; @@ -309,22 +310,55 @@ private static void privateStaticMethod() {} } @Test - void verifyNonPublicMembersIgnored() throws Exception { + void verifyNonPublicMembersIgnored() { Class proxy = generateProxy(IgnoreNonPublicMethods.class); assertThat(proxy.getConstructors()).hasSize(1); assertThat(proxy.getDeclaredMethods()) - .hasSize(2) + .hasSize(3) .anySatisfy(method -> assertThat(method.getName()).isEqualTo("publicMethod")) - .anySatisfy(method -> assertThat(method.getName()).isEqualTo("publicStaticMethod")); + .anySatisfy(method -> assertThat(method.getName()).isEqualTo("publicStaticMethod")) + // IndyProxy implementation visible but later hidden by reflection instrumentation + .anySatisfy( + method -> { + assertThat(method.getName()).isEqualTo(IndyProxyFactory.PROXY_DELEGATE_NAME); + assertThat(method.isSynthetic()).isTrue(); + }); + } + + @Test + void verifyProxyClass() throws Exception { + Class proxyType = generateProxy(ProxyUnwrapTest.class); + assertThat(proxyType) + .isNotInstanceOf(InstrumentationProxy.class) + .isNotInstanceOf(ProxyUnwrapTest.class); + + assertThat(InstrumentationProxy.class.isAssignableFrom(proxyType)) + .describedAs("proxy class can be cast to IndyProxy") + .isTrue(); + + Object proxyInstance = proxyType.getConstructor().newInstance(); + Object proxyDelegate = ((InstrumentationProxy) proxyInstance).__getIndyProxyDelegate(); + assertThat(proxyDelegate).isInstanceOf(ProxyUnwrapTest.class); + } + + @SuppressWarnings("all") + public static class ProxyUnwrapTest { + public ProxyUnwrapTest() {} + + public void sampleMethod() {} } private static Class generateProxy(Class clazz) { DynamicType.Unloaded unloaded = proxyFactory.generateProxy( TypeDescription.ForLoadedType.of(clazz), clazz.getName() + "Proxy"); - // Uncomment the following line to view the generated bytecode if needed - // unloaded.saveIn(new File("generated_proxies")); + // Uncomment the following block to view the generated bytecode if needed + // try { + // unloaded.saveIn(new File("build/generated_proxies")); + // } catch (IOException e) { + // throw new RuntimeException(e); + // } return unloaded.load(clazz.getClassLoader()).getLoaded(); } }