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.

Sign up for GitHub

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

@traced_function now works with AsyncIO's coroutines. #84

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

@traced_function now works with AsyncIO's coroutines. #84

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5c88b14
@traced_function now works with AsyncIO's coroutines.
May 23, 2019
6bbcf2c
Checking for correct exception and simplified logic on is_asyncio_cor…
Jun 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions opentracing_instrumentation/local_span.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,16 @@
import tornado.concurrent
from . import get_current_span, span_in_stack_context, utils

try:
import asyncio
_ASYNCIO = True
except Exception:
nicholasamorim marked this conversation as resolved.
Show resolved Hide resolved
_ASYNCIO = False


def is_asyncio_coroutine(func):
return False if not _ASYNCIO else asyncio.iscoroutine(func)
nicholasamorim marked this conversation as resolved.
Show resolved Hide resolved


def func_span(func, tags=None, require_active_trace=False):
"""
Expand Down Expand Up @@ -128,25 +138,31 @@ def decorator(*args, **kwargs):
with span_in_stack_context(span) as deactivate_cb:
try:
res = func(*args, **kwargs)
is_tornado = tornado.concurrent.is_future(res)
is_asyncio = is_asyncio_coroutine(res)
if is_asyncio:
task = asyncio.create_task(res)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@nicholasamorim nicholasamorim May 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @nicholasamorim and @condorcet,
Any updates here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these changes definitely should be proved, so we have to add some tests first.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jamim @condorcet

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

else:
task = res
# Tornado co-routines usually return futures, so we must wait
# until the future is completed, in order to accurately
# capture the function's execution time.
if tornado.concurrent.is_future(res):
if is_tornado or is_asyncio:
def done_callback(future):
deactivate_cb()
exception = future.exception()
if exception is not None:
span.log(event='exception', payload=exception)
span.set_tag('error', 'true')
span.finish()
if res.done():
if task.done():
done_callback(res)
else:
res.add_done_callback(done_callback)
task.add_done_callback(done_callback)
else:
deactivate_cb()
span.finish()
return res
return task
except Exception as e:
deactivate_cb()
span.log(event='exception', payload=e)
Expand Down