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

Instrument akka-http bindAndHandle #8174

Merged
merged 2 commits into from
Apr 5, 2023

Conversation

laurit
Copy link
Contributor

@laurit laurit commented Mar 30, 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).

@laurit laurit requested a review from a team as a code owner March 30, 2023 14:11
import scala.concurrent.Await

// FIXME: This doesn't work because we don't support bindAndHandle.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@laurit laurit merged commit d87f40c into open-telemetry:main Apr 5, 2023
@laurit laurit deleted the akka-http-bind-and-handle branch April 5, 2023 14:11
@zackman0010
Copy link
Contributor

zackman0010 commented Apr 5, 2023

@laurit It's good to see that someone else is picking this up, since our legal team never would respond to our emails to sign our CLA.
Disregard the struck text, I looked back at our old merge and saw we did the same thing.. That's what happens when you come back to a project after almost a year and try to talk like you know what you're talking about 😅. Although is there any particular reason you code to accept the scope leakage instead of using a PinnedDispatcher like our original MR did? And if we later get the CLA signed, would you have any objections to us reintroducing the PinnedDispatcher to get rid of the scope leakage?
However, I do have a potential concern. The way you've maintained the scopes is through a queue, which assumes the requests and responses will arrive in the same order. But unfortunately Akka doesn't work that way, the responses may leave in a different order than the requests arrived. If you make request A followed by request B but B finishes first, then B arriving back in the AkkaFlowWrapper would pull A's scope off the queue instead. This is the reason our solution ended up using the PinnedDispatcher to prevent scope leakage.

For what it's worth, we're attempting to restart the process with our legal team again so that we can help contribute. Our company is making a big push to move off AppDynamics and other paid services by replacing them with open source alternatives such as OpenTelemetry, so we'd love to be able to contribute fixes.

@laurit
Copy link
Contributor Author

laurit commented Apr 6, 2023

@zackman0010 For me it is too hard to understand the ramifications of using PinnedDispatcher instead of what is used by default. This is why I chose to let the scope leak and deal with that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants