Opened 8 years ago
Closed 7 years ago
#26688 closed Bug (fixed)
Inconsistent logging of 5xx and 4xx requests to django.request
Reported by: | Samir Shah | Owned by: | Sergio Oliveira |
---|---|---|---|
Component: | HTTP handling | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | seocam@… | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
The documentation for django.request
says:
Log messages related to the handling of requests. 5XX responses are raised as ERROR messages; 4XX responses are raised as WARNING messages.
The actual behaviour is not quite consistent with this:
- Only
4xx
and500
responses are logged todjango.request
- not any other responses in the5xx
range.
500
responses are only logged if they are the result of an uncaught exception. The logging happens indjango.core.handlers.base.handle_uncaught_exception
. If a view manually sets a500
response somewhere, this isn't logged.
- The same was true of
404
responses (they were only logged if an uncaughtHttp404
was raised), until the change made in ticket:26504 inadvertently altered this behaviour. After that change, all404
s are logged regardless of how they are generated.
- Other
4xx
responses meanwhile are only logged as the result of an exception (PermissionDenied
etc.), not if the status code is set manually.
400
responses are logged todjango.security
but not todjango.request
.
I would be happy to submit a patch that addresses this but would like some guidance on what the best solution is. My initial thoughts are:
- I think Django should log all
5xx
responses, regardless of how they were generated. This would mean refactoring the logic that does this logging.
- Either log all
4xx
responses todjango.request
or change the documentation to list what specific responses are logged.
Change History (12)
comment:1 by , 8 years ago
Description: | modified (diff) |
---|
comment:2 by , 8 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Type: | Uncategorized → Bug |
comment:3 by , 8 years ago
Cc: | added |
---|---|
Owner: | changed from | to
Status: | new → assigned |
comment:5 by , 8 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:6 by , 8 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
Patch needs a rebase and I left some additional comments for improvement.
comment:7 by , 7 years ago
I've taken the liberty of updating the original PR to work with changes in the code base since it was created - https://github.com/django/django/pull/8752. Most of the original work way done by Sergio.
comment:8 by , 7 years ago
Patch needs improvement: | unset |
---|
comment:9 by , 7 years ago
Patch needs improvement: | set |
---|
This looks good to me. I had a minor documentation comment on the patch review.
comment:10 by , 7 years ago
Patch needs improvement: | unset |
---|
Tim - thanks for the review. I've updated the PR to address your comments.
comment:11 by , 7 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
The logging behavior is clearly inconsistent with the docs. The documented behavior is preferable over the current one so that's actually a bug.