Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
c73f5c6
make servlet3 indy compatible
SylvainJuge Oct 10, 2024
59f857e
spring web mvc
SylvainJuge Oct 10, 2024
7a0993d
wip but still broken
SylvainJuge Oct 14, 2024
e6e4f7a
fix clasloading of injected class
SylvainJuge Oct 15, 2024
37970e8
avoid stack overflow with proxy
SylvainJuge Oct 15, 2024
cdd978c
spotless
SylvainJuge Oct 15, 2024
30f5356
spotless again
SylvainJuge Oct 15, 2024
4e6ce38
remove debugging exceptions
SylvainJuge Oct 15, 2024
dba9e7b
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
SylvainJuge Oct 15, 2024
dbbbdea
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
SylvainJuge Oct 25, 2024
b043e83
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
SylvainJuge Oct 28, 2024
99d8bff
add comment to clarify public field access
SylvainJuge Oct 28, 2024
0c91cbc
turns out proxy delegate access is not needed
SylvainJuge Oct 28, 2024
5b3b408
fix spotless
SylvainJuge Oct 28, 2024
d2714ad
public delegate needed with indy
SylvainJuge Oct 28, 2024
978ce81
add a few comments to elaborate on servlet module
SylvainJuge Oct 29, 2024
8740eff
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
SylvainJuge Nov 4, 2024
43ddd93
post review comment
SylvainJuge Nov 6, 2024
da8d9bc
remove indy changes for grails
SylvainJuge Nov 6, 2024
affe804
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
SylvainJuge Nov 6, 2024
54a9f00
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
SylvainJuge Nov 12, 2024
a7f57cf
better proxy unwrap
SylvainJuge Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion ...n/java/io/opentelemetry/javaagent/instrumentation/grails/GrailsInstrumentationModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,21 @@
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;

