Resteasy support#234
Conversation
* Adds scope manager for JAX-RS when running outside of servlet * Separates JAX-RS tests for different frameworks due to clashing implementations
|
Unclear why CI didn't run again; maybe it was getting tired of running and failing. The last issue was Java 7 support in the tests, which I just gave up on. |
|
Thanks for your contribution. It's not really clear from reading your PR... was there something wrong with the existing JAX-RS instrumentation that it wasn't working for V2 or resteasy, or were you mainly adding a test suite? |
|
@tylerbenson Without the new filter that I added, we weren't getting a parent scope at all when using Netty. |
avram
force-pushed
the
features/resteasy
branch
from
1b8497f to
f946a25
Compare
|
I've re-run the tests and they now pass without issue. |
|
Ah, I see... my instrumentation was focused on naming. It assumes you are using servlets which generates it's own trace/span. Later, when we add instrumentation for Netty, it should work correctly. How does this PR improve things? I don't see additional instrumentation. Does |
|
In my usage and in the test included here, Explicit registration: |
| import javax.ws.rs.ext.Provider; | ||
|
|
||
| @Provider | ||
| public final class OpenTracingFilter implements ContainerRequestFilter, ContainerResponseFilter { |
There was a problem hiding this comment.
Can you provide a class comment here describing the purpose and (since it's not explicit) how it gets picked up?
| include ':dd-java-agent:benchmark-integration:jetty-perftest' | ||
|
|
||
| // Resteasy integration test requires Java 8+ | ||
| include ':dd-java-agent:instrumentation:jax-rs:resteasy' |
There was a problem hiding this comment.
Instead of excluding the whole project, exclude just the test by doing something like this: https://github.com/DataDog/dd-trace-java/blob/master/dd-trace-ot/dd-trace-ot.gradle#L21-L30
|
|
||
| dependencies { | ||
| compileOnly group: 'javax.ws.rs', name: 'jsr311-api', version: '1.1.1' | ||
| compileOnly group: 'javax.ws.rs', name: 'javax.ws.rs-api', version: '2.0.1' |
There was a problem hiding this comment.
I'd rather not have both on the classpath to avoid confusion. Can this line be removed?
| compile project(':dd-trace-ot') | ||
| compile project(':dd-java-agent:agent-tooling') | ||
|
|
||
| testCompile project(':dd-java-agent:testing') |
There was a problem hiding this comment.
Can you leave JaxRsInstrumentationTest.groovy in this project and only move the jersey specific portion?
| final Scope scope = tracer.scopeManager().active(); | ||
| if (scope == null) { | ||
| final Span span = tracer.buildSpan("request-parent").start(); | ||
| tracer.scopeManager().activate(span, true); |
There was a problem hiding this comment.
What is the reason for activating the span in the scope manager (on the thread)? Since netty is async, this seems like a dangerous proposition. Perhaps this would be a good opportunity to explore the new API for ScopeMangers that I'm proposing: #233.
There was a problem hiding this comment.
I'll look into it. I haven't really looked into what it means to "activate" a span, so perhaps this is a bit misguided.
Since the complexity could also be driven by multithreaded access, I can also extend my Netty test coverage to try to stress the tracing more; plenty of naïve patterns will look workable in simple tests like we have here.
I can also put the PR on hold until we've had a chance to try the scope-annotating filter in our larger deployments (~1.5M RPM); the existing JAX-RS support should work for us in the meantime if I just add the filter to our internal codebase.
There was a problem hiding this comment.
That's fine. Just ping me when you're ready to revisit. I'll leave the PR open for now. Thanks for the contribution.
3 participants
While the Resteasy integration probably works under Java 7, I've excluded it from test runs because the tests fail.