Define helper classes in loadClass #5528
Conversation
|
|
||
| // Now attempt to load our injected instrumentation class from the same class loader as | ||
| // DispatcherServlet | ||
| Class<?> clazz = |
There was a problem hiding this comment.
We have a test with wildfly where loading our injected class from getBeanClassLoader() fails. Previously it worked because it hit findLoadedClass during class loading.
If we do it this way we can probably put OpenTelemetryHandlerMappingFilter back into our own package. We needed to put in spring package because we set up the bean definition in the wrong way. We set both name and class that are actually kept in the same field in spring bean definition so it always tried to use loadClass which previously worked only if the class was in spring package.
...ing-java9/src/main/java/io/opentelemetry/javaagent/tooling/ExposeAgentBootstrapListener.java
Show resolved
Hide resolved
| if (resolution.isResolved()) { | ||
| return new ClasspathType(resolution.resolve()); | ||
| if (!helperClassPredicate.isHelperClass(className)) { | ||
| Resolution resolution = classpathPool.describe(className); |
There was a problem hiding this comment.
Looking up an injected helper records it in AgentCachingPoolStrategy as not found. If that helper is defined before it gets evicted there will be a stack trace about resource not found.
There was a problem hiding this comment.
this is helpful explanation, can you turn it into code comment? (especially since breaking it won't fail tests, only log annoying debug messages)
laurit
changed the title
...ntelemetry/javaagent/instrumentation/internal/classloader/BootDelegationInstrumentation.java
Outdated
Show resolved
Hide resolved
| Class<?> helperClass = InjectedClassHelper.loadHelperClass(classLoader, name); | ||
| if (helperClass != null) { | ||
| return helperClass; | ||
| } |
javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/InjectedClassHelper.java
Outdated
Show resolved
Hide resolved
...ing-java9/src/main/java/io/opentelemetry/javaagent/tooling/ExposeAgentBootstrapListener.java
Outdated
Show resolved
Hide resolved
...ing-java9/src/main/java/io/opentelemetry/javaagent/tooling/ExposeAgentBootstrapListener.java
Show resolved
Hide resolved
| .setSuperClassName(TestHelperClasses.HelperSuperClass.name) | ||
| .setSuperClassName(TestAbstractSuperClass.name) |
There was a problem hiding this comment.
I didn't follow the reason for these changes
There was a problem hiding this comment.
This was needed because of the change in HelperReferenceWrapper that added !helperClassPredicate.isHelperClass(className) check. Now classes that are in io.opentelemetry.instrumentation are matched by that check which made a couple of tests fail. I believe the intent of the test was to test injected helper extending a class from application. To ensure that the class that the test extends is treated as an application class and not as a helper I had to copy it into a different package. I also tried moving TestHelperClasses but that made a bunch of other tests fail.
| if (resolution.isResolved()) { | ||
| return new ClasspathType(resolution.resolve()); | ||
| if (!helperClassPredicate.isHelperClass(className)) { | ||
| Resolution resolution = classpathPool.describe(className); |
There was a problem hiding this comment.
this is helpful explanation, can you turn it into code comment? (especially since breaking it won't fail tests, only log annoying debug messages)
|
🥳 |
* Define helper classes in loadClass similarly to regular classes * fix test * spotless * address review comments
Resolves #5499
Resolves #5493
Resolves #5466
Instead of injecting helper classes during transformation instrument
loadClassmethods of class loaders to load them. This should avoid defining helper class triggering loading additional classes that won't get transformed.