-
Notifications
You must be signed in to change notification settings - Fork 549
fix(Litestar): Apply failed_request_status_codes
to exceptions raised in middleware
#4074
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
base: master
Are you sure you want to change the base?
fix(Litestar): Apply failed_request_status_codes
to exceptions raised in middleware
#4074
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4074 +/- ##
==========================================
- Coverage 80.67% 80.66% -0.01%
==========================================
Files 142 142
Lines 15982 15988 +6
Branches 2729 2729
==========================================
+ Hits 12893 12897 +4
Misses 2232 2232
- Partials 857 859 +2
|
Relates to #3134 |
@sentrivana @antonpirker @untitaker can you review this please? 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution! I need some extra clarification on some items and also have some suggestions, please see the comments I left
sentry_sdk/integrations/asgi.py
Outdated
@@ -57,17 +57,6 @@ | |||
TRANSACTION_STYLE_VALUES = ("endpoint", "url") | |||
|
|||
|
|||
def _capture_exception(exc, mechanism_type="asgi"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to keep this function the same, as a standalone function instead of a method. I don't think it has to be modified to implement your changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
sentry_sdk/integrations/asgi.py
Outdated
def _capture_lifespan_exception(self, exc): | ||
# type: (Exception) -> None | ||
return self._capture_exception(exc) | ||
|
||
def _capture_request_exception(self, exc): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit unsure of the difference between these two functions. Please add docstrings to both to clarify the distinction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
sentry_sdk/integrations/litestar.py
Outdated
def _capture_request_exception(self, exc): | ||
# type: (Exception) -> None | ||
"""Ignore exceptions for requests here: we catch them in Litestar.after_exception handler.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am slightly unsure why this function is needed/how it relates to what you are trying to explain in this PR. Could you please explain in a bit more detail?
Also, I think you need a pass
here:
def _capture_request_exception(self, exc): | |
# type: (Exception) -> None | |
"""Ignore exceptions for requests here: we catch them in Litestar.after_exception handler.""" | |
def _capture_request_exception(self, exc): | |
# type: (Exception) -> None | |
"""Ignore exceptions for requests here: we catch them in Litestar.after_exception handler.""" | |
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, Litestar integration catches exceptions raised in middleware twice:
- In the custom ASGI handler.
- In the
Litestar.after_exception
handler, which is added inpatch_app_init
.
The custom ASGI handler doesn't filter exceptions using failed_request_status_codes
, so all exceptions (including those not filtered by the expected status code) appear in Sentry.
Interestingly, this behavior doesn't apply to exceptions raised in request handlers because Litestar prepends the built-in exception handling middleware.
This leads us to the conclusion that the ASGI handler only catches exceptions raised in middleware or lifespan handlers. It's still valuable to catch exceptions from lifespan handlers.
In this case, we patch _capture_request_exception
to prevent catching exceptions raised in middleware.
…LitestarASGIMiddleware
… SentryAsgiMiddleware for better clarity and support for derived integrations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @vrslev, apologies that this review took awhile for me to get around to.
Thanks for making the changes I requested and for explaining the two separate functions. These changes look good for the most part; however, before merging, I'd like us to add tests which ensure that any middleware exceptions which are not HttpException
s are still captured by the Litestar integration even after these changes.
sentry_sdk/integrations/litestar.py
Outdated
# type: (Exception) -> None | ||
"""Avoid catching exceptions from request handlers. | ||
|
||
Those exceptions are already han in Litestar.after_exception handler. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those exceptions are already han in Litestar.after_exception handler. | |
Those exceptions are already handled in Litestar.after_exception handler. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Hey @szokeasaurusrex, I've added the test you asked for :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, but I'd like a secondary review from someone else on the team before we merge
except RuntimeError: | ||
pass | ||
|
||
assert len(events) == 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd consider also asserting that the event is as expected (i.e. it is a runtime error with the message "Too Hot")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I’ll add it tomorrow 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
…ware-failed-request-status-codes
This is a fix for #4021: exceptions raised in middleware were sent without taking into account
failed_request_status_codes
value.See the test case for an example.
Thank you for contributing to
sentry-python
! Please add tests to validate your changes, and lint your code usingtox -e linters
.Running the test suite on your PR might require maintainer approval. The AWS Lambda tests additionally require a maintainer to add a special label, and they will fail until this label is added.