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

asyncio integration #1671

Merged
merged 25 commits into from Oct 17, 2022
Merged

asyncio integration #1671

merged 25 commits into from Oct 17, 2022

Conversation

antonpirker
Copy link
Member

@antonpirker antonpirker commented Oct 10, 2022

Make sure each asyncio task that is run has its own Hub and also creates a span.

Fixes #1333
Fixes #772

@antonpirker antonpirker marked this pull request as ready for review October 12, 2022 11:18
sentry_sdk/consts.py Outdated Show resolved Hide resolved
sentry_sdk/integrations/asyncio.py Outdated Show resolved Hide resolved
sentry_sdk/integrations/asyncio.py Outdated Show resolved Hide resolved
@antonpirker antonpirker enabled auto-merge (squash) October 14, 2022 11:21
sentry_sdk/integrations/asyncio.py Outdated Show resolved Hide resolved
sentry_sdk/integrations/asyncio.py Outdated Show resolved Hide resolved
# WARNING:
# If the default behavior of the task creation in asyncio changes,
# this will break!
task = Task(_coro_creating_hub_and_span(), loop=loop)
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be

Suggested change
task = Task(_coro_creating_hub_and_span(), loop=loop)
task = Task(_coro_creating_hub_and_span, loop=loop)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

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

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

Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, will test.

Copy link
Member Author

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

@antonpirker antonpirker merged commit 973b2f6 into master Oct 17, 2022
@antonpirker antonpirker deleted the antonpirker/asyncio-integration branch October 17, 2022 14:59
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.

Tracing: Some concurrently created Spans are not recorded. asyncio.gather tracing produces nested spans
4 participants