Check jax-rs AsyncResponse for span before starting new one #1403
Conversation
This will prevent the request from being broken due to replacing the unfinished span.
| @@ -44,7 +45,7 @@ | |||
| public Map<? extends ElementMatcher<? super MethodDescription>, String> transformers() { | |||
| final Map<ElementMatcher<? super MethodDescription>, String> transformers = new HashMap<>(); | |||
| transformers.put( | |||
| named("execute").and(takesArgument(0, Runnable.class)), | |||
| named("execute").and(takesArgument(0, Runnable.class)).and(takesArguments(1)), | |||
There was a problem hiding this comment.
This is not immediately related, though I discovered this problem in the same investigation and it is tangentially related...
When an executor has multiple execute methods, one calling the other, previously they were both instrumented and we would see Failed to set continuation because another continuation is already set log messages. This is because the first call would create a continuation, set it on the task, then call the next execute method, which would do the same, but it would close the second one and keep the first. These log messages were a red herring for my investigation as it didn't actually impact the application reporting.
...ns-2/src/main/java/datadog/trace/instrumentation/jaxrs2/JaxRsAnnotationsInstrumentation.java
Outdated
Show resolved
Hide resolved
| @Advice.This final Object target, | ||
| @Advice.Origin final Method method, | ||
| @Advice.AllArguments final Object[] args, | ||
| @Advice.Local("asyncResponse") AsyncResponse asyncResponse) { | ||
| ContextStore<AsyncResponse, AgentSpan> contextStore = null; | ||
| for (final Object arg : args) { | ||
| if (arg instanceof AsyncResponse) { | ||
| asyncResponse = (AsyncResponse) arg; | ||
| contextStore = InstrumentationContext.get(AsyncResponse.class, AgentSpan.class); | ||
| if (contextStore.get(asyncResponse) != null) { | ||
| /** | ||
| * We are probably in a recursive call and don't want to start a new span because it | ||
| * would replace the existing span in the asyncResponse and cause it to never finish. We | ||
| * could work around this by using a list instead, but we likely don't want the extra | ||
| * span anyway. | ||
| */ | ||
| return null; | ||
| } | ||
| break; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
And this doesn't need to be done for datadog.trace.instrumentation.jaxrs1.JaxRsAnnotationsInstrumentation?
There was a problem hiding this comment.
jaxrs1 doesn't support async. that is the main distinction between the two.
This will prevent the request from being broken due to replacing the unfinished span.