@AutoService(InstrumentationModule.class)
public class GrailsInstrumentationModule extends InstrumentationModule {
public class GrailsInstrumentationModule extends InstrumentationModule
implements ExperimentalInstrumentationModule {
public GrailsInstrumentationModule() {
super("grails", "grails-3.0");
}

@Override
public String getModuleGroup() {
return "servlet";
SylvainJuge marked this conversation as resolved.
Show resolved Hide resolved
}

@Override
public List<TypeInstrumentation> typeInstrumentations() {
return asList(
Expand Down
12 changes: 6 additions & 6 deletions ...va/io/opentelemetry/javaagent/instrumentation/jaxrs/v2_0/JerseyInstrumentationModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Expand All @@ -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
Expand Down
10 changes: 6 additions & 4 deletions ...o/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3InstrumentationModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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() {
Expand All @@ -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
Expand Down
10 changes: 9 additions & 1 deletion ...spring/security/config/v6_0/servlet/SpringSecurityConfigServletInstrumentationModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Expand Down Expand Up @@ -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());
Expand Down
15 changes: 6 additions & 9 deletions ...pentelemetry/javaagent/instrumentation/springweb/v3_1/SpringWebInstrumentationModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,15 @@
import static net.bytebuddy.matcher.ElementMatchers.not;

import com.google.auto.service.AutoService;
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");
}
Expand All @@ -31,13 +32,9 @@ public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
}

@Override
public void registerHelperResources(HelperResourceBuilder helperResourceBuilder) {
laurit marked this conversation as resolved.
Show resolved Hide resolved
// 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");
public String getModuleGroup() {
// depends on OpenTelemetryHandlerMappingFilter
return "servlet";
}

@Override
Expand Down
3 changes: 3 additions & 0 deletions ...emetry/javaagent/instrumentation/springweb/v3_1/WebApplicationContextInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
15 changes: 6 additions & 9 deletions ...entelemetry/javaagent/instrumentation/spring/web/v6_0/SpringWebInstrumentationModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,15 @@
import static java.util.Collections.singletonList;

import com.google.auto.service.AutoService;
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");
Expand All @@ -29,13 +30,9 @@ 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");
public String getModuleGroup() {
// depends on OpenTelemetryHandlerMappingFilter
return "servlet";
}

@Override
Expand Down
2 changes: 2 additions & 0 deletions ...metry/javaagent/instrumentation/spring/web/v6_0/WebApplicationContextInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
23 changes: 18 additions & 5 deletions ...emetry/javaagent/instrumentation/spring/webmvc/v3_1/DispatcherServletInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import io.opentelemetry.context.Scope;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
import java.lang.reflect.Field;
import java.util.List;
import net.bytebuddy.asm.Advice;
import net.bytebuddy.description.type.TypeDescription;
Expand Down Expand Up @@ -61,13 +62,25 @@ public static class HandlerMappingAdvice {
public static void afterRefresh(
@Advice.Argument(0) ApplicationContext springCtx,
@Advice.FieldValue("handlerMappings") List<HandlerMapping> 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;
if (bean instanceof OpenTelemetryHandlerMappingFilter) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[for reviewer] this is an edge-case where the inlined advice will be able to directly call the OpenTelemetryHandlerMappingFilter class directly and with indy we get a proxy instance that needs to be unwrapped (thus the delegate fields needs to be made public for simplicity).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to add a TODO here to drop this branch when we remove support for inlined advice then maybe?

// inline advice: no proxy class is used
filter = (OpenTelemetryHandlerMappingFilter) bean;
} else {
// non-inlined advice: proxy class is used
try {
Field delegate = bean.getClass().getField("delegate");
filter = (OpenTelemetryHandlerMappingFilter) delegate.get(bean);
} catch (NoSuchFieldException | IllegalAccessException e) {
throw new IllegalStateException(e);
}
}
filter.setHandlerMappings(handlerMappings);
}
}

Expand Down
18 changes: 15 additions & 3 deletions ...metry/javaagent/instrumentation/spring/webmvc/v3_1/SpringWebMvcInstrumentationModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -28,13 +32,21 @@ public ElementMatcher.Junction<ClassLoader> 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
Expand Down
24 changes: 19 additions & 5 deletions ...emetry/javaagent/instrumentation/spring/webmvc/v6_0/DispatcherServletInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import io.opentelemetry.context.Scope;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
import java.lang.reflect.Field;
import java.util.List;
import net.bytebuddy.asm.Advice;
import net.bytebuddy.description.type.TypeDescription;
Expand Down Expand Up @@ -61,13 +62,26 @@ public static class HandlerMappingAdvice {
public static void afterRefresh(
@Advice.Argument(0) ApplicationContext springCtx,
@Advice.FieldValue("handlerMappings") List<HandlerMapping> 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;
if (bean instanceof OpenTelemetryHandlerMappingFilter) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[for reviewer] this is an edge-case where the inlined advice will be able to directly call the OpenTelemetryHandlerMappingFilter class directly and with indy we get a proxy instance that needs to be unwrapped (thus the delegate fields needs to be made public for simplicity).

// inline advice: no proxy class is used
filter = (OpenTelemetryHandlerMappingFilter) bean;
} else {
Copy link
Contributor

@laurit laurit Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imo instead of doing the reflection here there should be an API to do this. Something that would unwrap the object when it is proxy and return the input when it is not. So you could write OpenTelemetryHandlerMappingFilter filter = = (OpenTelemetryHandlerMappingFilter) unwrap(springCtx.getBean("otelAutoDispatcherFilter"));
I guess using the reflection as currently is fine for start. Instead of making the delegate field public I'd probably try to make the proxy instance implement an interface that allow unwrapping. If needed that interface could be hidden from reflection

opentelemetry-java-instrumentation/instrumentation/internal/internal-reflection/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/reflection/ReflectionHelper.java

Line 59 in 647fc45

public static Class<?>[] filterInterfaces(Class<?>[] interfaces, Class<?> containingClass) {
Idk how hard all of this would be to implement.

// non-inlined advice: proxy class is used
try {
Field delegate = bean.getClass().getField("delegate");
filter = (OpenTelemetryHandlerMappingFilter) delegate.get(bean);
} catch (NoSuchFieldException | IllegalAccessException e) {
throw new IllegalStateException(e);
}
}
filter.setHandlerMappings(handlerMappings);
}
}

Expand Down
18 changes: 15 additions & 3 deletions ...metry/javaagent/instrumentation/spring/webmvc/v6_0/SpringWebMvcInstrumentationModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -28,13 +32,21 @@ public ElementMatcher.Junction<ClassLoader> 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
Expand Down
3 changes: 2 additions & 1 deletion ...c/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyProxyFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,8 @@ public DynamicType.Unloaded<?> generateProxy(
.implement(classToProxy.getInterfaces())
.name(proxyClassName)
.annotateType(classToProxy.getDeclaredAnnotations())
.defineField(DELEGATE_FIELD_NAME, Object.class, Modifier.PRIVATE | Modifier.FINAL);
// field must be public to enable resolving the proxy target using introspection
.defineField(DELEGATE_FIELD_NAME, Object.class, Modifier.PUBLIC | Modifier.FINAL);

for (MethodDescription.InDefinedShape method : classToProxy.getDeclaredMethods()) {
if (method.isPublic()) {
Expand Down
Loading