-
Notifications
You must be signed in to change notification settings - Fork 277
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
JAX-RS ContainerRequestContextFilter instrumentation #1134
Conversation
...rc/main/java/datadog/trace/instrumentation/jaxrs2/ContainerRequestFilterInstrumentation.java
Outdated
Show resolved
Hide resolved
10d125d
to
7306219
Compare
import javax.ws.rs.core.UriInfo; | ||
import net.bytebuddy.asm.Advice; | ||
|
||
/** Jersey specific filter 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.
Could you comment on why implementation specific behavior was needed?
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.
Similar reason to why connection-error-handling-jersey
and connection-error-handling-resteasy
exist: there's no standard JAX-RS way to get the matched resource from the context.
- Jersey has its
ExtendedUriInfo
class that implementsResourceInfo
- Resteasy has
PostMatchContainerRequestContext
that givesResourceMethodInvoker
- Apache CXF doesn't have anything 👎
I'll add some more to the javadoc
.and(named("abortWith")) | ||
.and(takesArguments(1)) | ||
.and(takesArgument(0, named("javax.ws.rs.core.Response"))), | ||
getClass().getName() + "$ContainerRequestContextAdvice"); |
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.
Clever.
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.
Optional comments... shame we had to break down into implementation specific integrations.
final AgentSpan parent, | ||
final Class target, | ||
final Method method, | ||
final String resourceName) { |
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 method seems to only be called by the overloaded version. Consider combining.
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.
You're right. Originally, onAbort
was doing something slightly different which is why I had to split out onControllerStart
originally. I'll refactor it back.
@@ -0,0 +1,25 @@ | |||
muzzle { | |||
// Cant assert fails because muzzle assumes all instrumentations will fail | |||
// Instrumentations in jax-rs-annotations-2 will pass |
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.
You may be able to work around this by changing the compile
dependency to compileOnly
.
This pull request adds instrumentation to apply resource names from JAX-RS annotations when
abort
is called from aContainerRequestContextFilter
. There are several issues that complicate this instrumentation:@PreMatching
)abort
method is located on theContainerRequestContext
and not the filterTo solve the above, there is: