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

Stop using fiber-local variables #174

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

coding-chimp
Copy link

GraphQL added a fiber-based batch loading API in one of their recent releases. Since the GraphQL execution is happening inside fibers and skylight is using fiber-local variables in the instrumenter to keep track of the current trace, it's pretty much breaking tracing inside the GraphQL runtime.

To fix this I changed the usage of fiber-local variables to thread-local. We're currently using this in production and it looks good.

I've thrown together a little app that I used for testing. I will also provide a link to one of our endpoints in the in-app messenger. I've been already talking to @zvkemp about this there.

@cla-bot
Copy link

cla-bot bot commented Feb 18, 2021

We require contributors to sign our Contributor License Agreement, and we don't have @coding-chimp on file. In order for us to review and merge your code, please sign the CLA.

@cla-bot
Copy link

cla-bot bot commented Feb 18, 2021

We require contributors to sign our Contributor License Agreement, and we don't have @coding-chimp on file. In order for us to review and merge your code, please sign the CLA.

@zvkemp
Copy link
Contributor

zvkemp commented Feb 19, 2021

@coding-chimp The test failures were caused by an update to Rails, and have been fixed on the master branch. Can you rebase this branch when you have a chance?

We'll review this internally to validate that the thread-local variables still work as expected.

@cla-bot
Copy link

cla-bot bot commented Feb 19, 2021

We require contributors to sign our Contributor License Agreement, and we don't have @coding-chimp on file. In order for us to review and merge your code, please sign the CLA.

@zvkemp
Copy link
Contributor

zvkemp commented Mar 3, 2021

Hi @coding-chimp, we'll continue to investigate this issue, but as of right now it does not appear that moving to thread-local variables will be a one-size-fits-all solution. It's somewhat difficult to survey the entire Ruby ecosystem for Fiber usage (most results on Github appear to be small experiments), but notably Falcon uses fibers for request concurrency, so Skylight would need to remain fiber-local in that context.

@coding-chimp
Copy link
Author

Hey, @zvkemp, thanks for the update. I didn't really know about Falcon when I opened this PR. This certainly makes things more complicated.

Not sure if it's helpful, but there's now also an issue about this on the DataDog agent.

@zvkemp
Copy link
Contributor

zvkemp commented May 18, 2021

There's been some additional work done in GraphQL's Dataloader on this:

rmosolgo/graphql-ruby#3461

Putting this here for reference only; I'm not yet convinced that was this was necessarily a good idea.

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

2 participants