Revert "Enable Sentry asyncio integration (#5685)" #5729
Conversation
This essentially reverts PR #5685. The Sentry `AsyncioIntegration` replaces the asyncio task factory with its instrumentalized version, which reports all execeptions which aren't handled *within* a task to Sentry. However, we quite often run tasks and handle exceptions outside, e.g. this commen pattern (example from `MountManager` `reload()``): ```python results = await asyncio.gather( *[mount.update() for mount in mounts], return_exceptions=True ) ... create resolution issues from results with exceptions ... ``` Here, asyncio.gather() uses ensure_future(), which converts the co-routines to tasks. These Sentry instrumented tasks will then report exceptions to Sentry, even though we handle exceptions gracefully. So the `AsyncioIntegration` doesn't work for our use case, and causes unnecessary noise in Sentry. Disable it again.
agners
added
the
refactor
📝 WalkthroughWalkthroughThe change removes the Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Sentry as Sentry SDK
App->>Sentry: Call init_sentry()
Note right of Sentry: Initialize with default integrations\n(excludes AsyncioIntegration)
Sentry-->>App: Sentry setup complete
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
supervisor/utils/sentry.py (2)
33-56: Consider adding integration tests for error handlingSince a key error reporting mechanism is being disabled, it would be beneficial to add specific tests that verify exceptions in asyncio tasks are properly caught and handled as intended without the
AsyncioIntegration.
25-56: Ensure consistent error tracking for critical unhandled exceptionsWhile removing the
AsyncioIntegrationaddresses the false positive issue, ensure critical errors aren't missed. For truly unhandled exceptions that should be reported to Sentry, you might want to consider implementing a custom exception handler that gives you more control over what gets reported.You could implement a custom error handler that's more selective about which exceptions are reported to Sentry:
def custom_exception_handler(loop, context): """Custom exception handler that selectively reports to Sentry.""" exception = context.get('exception') # Only report exceptions that aren't expected to be handled elsewhere if exception and not isinstance(exception, (ExpectedExceptionType1, ExpectedExceptionType2)): asyncio.create_task(async_capture_exception(exception)) # Call the default exception handler loop.default_exception_handler(context) # Set up the custom exception handler loop = asyncio.get_event_loop() loop.set_exception_handler(custom_exception_handler)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
supervisor/utils/sentry.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
- GitHub Check: Run tests Python 3.13.2
🔇 Additional comments (2)
supervisor/utils/sentry.py (2)
30-32: Well-documented removal of AsyncioIntegrationThe comment clearly explains why
AsyncioIntegrationisn't being used, which helps future maintainers understand the design decision. This aligns with the PR's objective of reducing unnecessary noise in Sentry by preventing the reporting of exceptions that are already being handled gracefully outside of tasks.
38-53:❓ Verification inconclusive
Verify error handling patterns in asyncio code
The remaining integrations look appropriate, but with the removal of
AsyncioIntegration, it's important to ensure that the codebase consistently handles exceptions outside of tasks as mentioned in the comment. This approach should be verified, particularly in code that usesasyncio.gather().
🏁 Script executed:
#!/bin/bash # Description: Find usages of asyncio.gather() to verify consistent exception handling patterns # Check for patterns where exceptions might be handled (with try/except) after asyncio.gather() calls echo "Searching for asyncio.gather() usage patterns:" rg -A 10 "asyncio\.gather\(" --type py | grep -B 5 -A 5 "gather" echo "\nChecking for try/except blocks around asyncio.gather():" rg -B 5 -A 10 "try:.*\n.*asyncio\.gather\(" --multiline --type pyLength of output: 31144
Attention: Confirm Consistent Exception Handling in Asyncio Code
The Sentry integrations now look appropriate in this file. However, with the removal of
AsyncioIntegration, please verify that all code usingasyncio.gather()handles exceptions correctly. Our initial search shows widespread usage ofasyncio.gather()(often withreturn_exceptions=True) across the codebase (for example, insupervisor/store/__init__.py), but explicit try/except wrappers around these calls were not found. This suggests that exception handling is being managed by checking return values rather than wrapping the calls directly.
- Review Task: Ensure that in all critical async workflows (e.g., in
supervisor/store/__init__.py,supervisor/api/store.py, andsupervisor/backups/manager.py), errors fromasyncio.gather()are captured and handled appropriately.- Action: Confirm that using
return_exceptions=Trueis sufficient in your context or add additional error handling if necessary, especially for exceptions occurring outside task scopes.Please manually verify these patterns across the codebase to ensure no unhandled cases occur due to the removal of
AsyncioIntegration.
Proposed change
This essentially reverts PR #5685.
The Sentry
AsyncioIntegrationreplaces the asyncio task factory with its instrumentalized version, which reports all execeptions which aren't handled within a task to Sentry.However, we quite often run tasks and handle exceptions outside, e.g. this commen pattern (example from
MountManager`reload()``):Here, asyncio.gather() uses ensure_future(), which converts the co-routines to tasks. These Sentry instrumented tasks will then report exceptions to Sentry, even though we handle exceptions gracefully.
So the
AsyncioIntegrationdoesn't work for our use case, and causes unnecessary noise in Sentry. Disable it again.See also AsyncioIntegration implementation: https://github.com/getsentry/sentry-python/blob/2.22.0/sentry_sdk/integrations/asyncio.py#L32
Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit