-
Notifications
You must be signed in to change notification settings - Fork 572
feat(asyncio): Add on-demand way to enable AsyncioIntegration #5288
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
Changes from all commits
42d7b15
c1dd56a
e6651e4
0042efa
dd5b71e
c447df0
36ef8cb
0193dea
d0db5af
8903c06
26802c7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,6 +47,10 @@ def patch_asyncio() -> None: | |
| loop = asyncio.get_running_loop() | ||
| orig_task_factory = loop.get_task_factory() | ||
|
|
||
| # Check if already patched | ||
| if getattr(orig_task_factory, "_is_sentry_task_factory", False): | ||
| return | ||
|
|
||
| def _sentry_task_factory( | ||
| loop: "asyncio.AbstractEventLoop", | ||
| coro: "Coroutine[Any, Any, Any]", | ||
|
|
@@ -102,6 +106,7 @@ async def _task_with_sentry_span_creation() -> "Any": | |
|
|
||
| return task | ||
|
|
||
| _sentry_task_factory._is_sentry_task_factory = True # type: ignore | ||
| loop.set_task_factory(_sentry_task_factory) # type: ignore | ||
|
|
||
| except RuntimeError: | ||
|
|
@@ -138,3 +143,48 @@ class AsyncioIntegration(Integration): | |
| @staticmethod | ||
| def setup_once() -> None: | ||
| patch_asyncio() | ||
|
|
||
|
|
||
| def enable_asyncio_integration(*args: "Any", **kwargs: "Any") -> None: | ||
| """ | ||
| Enable AsyncioIntegration with the provided options. | ||
|
|
||
| This is useful in scenarios where Sentry needs to be initialized before | ||
| an event loop is set up, but you still want to instrument asyncio once there | ||
| is an event loop. In that case, you can sentry_sdk.init() early on without | ||
| the AsyncioIntegration and then, once the event loop has been set up, | ||
| execute: | ||
|
|
||
| ```python | ||
| from sentry_sdk.integrations.asyncio import enable_asyncio_integration | ||
|
|
||
| async def async_entrypoint(): | ||
| enable_asyncio_integration() | ||
| ``` | ||
|
|
||
| Any arguments provided will be passed to AsyncioIntegration() as is. | ||
|
|
||
| If AsyncioIntegration has already patched the current event loop, this | ||
| function won't have any effect. | ||
|
|
||
| If AsyncioIntegration was provided in | ||
| sentry_sdk.init(disabled_integrations=[...]), this function will ignore that | ||
| and the integration will be enabled. | ||
| """ | ||
| client = sentry_sdk.get_client() | ||
| if not client.is_active(): | ||
| return | ||
|
|
||
| # This function purposefully bypasses the integration machinery in | ||
| # integrations/__init__.py. _installed_integrations/_processed_integrations | ||
| # is used to prevent double patching the same module, but in the case of | ||
| # the AsyncioIntegration, we don't monkeypatch the standard library directly, | ||
| # we patch the currently running event loop, and we keep the record of doing | ||
| # that on the loop itself. | ||
| logger.debug("Setting up integration asyncio") | ||
|
|
||
| integration = AsyncioIntegration(*args, **kwargs) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Calling 🔍 Detailed AnalysisThe 💡 Suggested FixAdd an 🤖 Prompt for AI AgentDid we get this right? 👍 / 👎 to inform future reviews.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is fine, they're there to make this future-proof in case we add args to the integration. |
||
| integration.setup_once() | ||
|
|
||
| if "asyncio" not in client.integrations: | ||
| client.integrations["asyncio"] = integration | ||
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.
For my understanding, and no action required:
do you know if there is a reason we have both
Client.integrationsand thesentry_sdk.integrations._installed_integrationsglobal?Uh oh!
There was an error while loading. Please reload this page.
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.
Afaik the general idea is:
_installed_integrationskeeps track of what integrations have had theirsetup_oncealready run in the current process.setup_onceshould, as the name suggests, only be run (i.e., monkeypatch things) once, so even if you, for example, set up a client with an integration and then set up a different client with the same integration, the latter client should not runsetup_onceon that integration again. (Setting up multiple clients is in general a very niche scenario though.)client.integrationstracks the integrations that a specific client is using. So for example, if you'd enabled integration A before in another client, itssetup_oncewould have run, and it would be in_installed_integrations. If you then create another client that shouldn't have A active, A will not have itssetup_oncerun again, but it will still be patched, and we'll useclient.integrationsto check whether the patch should actually be applied or if we should exit early (this pattern).So
_installed_integrationsis process-wide and means "this package has been patched", andclient.integrationsis client-specific, saying "this patch should actually be applied", to allow for these sorts of multi-client scenarios.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.
Now that I've looked into this a bit more I see that I'm not checking
_installed_integrationscorrectly in this PR. Will fixThere 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.
So AsyncioIntegration is special in that it patches the current event loop, and not anything global in asyncio, so it actually should not be affected by
_installed_integrations🤡 36ef8cbUh oh!
There was an error while loading. Please reload this page.
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.
For the server frameworks there should be one loop per process afaict, but letting user's re-initialize would still be good if a user also initializes the SDK with
AsyncioIntegrationbefore the event loop was started.So looks good to me (apart from Seer's comment that may need to be addressed)!
Uh oh!
There was an error while loading. Please reload this page.
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.
Added a bool to the patched event loop to be able to tell whether it's already been patched. That should take care of always being able to patch if not patched, and avoiding double-patching. And since this completely bypasses the
_installed_integrationsmachinery (because knowing if we've patched in the current process has no value in the case of the asyncio integration), I opted for making everything specific to theAsyncioIntegrationonly instead of having a more general_enable_integrationfunction.