Akka Actor and Akka HTTP bindAndHandle instrumentation #2050
Conversation
bantonsson
force-pushed
the
ban/akka-http-bah
branch
3 times, most recently
from
89649cd
to
7eac56a
Compare
bantonsson
changed the title
| @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) | ||
| public static void exit(@Advice.Enter TraceScope scope) { | ||
| // Clean up any leaking scopes from akka-streams/akka-http et.c. | ||
| TraceScope activeScope = activeScope(); | ||
| while (activeScope != null && activeScope != scope) { | ||
| activeScope.close(); | ||
| activeScope = activeScope(); | ||
| } | ||
| while (activeScope == scope) { | ||
| scope.close(); | ||
| activeScope = activeScope(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This seems rather scary and will likely result in unexpected bugs elsewhere. Do you know why this is necessary?
There was a problem hiding this comment.
There are two things here. One is the intentional leak in the akka-http server instrumentation that you commented on, and the second is just plain defensive programming. There should never ever be anything left on the scope stack after the actor has processed a message.
|
|
||
| public static class ConstructAdvice { | ||
| @Advice.OnMethodExit(suppress = Throwable.class) | ||
| public static void afterInit(@Advice.This Envelope zis) { |
| @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) | ||
| public static void exit(@Advice.Enter final AgentScope scope) { | ||
| // Clean up any leaking scopes from akka-streams/akka-http et.c. | ||
| TraceScope activeScope = activeScope(); | ||
| while (activeScope != null && activeScope != scope) { | ||
| activeScope.close(); | ||
| activeScope = activeScope(); | ||
| } | ||
| while (activeScope == scope) { | ||
| scope.close(); | ||
| activeScope = activeScope(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Same concern applies here.
...10.0/src/main/java/datadog/trace/instrumentation/akkahttp/AkkaHttpServerInstrumentation.java
Outdated
Show resolved
Hide resolved
...10.0/src/main/java/datadog/trace/instrumentation/akkahttp/AkkaHttpServerInstrumentation.java
Outdated
Show resolved
Hide resolved
...10.0/src/main/java/datadog/trace/instrumentation/akkahttp/AkkaHttpServerInstrumentation.java
Show resolved
Hide resolved
| // Since we haven't instrumented the akka stream state machine, we can't rely | ||
| // on spans and scopes being propagated during the push and pull of the | ||
| // element. Instead we let the scope leak intentionally here and clean it | ||
| // up when the user response comes back, or in the actor message processing | ||
| // instrumentation that drives this state machine. |
There was a problem hiding this comment.
This is interesting. I guess this is the reason you need that logic to clear the scope stack. I don't think I understand though why the scope can't be closed after the push.
There was a problem hiding this comment.
So akka streams are run by a state machine, and the only thing you know after the push is that this specific element has been put in the slot representing the requestOutlet. Nothing more has happened, and no request handling code has run yet. If we close the scope at that point, there will be nothing in the scope. Since the whole stream machinery has been fused we can be certain that this element will be picked up by the same thread that is running this onPush method, and then the request handling code will be run. If the request handling code completes synchronously, then the handler for the responseInlet will close the scope directly in that onPush method. Otherwise the scope will be closed by the actor messaging instrumentation cleaning the scope stack.
I'll add a more detailed comment about how things work.
I'll write a
...10.0/src/main/java/datadog/trace/instrumentation/akkahttp/AkkaHttpServerInstrumentation.java
Outdated
Show resolved
Hide resolved
| } | ||
| // If there is no scope created from the envelope, we create our own noopSpan to make sure | ||
| // that we can close all scopes up until this position after exit. | ||
| AgentSpan span = new AgentTracer.NoopAgentSpan(); |
There was a problem hiding this comment.
AgentTracer.noopSpan() would be better here
There was a problem hiding this comment.
The reason that I construct a new span instance is that I want to minimize the chance that the cleaning code messes up the scope stack reference count for any scope that is active when we come in to this code, and using the AgentTracer.noopSpan() could easily do that since I can't distinguish between this span/scope and any that was active when calling this method.
...10.0/src/main/java/datadog/trace/instrumentation/akkahttp/AkkaHttpServerInstrumentation.java
Show resolved
Hide resolved
bantonsson
force-pushed
the
ban/akka-http-bah
branch
from
610cf62
to
f18e638
Compare
|
Thanks for the additional comments/docs. |
A number of changes to support
bindAndHandleinakka-http:akka-actorinstrumentation to not leak context between actors, and attach context to the messages instead of the mailboxes.flowinbindAndHandleforakka-httpto have one single place that covers all serverbind*variants.akka-httpto not rely on a thread local when checking for nesting to avoid repeated spans in preparation for client flow support.