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

ZIO 2.0 instrumentation #7980

Merged
merged 8 commits into from
Apr 6, 2023
Merged

Conversation

dmytr
Copy link
Contributor

@dmytr dmytr commented Mar 5, 2023

Summary

ZIO is a popular Scala library for asynchronous and concurrent programming. Its runtime uses green threads (fibers) which breaks instrumentation because context is stored in ThreadLocal variable. This PR takes care of synchronizing context between fibers and JVM threads on which they are executed.

Notes about implementation

FiberContext is the main piece which handles context synchronization. Implementation was tested on a non-trivial distributed application.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 5, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: dmytr / name: Dmytro Iaroslavskyi (dc24d4f)

@github-actions github-actions bot requested a review from theletterf March 5, 2023 20:10
@dmytr dmytr changed the title ZIO 2.0 instrumentation [WIP] ZIO 2.0 instrumentation Mar 5, 2023
@dmytr dmytr changed the title [WIP] ZIO 2.0 instrumentation ZIO 2.0 instrumentation Mar 7, 2023
@dmytr dmytr marked this pull request as ready for review March 7, 2023 22:00
@dmytr dmytr requested a review from a team as a code owner March 7, 2023 22:00
@dmytr
Copy link
Contributor Author

dmytr commented Mar 24, 2023

Hi @laurit, what do you think about the following change to this PR: dmytr#2?

It seems to be more correct if we store thread's active context when fiber is resumed, and restore it when fiber is suspended. Instead of just restoring the root context.

Tests pass, and I've tested this with my test harness - everything still seems to work.

@laurit
Copy link
Contributor

laurit commented Apr 3, 2023

Hi @laurit, what do you think about the following change to this PR: dmytr#2?

It seems to be more correct if we store thread's active context when fiber is resumed, and restore it when fiber is suspended. Instead of just restoring the root context.

@dmytr I think it would work. But as the thread executing the fibers probably should not have a context outside of executing the fiber, and when it has, it is probably a context that was inadvertently leaked there, I'd keep the current approach just because it is a bit simpler. Unless you are suspecting that the thread could have a meaningful context outside of executing fibers that must be preserved I wouldn't change it.

@trask trask merged commit 511f6b7 into open-telemetry:main Apr 6, 2023
@dmytr
Copy link
Contributor Author

dmytr commented Apr 6, 2023

Hi @laurit 👋 Seems like I'm a bit late to the party 😅

I think that you are right and in most cases context outside of the fiber wouldn't matter.

However I looked into the details of how zio-http library is implemented. It provides HTTP server based on Netty. It used to create custom ZIO runtime that was using threads spawned by Netty for its event loop group to execute ZIO effects (link). I guess it was working with the current version of ZIO instrumentation because Netty instrumentation was doing the right thing somehow.

zio-http's implementation was changed recently to use ZIO's own threads for execution. But I'm wondering if there are any libraries out there that would do something similar and it would matter if we restore the right context when fiber is suspended/ended 🤔

@laurit
Copy link
Contributor

laurit commented Apr 6, 2023

@dmytr Even with netty threads I would assume that the thread that is used for fiber doesn't have a meaningful scope. If it had a meaningful scope it would be in the middle of some other operation and getting borrowed to run the fiber would be strange.
If zio-http also needs instrumentation then when this instrumentation is added concurrency tests in our http server/client test suites could surface the issue if there is one.

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.

None yet

3 participants