-
Notifications
You must be signed in to change notification settings - Fork 543
asyncio integration #1671
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
asyncio integration #1671
Conversation
…y/sentry-python into antonpirker/asyncio-integration
Co-authored-by: Neel Shah <neelshah.sa@gmail.com>
sentry_sdk/integrations/asyncio.py
Outdated
# WARNING: | ||
# If the default behavior of the task creation in asyncio changes, | ||
# this will break! | ||
task = Task(_coro_creating_hub_and_span(), loop=loop) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be
task = Task(_coro_creating_hub_and_span(), loop=loop) | |
task = Task(_coro_creating_hub_and_span, loop=loop) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but how did it work before lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does it work now, actually 😄 The correct version is the first one.
In [1]: from asyncio import Task
In [2]: async def foo():
...: print("test")
...:
In [3]: await Task(foo)
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
Input In [3], in <cell line: 1>()
----> 1 await Task(foo)
TypeError: a coroutine was expected, got <function foo at 0x7f7fb9241f30>
In [4]: await Task(foo())
test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, please test all cases @antonpirker
this is what cpython does
https://github.com/python/cpython/blob/main/Lib/asyncio/base_events.py#L436-L444
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, will test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really good catch @vmarkovtsev!
This lead to this fix: #1689
Co-authored-by: Neel Shah <neelshah.sa@gmail.com>
Make sure each asyncio task that is run has its own Hub and also creates a span.
Fixes #1333
Fixes #772