feat(profiling): Set active thread id for ASGI frameworks #1778
Conversation
When running in ASGI sync views, the transaction gets started in the main thread then the request is dispatched to a handler thread. We want to set the handler thread as the active thread id to ensure that profiles will show it on first render.
Zylphrex
force-pushed
the
txiao/feat/set-active-thread-id-for-asgi-frameworks
branch
from
7d648df
to
9724720
Compare
| @@ -62,6 +65,21 @@ def patch_get_request_handler(): | |||
|
|
|||
| def _sentry_get_request_handler(*args, **kwargs): | |||
| # type: (*Any, **Any) -> Any | |||
| dependant = kwargs.get("dependant") | |||
| if dependant and not asyncio.iscoroutinefunction(dependant.call): | |||
There was a problem hiding this comment.
Is it safe to use asyncio here for this?
There was a problem hiding this comment.
Yes I think so.
We vendored this from starlette for doing these coroutine checks.
@antonpirker knows this stuff better since he worked on it.
| from quart.signals import ( # type: ignore | ||
| got_background_exception, | ||
| got_request_exception, | ||
| got_websocket_exception, | ||
| request_started, | ||
| websocket_started, | ||
| ) | ||
| from quart.utils import is_coroutine_function # type: ignore |
There was a problem hiding this comment.
This was introduced in quart==0.11.1, do we need to support older versions?
There was a problem hiding this comment.
nope, tox says min 0.16.1, so all good
There was a problem hiding this comment.
left some random comments, but I want to discuss some high-level stuff first.
- We have a
ThreadingIntegration, should we update the scope there on run? - Would this be cleaner if we just patch the
run_in_threadpoolfunction in starlette or even lower level?
| @@ -62,6 +65,21 @@ def patch_get_request_handler(): | |||
|
|
|||
| def _sentry_get_request_handler(*args, **kwargs): | |||
| # type: (*Any, **Any) -> Any | |||
| dependant = kwargs.get("dependant") | |||
| if dependant and not asyncio.iscoroutinefunction(dependant.call): | |||
There was a problem hiding this comment.
Yes I think so.
We vendored this from starlette for doing these coroutine checks.
@antonpirker knows this stuff better since he worked on it.
| if dependant and not asyncio.iscoroutinefunction(dependant.call): | ||
| old_call = dependant.call | ||
|
|
||
| def _sentry_call(*args, **kwargs): |
There was a problem hiding this comment.
is setting the thread in fastapi still necessary if we already do it in request_response in starlette?
| from quart.signals import ( # type: ignore | ||
| got_background_exception, | ||
| got_request_exception, | ||
| got_websocket_exception, | ||
| request_started, | ||
| websocket_started, | ||
| ) | ||
| from quart.utils import is_coroutine_function # type: ignore |
There was a problem hiding this comment.
nope, tox says min 0.16.1, so all good
I thought about this but this would imply that any time a thread is started to do any work, it'll be marked as an active thread. So for use cases like background work, this would cause issues.
I looked into only patching starlette and leaving fastapi alone but these lower level functions are used for more than just starting sync requests. This would cause the same issue as adding this into the threading integration if someone were to use these lower level apis. |
| @@ -175,7 +176,7 @@ async def _run_app(self, scope, callback): | |||
|
|
|||
| with hub.start_transaction( | |||
| transaction, custom_sampling_context={"asgi_scope": scope} | |||
| ): | |||
| ), start_profiling(transaction, hub): | |||
There was a problem hiding this comment.
This will enable profiling for all the ASGI frameworks as well. This should be removed in #1797 as that will enable it for all transactions.
|
replaced by #1824 |
antonpirker
deleted the
txiao/feat/set-active-thread-id-for-asgi-frameworks
branch
When running in ASGI sync views, the transaction gets started in the main thread then the request is dispatched to a handler thread. We want to set the handler thread as the active thread id to ensure that profiles will show it on first render.