Library instrumentation for Java Servlet 3.0 Filters. #15188
Conversation
Includes handling for attaching all the async stuff, but can't support everything the java agent does. I could use help figuring out the right way to shadow all the necessary dependencies. I'm a bit rusty with our shadow usage. Depending on how aggressive we want to be, the dependency could be reversed so that `:servlet-3.0:javaagent` depends on this new module, but that's more aggressive. Perhaps shadow could prune unused classes (with `minimize()`) instead. Once we get the modules figured out, I can work on adding some unit tests. If we like this approach, it could probably be copied and modified pretty easily to support servlet 5, but I'm not familiar with that instrumentation.
You shouldn't shadow anything. If you need to share code you can extract common code between the javaagent and library instrumentation into separate module or make javaagent module depend on the library module if that make sense. In your place I would focus on building a functioning instrumentation along with tests and explore options for sharing code later. It is quite possible that the agent code does things that don't make sense for the library instrumentation which could limit the amount of code that can be shared between them. |
| @@ -0,0 +1,43 @@ | |||
| package io.opentelemetry.javaagent.instrumentation.servlet.v3_0; | |||
|
|
|||
| import static io.opentelemetry.javaagent.instrumentation.servlet.v3_0.Servlet3Singletons.helper; | |||
There was a problem hiding this comment.
@laurit I'm not sure how to handle the dependency on classes like io.opentelemetry.javaagent.bootstrap.http.HttpServerResponseMutator from Servlet3Accessor referenced by Servlet3Singletons that seem to expect to be on the bootstrap classpath and are part of the javaagent-extension-api.
More and more it seems like there will be relatively little code that can be shared between the javaagent and library instrumentation.
There was a problem hiding this comment.
That one shouldn't be too hard to handle. You can easily remove that interface from Servlet3Accessor and have the usage in advice create a HttpServerResponseMutator that calls the appendHeader method in Servlet3Accessor.
There was a problem hiding this comment.
Ok, I ended up just copying over all the classes that were referenced. Let me know what/if you want me to prune anything significant.
Copied over from :servlet3.0:javaagent. Had to disable/remove some test cases/scenarios that didn't make sense or just weren't working. Still not using proper gradle project structure though.
| @Override | ||
| public void run() { | ||
| try { | ||
| try (Scope scope = context.makeCurrent()) { |
There was a problem hiding this comment.
It seems like an oversight that this isn't already here.
|
|
||
| public static final VirtualField<Filter, MappingResolver.Factory> | ||
| FILTER_MAPPING_RESOLVER_FACTORY = | ||
| VirtualField.find(Filter.class, MappingResolver.Factory.class); |
There was a problem hiding this comment.
This change isn't necessary, but it seems odd that these are duplicated. I combined them, but if it's intentional I can revert this change.
| Throwable throwable = null; | ||
| Servlet3RequestAdviceScope adviceScope = | ||
| new Servlet3RequestAdviceScope( | ||
| CallDepth.forClass(OpenTelemetryServletFilter.class), httpRequest, httpResponse, this); |
There was a problem hiding this comment.
I tried to keep the instrumentation as similar as possible to the javaagent instrumentation.
tylerbenson
force-pushed
the
tyler/servlet-filter-inst-lib
branch
from
4efcd9e to
2e0d691
Compare
|
|
||
| dependencies { | ||
| library("javax.servlet:javax.servlet-api:3.0.1") | ||
| library("io.opentelemetry.semconv:opentelemetry-semconv-incubating") |
There was a problem hiding this comment.
Seeing some latestDepTest failures on this. How should I be declaring this dependency properly?
There was a problem hiding this comment.
You should use compileOnly for servlet-api and implementation for opentelemetry-semconv-incubating (actually you shouldn't add that dependency at all, but more on that later). library dependencies have their version bumped to latest available version when running latest dep tests, you don't want that for either of these dependencies.
In library instrumentations we don't depend on incubating semconv but copy the keys we need from there. For example
There was a problem hiding this comment.
Thanks for the help. Build passes now!
Incubating keys are internalized to avoid a dependency on a moving target.
|
🔧 The result from spotlessApply was committed to the PR branch. |
tylerbenson
force-pushed
the
tyler/servlet-filter-inst-lib
branch
from
c822c44 to
6ce59fa
Compare
3 participants
Includes handling for attaching all the async stuff, but can't support everything the java agent does.
I could use help figuring out the right way to shadow all the necessary dependencies. I'm a bit rusty with our shadow usage. Depending on how aggressive we want to be, the dependency could be reversed so that
:servlet-3.0:javaagentdepends on this new module, but that's more aggressive. Perhaps shadow could prune unused classes (withminimize()) instead.Once we get the modules figured out, I can work on adding some unit tests.
If we like this approach, it could probably be copied and modified pretty easily to support servlet 5, but I'm not familiar with that instrumentation.