-
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
base: master
Are you sure you want to change the base?
Conversation
sentry_sdk/integrations/__init__.py
Outdated
| with _installer_lock: | ||
| if identifier in client.integrations: | ||
| logger.debug("Integration already enabled: %s", identifier) | ||
| return None |
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 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.
sentry_sdk/integrations/__init__.py
Outdated
| client = sentry_sdk.get_client() | ||
|
|
||
| with _installer_lock: | ||
| if identifier in client.integrations: |
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.
Missing global check allows duplicate setup_once() after re-init
Medium Severity
The _enable_integration function only checks client.integrations (specific to the current client), but doesn't check _installed_integrations (the global set tracking all successfully installed integrations). The original setup_integrations uses _processed_integrations to prevent calling setup_once() multiple times since it's a static method that patches global state. When a user re-initializes Sentry (calling init() again), the new client has an empty integrations dict, so _enable_integration will call setup_once() again even though the event loop was already patched by a previous client, causing double instrumentation.
alexander-alderman-webb
left a comment
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.
Nice, this will be useful!
| self.monitor: "Optional[Monitor]" = None | ||
| self.log_batcher: "Optional[LogBatcher]" = None | ||
| self.metrics_batcher: "Optional[MetricsBatcher]" = None | ||
| self.integrations: "dict[str, 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.integrations and the sentry_sdk.integrations._installed_integrations global?
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.)- On the other hand,
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_integrations is process-wide and means "this package has been patched", and client.integrations is 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_integrations correctly in this PR. Will fix
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.
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 🤡 36ef8cb
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 AsyncioIntegration before the event loop was started.
So looks good to me (apart from Seer's comment that may need to be addressed)!
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_integrations machinery (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 the AsyncioIntegration only instead of having a more general _enable_integration function.
sentry_sdk/integrations/__init__.py
Outdated
| if identifier not in _installed_integrations or identifier == "asyncio": | ||
| # Asyncio is special because it patches the currently running event |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
sentry_sdk/integrations/asyncio.py
Outdated
| sentry_sdk.init(disabled_integrations=[...]), this function will ignore that | ||
| and the integration will be enabled. | ||
| """ | ||
| _enable_integration(AsyncioIntegration(*args, **kwargs)) |
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.
Docstring claims argument passing works but integration rejects arguments
Low Severity
The enable_asyncio_integration function signature accepts *args, **kwargs and the docstring explicitly states "Any arguments provided will be passed to AsyncioIntegration() as is." However, AsyncioIntegration doesn't define a custom __init__ method, so it uses object.__init__ which accepts no arguments. Users who follow the documentation and pass arguments will get a TypeError. The test test_delayed_enable_integration_with_options appears to verify this feature but only passes because it mocks __init__, masking the actual broken behavior.
Additional Locations (1)
| # 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Calling enable_asyncio_integration() with any arguments will raise a TypeError because AsyncioIntegration lacks an __init__ method to accept them.
Severity: CRITICAL
🔍 Detailed Analysis
The enable_asyncio_integration() function passes *args and **kwargs to the AsyncioIntegration constructor. However, neither AsyncioIntegration nor its base class Integration define an __init__ method. They inherit object.__init__(), which accepts no arguments. Consequently, any attempt to call enable_asyncio_integration() with arguments, as the docstring suggests, will result in a TypeError. This is confirmed by a test that explicitly mocks AsyncioIntegration.__init__ to avoid this exact error, indicating the bug is known but unaddressed in the implementation.
💡 Suggested Fix
Add an __init__ method to the AsyncioIntegration class that accepts *args and **kwargs. The method body can simply be pass if the arguments are not used. This will prevent the TypeError from object.__init__ and align the function's behavior with its documentation.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: sentry_sdk/integrations/asyncio.py#L186
Potential issue: The `enable_asyncio_integration()` function passes `*args` and
`**kwargs` to the `AsyncioIntegration` constructor. However, neither
`AsyncioIntegration` nor its base class `Integration` define an `__init__` method. They
inherit `object.__init__()`, which accepts no arguments. Consequently, any attempt to
call `enable_asyncio_integration()` with arguments, as the docstring suggests, will
result in a `TypeError`. This is confirmed by a test that explicitly mocks
`AsyncioIntegration.__init__` to avoid this exact error, indicating the bug is known but
unaddressed in the implementation.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8487723
Initializing the SDK in an async environment can be a challenge, since the requirement to initialize as early as possible might clash with the requirement to initialize when there's an event loop running, if one wants to use the AsyncioIntegration. These sort of scenarios would benefit from a dedicated way to activate the integration after
init()has been called.Supersedes the needlessly more general #5285