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

Sentry background worker is chronically blocking async event loop when many exceptions are raised #2824

Open
cpvandehey opened this issue Mar 14, 2024 · 5 comments
Labels
Component: SDK Core Dealing with the core of the SDK Component: Transport Dealing with the transport Enhancement New feature or request

Comments

@cpvandehey
Copy link

How do you use Sentry?

Self-hosted/on-premise

Version

1.40.6

Steps to Reproduce

Hello! And thanks for reading my ticket :)

The python sentry client is a synchronous client library that is retrofitted to fit the async model (by spinning off separate threads to avoid disrupting the event loop thread -- see background worker (1) for thread usage).

Under healthy conditions, the sentry client doesn’t need to make many web requests. However, if conditions become rocky and exceptions are frequently raised (caught or uncaught), the sentry client may become an extreme inhibitor to the app event loop (assuming high sample rate). This is due to the necessary OS thread context switching that effectively pauses/blocks the event loop to work on other threads (i.e the background worker (1)). This is not a recommended pattern (obviously) due to the costs of switching threads, but can be useful for quickly/lazily retrofitting sync code.

Relevant flow - in short:
Every time an exception is raised (caught or uncaught) in my code, a web request is immediately made to dump the data to sentry when sampled. Since sentry’s background worker is thread based (1), this will trigger an thread context switch and then a synchronous web request to dump the data to sentry. When applications receive many exceptions in a short period of time, this becomes a context switching nightmare.

Suggestion:
In an ideal world, sentry would asyncify its Background worker to use a task (1) and its transport layer (2) would use aiohttp. I don't think this is of super high complexity, but I could be wrong.

An immediate workaround could be made with more background worker control. If sentry’s background worker made web requests to dump data at configurable intervals, it would behave far more efficiently for event loops apps. At the moment, the background worker always dumps data immediately with regards to exceptions. In my opinion, if sentry is flushing data at app exit, having a 60 second timer to dump data would alleviate most of the symptoms I described above without ever losing data (albeit it would be up to 60 seconds slower).

(1) -

class BackgroundWorker(object):

(2) -

response = self._pool.request(

Expected Result

I expect to have less thread context switching when using sentry.

Actual Result

I see a lot of thread context switching when there are high exception rates.

@antonpirker
Copy link
Member

Hey @cpvandehey ! Thanks for the great ticket!

@antonpirker antonpirker added Enhancement New feature or request Component: SDK Core Dealing with the core of the SDK labels Mar 18, 2024
@sentrivana
Copy link
Contributor

Hey @cpvandehey, thanks for writing in. Definitely agree with you that our async support could definitely use some improvements (see e.g. #1735, #2007, #2184, and multiple other issues).

Using an aiohttp client and an asyncio task both sounds doable and would go a long way in making the SDK more async friendly.

@sentrivana sentrivana added the Component: Transport Dealing with the transport label Mar 18, 2024
@antonpirker
Copy link
Member

We could detect if aiohttp is in the project and based on this enable the new async support automatically. (have not thought long about this if this could lead to problems though..)

@sentrivana sentrivana added this to the Better async support milestone Mar 18, 2024
@cpvandehey
Copy link
Author

cpvandehey commented Apr 29, 2024

Hey @sentrivana / @antonpirker , any update on the progress for this? Happy to help

@sentrivana
Copy link
Contributor

Hey @cpvandehey, no news on this, but PRs are always welcome if you feel like giving this a shot.

@antonpirker antonpirker removed this from the Better async support milestone Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: SDK Core Dealing with the core of the SDK Component: Transport Dealing with the transport Enhancement New feature or request
Projects
Status: No status
Development

No branches or pull requests

3 participants