getsentry / sentry-python

The official Python SDK for Sentry.io
https://sentry.io/for/python/
MIT License
1.86k stars 486 forks source link

`ensure_integration_enabled_async` doesn't work sometimes #2892

Open sentrivana opened 6 months ago

sentrivana commented 6 months ago

Two different ways in which the decorator seems to alter the behavior of the decorated function. Unsure whether it's the same issue or two different ones.

1. Repro via a simple Starlette app

Install sentry-sdk==2.0.0, uvicorn, starlette.

Create a simple Starlette app as app.py:

import sentry_sdk
from starlette.applications import Starlette

sentry_sdk.init()

app = Starlette()

Run with uvicorn app:app.

Request http://127.0.0.1:8000/ (or wherever your server is running). We didn't define any routes so it's not expected to resolve -- you should get a 404. Instead you'll get a 500:

ERROR:    Exception in ASGI application
Traceback (most recent call last):
  File "/Users/ivana/dev/repro/starlette-uvicorn/starlette.env/lib/python3.12/site-packages/uvicorn/protocols/http/h11_impl.py", line 407, in run_asgi
    result = await app(  # type: ignore[func-returns-value]
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ivana/dev/repro/starlette-uvicorn/starlette.env/lib/python3.12/site-packages/uvicorn/middleware/proxy_headers.py", line 69, in __call__
    return await self.app(scope, receive, send)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ivana/dev/repro/starlette-uvicorn/starlette.env/lib/python3.12/site-packages/uvicorn/middleware/asgi2.py", line 14, in __call__
    instance = self.app(scope)
               ^^^^^^^^^^^^^^^
TypeError: 'coroutine' object is not callable

If you remove the ensure_integration_enabled_async decorator from the Starlette integration, rerun the server and request / again, you'll get a 404 as expected.

2. Repro via tests

  1. Open the Starlite integration.
  2. Decorate the _create_span_call wrapper with the ensure_integration_enabled_async decorator.
  3. Remove the now unnecessary check in the wrapper body here.
  4. Run the Starlite test suite.

The tests will fail, even though the code should behave the same way as before the change.

This can also be observed with Starlette in CI on this PR: these are the failing tests, and this change makes the tests green.

I've observed this in other integrations as well, so it appears to be a problem with the decorator itself. The sync version of the decorator works without issues.

antonpirker commented 4 months ago

What I just noticed. In our starlette integration when using the decorator on _sentry_patched_asgi_app the error occurs. In the stacktrace above it is in asgi2.py in uvicorn. And our integration calls our internal _run_asgi3. So maybe it has something to do with ASGI 2 and 3 protocoll differences.

jonathanberthias commented 4 months ago

In my investigation, I found that the issue comes from this part in uvicorn:

https://github.com/encode/uvicorn/blob/0efd3835da6dcc713f74aadf7b52779d0d1fa17d/uvicorn/config.py#L439-L459

Basically, it tries to call the "app" (which can be an app factory at this point) with no arguments, and it expects the call to fail if you have an app and not a factory. However, the patched app.__call__ succeeds with no arguments, which leads uvicorn to think it was given an app factory instead of an app (hence the WARNING: ASGI app factory detected. Using it, but please consider setting the --factory flag explicitly. in #3021).

Then, the inspect.isfunction(self.loaded_app) returns false though I wasn't able to pinpoint why at the moment. This leads to uvicorn calling the app with ASGI2.

Hope this helps a little!