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.

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

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

Merged
merged 47 commits into from
Nov 30, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
47 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
33c2778
add todo to remove reflection and public field
SylvainJuge Nov 13, 2024
397213f
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
SylvainJuge Nov 14, 2024
4163047
revert previous impl
SylvainJuge Nov 14, 2024
f330abb
another attempt with helper class
SylvainJuge Nov 14, 2024
abc58f8
code reformat
SylvainJuge Nov 14, 2024
6d9198b
further simplify caller code
SylvainJuge Nov 14, 2024
5a6683a
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
SylvainJuge Nov 14, 2024
bceeb00
fix pebkc
SylvainJuge Nov 14, 2024
8708bf7
do not rely on public delegate field
SylvainJuge Nov 14, 2024
59552ab
pebkc printf debug
SylvainJuge Nov 14, 2024
56a52b1
fix test on proxy class
SylvainJuge Nov 14, 2024
c468d47
hide IndyProxy interface from reflection
SylvainJuge Nov 19, 2024
20a95bc
indy proxy method impl should be synthetic
SylvainJuge Nov 19, 2024
56f1aa9
update proxy factory test
SylvainJuge Nov 19, 2024
c13b72e
add a bit of javadoc
SylvainJuge Nov 19, 2024
63a501e
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
SylvainJuge Nov 19, 2024
1806c82
test IndyProxyHelper
SylvainJuge Nov 19, 2024
44977f2
spotless
SylvainJuge Nov 20, 2024
37e1823
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
SylvainJuge Nov 20, 2024
99cc98a
add test for reflection method filtering
SylvainJuge Nov 20, 2024
a43b667
also test interface is hidden from reflection
SylvainJuge Nov 20, 2024
1427f9f
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
SylvainJuge Nov 25, 2024
a5806c6
keep registerHelperResources for inlined
SylvainJuge Nov 27, 2024
bf1beca
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
SylvainJuge Nov 27, 2024
9def1a6
IndyProxy > InstrumentationProxy
SylvainJuge Nov 27, 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
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
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
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
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
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) {
SylvainJuge 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
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
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
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
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) {
SylvainJuge marked this conversation as resolved.
Show resolved Hide resolved
// 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
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
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) {
SylvainJuge marked this conversation as resolved.
Show resolved Hide resolved
// inline advice: no proxy class is used
filter = (OpenTelemetryHandlerMappingFilter) bean;
} else {
SylvainJuge marked this conversation as resolved.
Show resolved Hide resolved
// 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
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
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