-
Notifications
You must be signed in to change notification settings - Fork 608
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 5 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 |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
| from threading import Lock | ||
| from typing import TYPE_CHECKING | ||
|
|
||
| import sentry_sdk | ||
| from sentry_sdk.utils import logger | ||
|
|
||
| if TYPE_CHECKING: | ||
|
|
@@ -279,6 +280,28 @@ def setup_integrations( | |
| return integrations | ||
|
|
||
|
|
||
| def _enable_integration(integration: "Integration") -> "Optional[Integration]": | ||
| identifier = integration.identifier | ||
| client = sentry_sdk.get_client() | ||
|
|
||
| with _installer_lock: | ||
| if identifier in client.integrations: | ||
|
cursor[bot] marked this conversation as resolved.
Outdated
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. Missing global check allows duplicate
|
||
| logger.debug("Integration already enabled: %s", identifier) | ||
| return None | ||
|
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. I'm explicitly not allowing overwriting the old integration with the new one so that we don't end up double patching the event loop. We could add extra logic to restore the original code and re-patch that, but it seems like that's a lot of extra work for a usecase no one will probably need. |
||
|
|
||
| logger.debug("Setting up integration %s", identifier) | ||
| _processed_integrations.add(identifier) | ||
| try: | ||
| type(integration).setup_once() | ||
| integration.setup_once_with_options(client.options) | ||
| except DidNotEnable as e: | ||
| logger.debug("Did not enable integration %s: %s", identifier, e) | ||
| return None | ||
| else: | ||
| _installed_integrations.add(identifier) | ||
| return integration | ||
|
|
||
|
|
||
| def _check_minimum_version( | ||
| integration: "type[Integration]", | ||
| version: "Optional[tuple[int, ...]]", | ||
|
|
||
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.