ref: Refactor ASGI middleware and improve contextvars error message #701
Conversation
sentry_sdk/integrations/asgi.py
Outdated
| if client and _should_send_default_pii(): | ||
| request_info["env"] = {"REMOTE_ADDR": client[0]} | ||
|
|
||
| if event.get("transaction", TRANSACTION_SENTINEL) == TRANSACTION_SENTINEL: |
There was a problem hiding this comment.
We need this to allow users to actually override the transaction in their own code which previously was not possible.
| return True, ContextVar | ||
| except ImportError: | ||
| pass | ||
| else: |
There was a problem hiding this comment.
This change is necessary as a bare contextvars installation from PyPI on Python 3.6 is totally useless for our purposes, in those cases we do want to return HAS_REAL_CONTEXTVARS = False
rhcarvalho
left a comment
rhcarvalho
left a comment
There was a problem hiding this comment.
Thanks @untitaker!
- I'm concerned that renaming/removing implicitly public methods now could break users. Do we follow a different stability guarantee in integrations?
Other than the above, you have my approval.
-
The part related to #700 seems clear and obvious. My only suggestion is adding a test case to avoid regression. Up to your decision.
-
How this is fixing #630 and #694 is a bit more nuanced. I understood part of fixing #694 is the more thorough error message and
HAS_REAL_CONTEXTVARS = False. For #630 is the error message documenting the situation better.
| request_info["query_string"] = self.get_query(asgi_scope) | ||
| ty = asgi_scope["type"] | ||
| if ty in ("http", "websocket"): | ||
| request_info["method"] = asgi_scope.get("method") |
There was a problem hiding this comment.
👍 this fixes #700
Shall we add a test case to cover this against regression?
There was a problem hiding this comment.
I did add a test to ensure websocket applications using that middleware do not crash. There are some issues with websockets crash reporting that I think I need to investigate separately.
There was a problem hiding this comment.
The test did not bother whether we do asgi.scope["method"] (the original problem from #700) or asgi.scope.get("method") (the minimal fix, I believe):
$ git df -- sentry_sdk/integrations/ ref/asgi ✭ ✱ ◼
diff --git a/sentry_sdk/integrations/asgi.py b/sentry_sdk/integrations/asgi.py
index 202c490..d9e247f 100644
--- a/sentry_sdk/integrations/asgi.py
+++ b/sentry_sdk/integrations/asgi.py
@@ -150,7 +150,7 @@ class SentryAsgiMiddleware:
ty = asgi_scope["type"]
if ty in ("http", "websocket"):
- request_info["method"] = asgi_scope.get("method")
+ request_info["method"] = asgi_scope["method"]
request_info["headers"] = headers = _filter_headers(
self._get_headers(asgi_scope)
)
$ pytest -k websocket ./tests/integrations/asgi/ ref/asgi ✭ ✱ ◼
================================================================================ test session starts =================================================================================
platform darwin -- Python 3.7.7, pytest-3.7.3, py-1.8.1, pluggy-0.13.1
rootdir: /Users/rodolfo/sentry/sentry-python, inifile: pytest.ini
plugins: localserver-0.5.0, forked-1.1.3, cov-2.8.1
collected 4 items / 3 deselected
tests/integrations/asgi/test_asgi.py . [100%]
======================================================================= 1 passed, 3 deselected in 0.13 seconds =======================================================================There was a problem hiding this comment.
You're right, thanks for catching that! I figured out why I was not able to get events through too so the test should be good now.
rhcarvalho
left a comment
rhcarvalho
left a comment
There was a problem hiding this comment.
Since this is a 0.x release, it is okay to note the breaking changes in the changelog and move on. For the other suggestions, I leave it up to @untitaker do decide what to do in the scope of this PR or not.
|
I think I addressed all feedback, please check again |
| installation of `contextvars` to avoid leaking scope/context data across | ||
| requests. | ||
|
|
||
| Please refer to https://docs.sentry.io/platforms/python/contextvars/ for more information. |
| request_info["query_string"] = self.get_query(asgi_scope) | ||
| ty = asgi_scope["type"] | ||
| if ty in ("http", "websocket"): | ||
| request_info["method"] = asgi_scope.get("method") |
Found multiple issues with the asgi middleware:
Fix #630
Fix #700
Fix #694