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

Instrumenting additional Akka server methods #6243

Closed
wants to merge 6 commits into from

Conversation

zackman0010
Copy link
Contributor

Previous PR #6109. Opened a new PR to use a different branch in my fork so that my fork can have a clean main.

Add support for auto-instrumentation of Akka's IncomingConnection.handleWith and HttpExt.bindAndHandle methods. Both methods utilize a custom BidiGraphStage to wrap a request in instrumentation. A PinnedDispatcher is utilized to make sure the graph stage runs on the same thread for both request and response handling, preventing scope leakage.

Fixes #6081 and #5137

Co-authored-by: Jeremy Ingle [email protected]

zackman0010 and others added 6 commits May 27, 2022 23:06
Add support for auto-instrumentation of Akka's IncomingConnection.handleWith and HttpExt.bindAndHandle methods. Both methods utilize a custom BidiGraphStage to wrap a request in instrumentation. A PinnedDispatcher is utilized to make sure the graph stage runs on the same thread for both request and response handling, preventing scope leakage.

Co-authored-by: Jeremy Ingle <[email protected]>
* Added Akka HTTP Unmarshaller instrumentation to fix Context loss with 10.2

* Fix tests, all Akka tests are now successful

Co-authored-by: Zachary Sistrunk <[email protected]>
We found out that the Unmarshal context loss is actually caused by a larger context loss issue in akka-streams. We'll aim to fix this via a separate future pull request.
@zackman0010 zackman0010 requested a review from a team June 29, 2022 16:58
@linux-foundation-easycla
Copy link

CLA Not Signed

@zackman0010 zackman0010 marked this pull request as draft June 29, 2022 16:58
@mateuszrzeszutek mateuszrzeszutek linked an issue Jun 30, 2022 that may be closed by this pull request
@zackman0010
Copy link
Contributor Author

zackman0010 commented Jul 11, 2022

Just confirming that the CLA is still in progress internally. I confirmed with my manager that it's still in the pipeline, it's just taking a while. It's the first time we've contributed to open source, so there's no existing protocol for this kind of request to go through.

@trask
Copy link
Member

trask commented Oct 23, 2022

hi @zackman0010! any objection to closing this (just trying to clean up the backlog a little), and you can re-open when you are able to sign the CLA? I think if I close it you won't be able to re-open it, but if you close it, I think you will be able to re-open yourself. And of course you can always ping on the closed PR and one of us will re-open it if that fails.

also, I'm curious if DCO would be easier for you/your company compared to SLA, check out recent discussion going on about CLA vs DCO in the OpenTelemetry community repo: open-telemetry/community#1252

@zackman0010
Copy link
Contributor Author

zackman0010 commented Oct 23, 2022

@trask Sure, we can do that. I'm sorry again that we haven't had any progress for so long. Our manager has been caught up in another project for several months, and without someone actively working on getting the CLA signed the whole process is stuck. It's a shame because we were very excited to contribute.

I'll bring it back up again this week and see if we can get it picked back up, but if that doesn't work I'll go ahead and close the MR.

As for CLA vs DCO, I don't know enough about the legal side of things to comment, but I imagine that it would run into the same issues here. After reading the thread, though, my personal opinion as a developer is that I like the suggested hybrid approach. I don't have a problem with signing the CLA, and I'd rather not risk forgetting to sign off on a commit and having to rebase the branch to fix it.

@zackman0010
Copy link
Contributor Author

As discussed, closing this PR until our CLA is taken care of.

laurit added a commit that referenced this pull request 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Akka HTTP connection source Akka HTTP bindFlow leads to non working http tracing
3 participants