Span should support async context manager protocol

[notypecheck]

Problem Statement

Span and Transaction should support async context manager protocol to avoid unnecessary nesting.

As an example:

async def main() -> None:
    async with a:
        with sentry_sdk.start_transaction(
            op="op",
            name="name",
        ):
            with sentry_sdk.start_span(op="b"):
                async with b:
                    await b.run()

            with sentry_sdk.start_span(op="c"):
                async with c:
                    await c.run()

Could be shortened to

async def main() -> None:
    async with a, sentry_sdk.start_transaction(
        op="op",
        name="name",
    ):
        async with sentry_sdk.start_span(op="b"), b:
            await b.run()

        async with sentry_sdk.start_span(op="c"), c:
            await c.run()

Solution Brainstorm

I think currently __aenter__ and __aexit__ could do the same as their synchronous counterparts and later could be replaced with async code, if that would be possible at some point 🤔

[antonpirker]

Hey @ThirVondukr !

Thanks for bringing this up. Yes, our async Python support needs some improvements.
I put this on our backlog, but we are quite swamped right now, so it will take some time for us to implement this.

But pull requests are always welcomed, if you (or anyone else) would like to give it a try!

[notypecheck]

@antonpirker I think to make these objects compatible with async (just to reduce nesting for now) they could call their sync counterparts, would that implementation suffice? 🤔

[antonpirker]

Probably! If you submit a PR, I am happy to review!

[isra17]

Edit: Seems like I was wrong, the default client should dispatch work in the background. Unless you explicitely flush, it should not block.

This is a pretty bad idea as per #1735 .

You would pretty much be lying through the API signature that sentry is async-compatible while it’s clearly not. As it turns out, all the async framework integration is very bad as well. Any error will stop the whole event loop until the event has been captured and sent to the sentry server. This is a big no-no for async code.

In my opinion, the correct thing right now would be to put a big warning about how sentry integration are not working as intended for async codes and framework and that big performance penalty will come with their use.

This might come out a bit harsh, but I just come out of a performance debug session about a long standing issue and I was quite surprised that only one person raised this flag in the issues. Doing async API is not as easy as slapping the async keyword in front of any function.

[notypecheck]

@isra17 This could be an issue, but it depends on your workload, since event is sent only when you exit the transaction, so it's not a big issue for low RPS operations/processes, like ETL
Maybe it would be possible to use something like greenlet here?

[isra17]

No it doesn't depend on your workload, it's always an issue, and a critical one at that. Sentry is not async safe. It's going to block your whole event loop each time it capture an event.

It's like saying that non-threadsafe code is not an issue when using threads as long as you get lucky and don't run into the race conditions. You are either threadsafe or not threadsafe the same way you are either async or not async.

I don't see how greenlet fix anything in relation to async.

For what it's worth, in relation to your original post, you might as well just have a small helper, ie:

async def asynccontext(ctx):
  with ctx as value:
    yield value

async with a, asynccontext(sentry_sdk.start_transaction()):
  ...

But really, using sentry with async code is just a plain bad idea in its current form.

[isra17]

Actually it seems like the default transport use a background worker to send events, I still need to investigate why it seems to block the event loop 🤔