Skip to content
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

Support for Akka HTTP connection source #6081

Closed
zackman0010 opened this issue May 23, 2022 · 5 comments · Fixed by #8174
Closed

Support for Akka HTTP connection source #6081

zackman0010 opened this issue May 23, 2022 · 5 comments · Fixed by #8174
Labels
enhancement New feature or request

Comments

@zackman0010
Copy link
Contributor

Currently, the Akka HTTP instrumentation only supports using .bindAndHandleSync and .bindAndHandleAsync methods (.bind and .bindAsync in Akka HTTP 10.2's new ServerBuilder methods). I'd like to add support for the connection source method (Http().bind in 10.0 and 10.1, builder.connectionSource() in 10.2).

The connection source method works by providing the user with a Source[Http.IncomingConnection]. We can instrument this class's .handleWith function to wrap the handler in instrumentation.

The instrumentation required to wrap .handleWith can also be used to complete #5137, as they require wrapping a handler of type Flow.

We're currently internally testing an implementation of this instrumentation, but decided to create this issue first for feedback as recommended in the contributing guidelines.

@zackman0010 zackman0010 added the enhancement New feature or request label May 23, 2022
@laurit
Copy link
Contributor

laurit commented May 24, 2022

@zackman0010 feel free to submit a PR

@zackman0010
Copy link
Contributor Author

We're still testing our implantation internally, but we'll be submitting a PR as soon as we're done!

We're actually running into a strange issue right now. We're calling instrumenter().start(..) and context.setCurrent() on each request, saving both the Context and Scope values to be used after the response returns. When we get the response, we make sure to call instrumenter().end(..) with the same Context provided by the call to start and calling scope.close() on the Scope provided by the setCurrent call.
Everything appears to be working correctly. We can look in Grafana and see the parent span generated by our instrumentation and all the children spans generated by the application code underneath our HTTP server. However, later requests are reusing the same trace IDs. When we investigated, it appears to happen because currentContext() is still returning the Context object from the previous request.

Do you have any suggestions on why this might be happening? We can go ahead and push our PR if you'd need to see code.

@trask
Copy link
Member

trask commented May 25, 2022

hi @zackman0010, it sounds most likely that you are leaking a Scope, meaning you are setting the context as current (which binds it to thread local) and then not closing the associated scope in the same thread.

it's best to always call makeCurrent in a try-with-resources block, or if you are inside of bytebuddy advice it's a little tricker, but make sure you are starting the scope in OnMethodEnter, passing the Scope object to OnMethodExit and closing it there

@zackman0010
Copy link
Contributor Author

That sounds very plausible, I know Akka doesn't play well with ThreadLocals. The comment on the existing Akka instrumentation about there being no clean way to handle closing scopes is making more sense now as well.

We'll keep looking into this, see if we can find a way around it.

@zackman0010
Copy link
Contributor Author

We successfully found a workaround to fix the scope leakage. Akka has the concept of a PinnedDispatcher, which is used to enforce a "one thread per actor" rule. Internally, it assigns each actor with a thread pool of size 1, which causes that actor to exclusively use that one thread as well as to have exclusive use of that thread.

We were able to use instrumentation to inject a new PinnedDispatcher into the configuration, then have have the wrapper we made utilize our dispatcher instead of Akka's default dispatcher. This fixed the Scope leakage issue that we were previously seeing.

We created PR #6109 for both this issue and the issue described in #5137, as both could be fixed with the same dispatcher/wrapper combination.

laurit added a commit that referenced this issue Apr 5, 2023
Resolves
#8143
Resolves
#6081
Resolves
#5137
Using the same approach as in
#6243
and as used by DataDog. Unlike in #6243 this pr does not attempt to
prevent leaking scopes into actors but rather instruments the actor to
reset context to get rid of the leaked scopes (DataDog does the same).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment