feat(django): Add failed_request_status_codes - #7140
Conversation
ericapisani
left a comment
ericapisani
left a comment
There was a problem hiding this comment.
Thanks for opening this PR @mgaligniana , we really appreciate you taking the time to do so!
Overall the changes are looking great. However, I don't think we can exclude the 5xx codes from the failed_request_status_codes (left a more detailed comment below) and we'll need to address that before we can merge.
Don't hesitate to reach out with any questions!
|
|
||
|
|
||
| def _capture_exception( | ||
| exc_info: "Any", |
There was a problem hiding this comment.
We can tighten this typing a bit by copying what we do within the event_from_exception method that's being invoked within this function:
| exc_info: "Any", | |
| exc_info: "Union[BaseException, ExcInfo]", |
This requires updating the import from sentry_sdk._types at the top to import ExcInfo:
from sentry_sdk._types import (
Event,
EventProcessor,
ExcInfo,
Hint,
NotImplementedType,
)
| @pytest.mark.parametrize( | ||
| "integration_kwargs", | ||
| ( | ||
| {}, | ||
| {"failed_request_status_codes": set()}, | ||
| {"failed_request_status_codes": {404}}, | ||
| ), | ||
| ) | ||
| def test_failed_request_status_codes_unhandled_exception( | ||
| sentry_init, client, capture_events, integration_kwargs | ||
| ): | ||
| """ | ||
| Exceptions Django gives up on are always reported, exactly once, no matter how | ||
| failed_request_status_codes is set. | ||
| """ | ||
| sentry_init(integrations=[DjangoIntegration(**integration_kwargs)]) | ||
| events = capture_events() | ||
|
|
||
| _, status, _ = unpack_werkzeug_response(client.get(reverse("view_exc"))) | ||
| assert status.lower() == "500 internal server error" | ||
|
|
||
| (event,) = events | ||
| (exception,) = event["exception"]["values"] | ||
| assert exception["type"] == "ZeroDivisionError" | ||
| assert exception["mechanism"]["type"] == "django" | ||
| assert exception["mechanism"]["handled"] is False |
There was a problem hiding this comment.
Although I don't imagine there are many cases where someone would want to do this, if a user provides an empty set for failed_request_status_codes, or a non-empty set that doesn't include 5xx response codes, they will expect that no events are sent for these exceptions.
This will mean we need some additional changes to _got_request_exception to account for this, using a similar check to what you've added in _patch_response_for_exception.
Once that's done, we can look to remove this test case in favour of another parameterized case to the test you've added above (test_failed_request_status_codes), ideally one that confirms the behaviour by making a request against the view_exc endpoint to trigger a 500 error and ensure that no events are created.
2 participants
Description
Hi! In this PR I've added
failed_request_status_codeto the Django integrationSince English isn't my first language, I tried to keep the comments as simple and clear as possible, so they're easy to understand even for beginners like me who don't know the full Sentry product. Feel free to make any changes or suggestions!
Issues
failed_request_status_codesfor more web frameworks #3134