Handle connection failures for Jersey and Resteasy #479
Conversation
tylerbenson
force-pushed
the
tyler/broken-jax-rs-client
branch
from
44039f3
to
ec8b8dc
Compare
tylerbenson
force-pushed
the
tyler/broken-jax-rs-client
branch
4 times, most recently
from
de5c70c
to
b34faac
Compare
| Project instr_project = project | ||
| subprojects { subProj -> | ||
| if (subProj.getParent() == instr_project) { | ||
| if (!skip.contains(name)) { |
There was a problem hiding this comment.
Could you please add a comment about that this is achieving?
From what I see currently is that you script connection-error-handling subproject there - but it contains code so it is unclear how this actually gets included into the agent.
There was a problem hiding this comment.
Looks like this change is there to prevent the connection-error-handling project itself from being added, but its two children, jersey and resteasy, will still be added. Some kind of comment or formalization of this "only add the leaf projects" would be nice.
Another problem is this change will cause the scala-testing and akka-testing projects to be added to the agent's jar.
There was a problem hiding this comment.
I can add a comment.
@realark I moved the source for scala and akka testing projects into the test folder, so those projects don't have any production source or dependencies.
| public final class JerseyClientConnectionErrorInstrumentation extends Instrumenter.Default { | ||
|
|
||
| public JerseyClientConnectionErrorInstrumentation() { | ||
| super("jax-rs", "jaxrs", "jax-rs-client"); |
There was a problem hiding this comment.
You may want to consider adding a way to disable this part of the instrumentation without disable all of jax-rs
There was a problem hiding this comment.
I considered that, but I think it's not a good idea to have the client instrumentation enabled without this, which is why I didn't add it.
| } | ||
|
|
||
| public static class WrappedFuture<T> implements Future<T> { | ||
| public static final String SPAN_PROPERTY_NAME = "datadog.trace.jaxrs.span"; // Copied elsewhere |
There was a problem hiding this comment.
It looks like you could add compile (or compileOnly) dependency on :dd-java-agent:instrumentation:jax-rs-client into this project and avoid this copy-paste.
| Project instr_project = project | ||
| subprojects { subProj -> | ||
| if (subProj.getParent() == instr_project) { | ||
| if (!skip.contains(name)) { |
There was a problem hiding this comment.
Looks like this change is there to prevent the connection-error-handling project itself from being added, but its two children, jersey and resteasy, will still be added. Some kind of comment or formalization of this "only add the leaf projects" would be nice.
Another problem is this change will cause the scala-testing and akka-testing projects to be added to the agent's jar.
| } | ||
|
|
||
| public static class WrappedFuture<T> implements Future<T> { | ||
| public static final String SPAN_PROPERTY_NAME = "datadog.trace.jaxrs.span"; // Copied elsewhere |
| public static void handleError( | ||
| @Advice.FieldValue("configuration") final ClientConfiguration context, | ||
| @Advice.Return(readOnly = false) Future future) { | ||
| future = new WrappedFuture(future, context); |
There was a problem hiding this comment.
(Same CallDepthThreadLocalMap question)
| public static void handleError( | ||
| @Advice.FieldValue("requestContext") final ClientRequest context, | ||
| @Advice.Return(readOnly = false) Future future) { | ||
| future = new WrappedFuture(future, context); |
There was a problem hiding this comment.
Do we need to guard against nested calls with CallDepthThreadLocalMap?
CXF and other client frameworks will still lose the span and miss the error.
tylerbenson
force-pushed
the
tyler/broken-jax-rs-client
branch
from
b34faac
to
6a03eb5
Compare
tylerbenson
force-pushed
the
tyler/broken-jax-rs-client
branch
from
6a03eb5
to
de21c7b
Compare
tylerbenson
dismissed
mar-kolya’s stale review
Please review again. Everything has been addressed.
|
Shoot. Forgot to merge this before the release. |
CXF and other client frameworks will still lose the span and miss the error.
(See #437)