@traced_function now works with AsyncIO's coroutines. #84
Conversation
| is_tornado = tornado.concurrent.is_future(res) | ||
| is_asyncio = is_asyncio_coroutine(res) | ||
| if is_asyncio: | ||
| task = asyncio.create_task(res) |
There was a problem hiding this comment.
What about span propagation? I'm afraid that it doesn't work properly here, because we add task to asyncio loop and next time asyncio may execute the task in another scope. See https://github.com/opentracing/opentracing-python/blob/master/opentracing/scope_managers/asyncio.py#L37
There was a problem hiding this comment.
Oh, wow, I hadn't seen that. I'm still new to all of this, so.. makes sense.
How can that be fixed? I'm guessing this should be fixed at the ScopeManager?
What happens on Tornado 6 (no more StackContext) - just use the AsyncIO scope manager?
There was a problem hiding this comment.
In my opinion the best way to solve the problem is make auto span propagation in asyncio code (including scheduling tasks) with scope manager. It can work something like this PR opentracing/opentracing-python#118
Current code of traced_function isn't ready for it now as I can see.
There was a problem hiding this comment.
Hi @nicholasamorim and @condorcet,
Any updates here?
There was a problem hiding this comment.
I think these changes definitely should be proved, so we have to add some tests first.
There was a problem hiding this comment.
I might be way over my head in this but these changes seems to work fine with the changes @condorcet merged on opentracing/opentracing-python#118
I've been using it with the ContextVarsScopeManager and propagation seems to be working fine
There was a problem hiding this comment.
@nicholasamorim Behavior of the decorator is different for AsyncioScopeManager and ContextVarsScopeManager and it's not obvious now. Should we aware users about it?
Also we need some unit tests :)
There was a problem hiding this comment.
Behavior of the decorator is different for AsyncioScopeManager and ContextVarsScopeManager and it's not obvious now.
How different would be? I don't understand the internals enough to see that. Looking at the decorator I'd expect it to be scopemanager-agnostic.
Can we "fix" what's different?
I can add some unit tests, sure, just trying to understand it all before.
There was a problem hiding this comment.
Sorry for delay!
The problem with AsyncioScopeManager still the same: it doesn't support auto context propagation, so if you setup this scope manager and use the decorator then wrapped coroutine won't take proper parent context. We have to provide and activate parent span explicitly as mentioned in docs https://github.com/opentracing/opentracing-python/blob/master/opentracing/scope_managers/asyncio.py#L37
I believe it's possible to write a magic wrapper that do something for us. But in my opinion it makes code more complex (at least for debugging) and the best way just to mention that in asyncio application traced_function should be used with ContextVarsScopeManager.
Ask me If you need additional explanation!
Also notice this part of code changed in master.
|
Thanks for you suggestions @Jamim. I will look into this PR that was suggested by @condorcet opentracing/opentracing-python#118 and see how I can improve this. Accepting any pointers, though. |
|
Nicholas Amorim seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
When using Tornado 5 it's common to start migrating to
async/awaitsyntaxes.However, @traced_function only understands Tornado's Futures but not necessarily AsyncIO coroutines.
This PR adds supports to it without breaking compatibility with older versions of Python.