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

Not compatible with gevent #628

Closed
aberres opened this issue Apr 23, 2019 · 7 comments
Closed

Not compatible with gevent #628

aberres opened this issue Apr 23, 2019 · 7 comments

Comments

@aberres
Copy link
Contributor

aberres commented Apr 23, 2019

Depending on the availability of the contextvars module either a _ThreadLocalRuntimeContext or a _AsyncRuntimeContext is created. The former uses threading.local while the latter uses contextvars.ContextVar to store data.

gevent only supports threading.local properly. The contexts are not switched when contextvars.ContextVar is used. This results in side effects when requests are executed in parallel.

An extended writeup can be found in this issue: elastic/apm-agent-python#451

A solution to this problem is to try to detect if running under gevent and then using _ThreadLocalRuntimeContext even if contextvars are available. An implementation could be heavily inspired by this one: elastic/apm-agent-python@090b938

@c24t
Copy link
Member

c24t commented Apr 24, 2019

Thanks for raising the issue @aberres. We've got the same problem with other event-loop-based frameworks. E.g., we'd have to use StackContext to store the runtime context in tornado (at least in versions < 5.1, thankfully they moved to contextvars in later versions).

One problem is that we might have competing threading models: suppose we force RuntimeContext to use threadlocals when we detect that gevent is patching threading, what happens if a user calls an asyncio coroutine from a greenlet?

If we do want to make using threadlocals a configurable option, one solution may be to let integrations swap out the storage for RuntimeContext (similar to the suggestion in elastic/apm-agent-python#16) and add an opencensus-ext-gevent package that does just that.

@reyang may have some insight on the design of the context API here.

@reyang
Copy link
Contributor

reyang commented Apr 25, 2019

Thanks for sharing the findings @aberres!

"Trying to detect if running under gevent and then using _ThreadLocalRuntimeContext even if contextvars are available" would lead us to more problems as @c24t mentioned.

Eventually gevent will move to contextvars given it is the direction of Python runtime. I think it makes sense to provide an explicit way to use threading.local as a temporary workaround.

Would love to hear your idea on what the explicit way should look like. Probably some environment variable, or OPENCENSUS global settings? (I'm a bit concerned about swapping context at runtime, as there could be context creation done before the point we start to swap things)

@aberres
Copy link
Contributor Author

aberres commented Apr 29, 2019

I'm a bit concerned about swapping context at runtime, as there could be context creation done before the point we start to swap things

At least from a gevent POV this should not be a problem. To properly setup gevent one should patch all used libraries before loading/using them for the first time. An example is using the gunicorn gevent worker and doing any needed preparation in a post_fork implementation.

@reyang
Copy link
Contributor

reyang commented Apr 29, 2019

@aberres would the following code work?

import opencensus.common.runtime_context as runtime_context
runtime_context.RuntimeContext = runtime_context._ThreadLocalRuntimeContext()

# import other opencensus components
# work with gevent

@aberres
Copy link
Contributor Author

aberres commented Apr 30, 2019

@reyang Yes, this works.

I created a pull request to add some lines to the documentation instead of doing any code changes.

@aberres
Copy link
Contributor Author

aberres commented May 3, 2019

Closed with #636

@AyWa
Copy link

AyWa commented Jan 29, 2020

Maybe I will create a new issue but:

opencensus==0.7.6
opencensus-ext-flask==0.7.3
opencensus-ext-requests==0.7.2
opencensus-ext-zipkin==0.2.2
opencensus-ext-gevent==0.1.0

With

Flask==1.0.2
gevent==1.4.0

And a setup like:

middleware = FlaskMiddleware(app)
config_integration.trace_integrations(['requests'])

it does not seems to trace properly when

gevent.spawn(requests.post, "XXX")

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

No branches or pull requests

4 participants