-
Notifications
You must be signed in to change notification settings - Fork 845
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
Move helper class to spring package so that loadClass can find it #3718
Move helper class to spring package so that loadClass can find it #3718
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would love to have a test for this somehow
@trask tests added |
❤️ the tests look great, I'll review in the next couple days |
Class<?> clazz = | ||
beanFactory | ||
.getBeanClassLoader() | ||
.loadClass("org.springframework.web.servlet.OpenTelemetryHandlerMappingFilter"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OpenTelemetryHandlerMappingFilter
is defined in the webmvc instrumentation, right? This makes spring-web and spring-webmvc instrumentations depend on each other; one can't function without the other.
I think it'd be more preferable to have one InstrumentationModule
delivering a single, coherent piece of functionality (webmvc instrumentation) even if we have to depend on two libs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that doesn't really work when 2 modules are in different class loaders. Muzzle will fail on spring-web because instrumentation uses classes from spring-webmvc hence they can't be in the same InstrumentationModule
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that's true -- the instrumentation won't be applied if the classloader doesn't contain all references.
🤯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have this problem for all instrumentations which use currently apply a muzzle extraDependency
? is this change needed for this PR, or can we split out to discuss separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we assume that all spring jars are in the same class loader then this would not be needed. Nowadays this probably isn't super important, but years ago when spring was pushing their dm server and all spring jars had osgi manifests then when instrumenting spring it was best to make sure that the code you inject with instrumentation doesn't reference anything from bundles that instrumented bundle doesn't depend on. If we just insert reference to OpenTelemetryHandlerMappingFilter
into something like org.springframework.web.context.support.AbstractRefreshableWebApplicationContext
we have created a dependency from spring-web
to spring-webmvc
which doesn't exist in original code (dependency is the other way around) which would make application fail if it for some reason used spring-web
but not spring-webmvc
.
extraDependency
is usually completely fine, as long as instrumented library has a non optional dependency to something then we can freely use it in instrumentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extraDependency
is usually completely fine, as long as instrumented library has a non optional dependency to something then we can freely use it in instrumentation.
oh yes this makes perfect sense thx 👍👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just adding comment in case I come back here later 😄: we can freely use the extraDependency
in the instrumentation, but we cannot freely instrument the extraDependency
<!-- | ||
Spring boot automatically registers a servlet filter when spring bean implements | ||
javax.servlet.Filter but vanilla spring does not so we need to add a placeholder | ||
for our servlet filter here. | ||
--> | ||
<filter> | ||
<filter-name>otelAutoDispatcherFilter</filter-name> | ||
<filter-class>org.springframework.web.filter.DelegatingFilterProxy</filter-class> | ||
</filter> | ||
|
||
<filter> | ||
<filter-name>testFilter</filter-name> | ||
<filter-class>org.springframework.web.filter.DelegatingFilterProxy</filter-class> | ||
</filter> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not clear to me why the double registration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because there are 2 filters otelAutoDispatcherFilter
is used by the filter added by instrumentation, testFilter
is used by test. Basically what the integration does it attempts to figure out what would handle the request before the request processing reaches spring so to test it we need something that also runs before spring request processing to handle it so that it never reaches spring. I guess this could be useful when there is a spring security filter that returns not authorized, without this integration there wouldn't be a nice path for such requests.
With spring boot otelAutoDispatcherFilter
would be added automatically but not with plain spring so this instrumentation is kind of useless when not using spring boot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx, I finally got it 😅
import org.jboss.arquillian.container.test.api.Deployment | ||
import org.jboss.arquillian.container.test.api.RunAsClient | ||
import org.jboss.arquillian.spock.ArquillianSputnik | ||
import org.jboss.arquillian.test.api.ArquillianResource | ||
import org.jboss.shrinkwrap.api.Archive | ||
import org.jboss.shrinkwrap.api.ShrinkWrap | ||
import org.jboss.shrinkwrap.api.spec.EnterpriseArchive | ||
import org.jboss.shrinkwrap.api.spec.WebArchive |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is amazing 👍
Resolves #3306