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 Samir Shah)

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 and 500 responses are logged to django.request - not any other responses in the 5xx range.
  • 500 responses are only logged if they are the result of an uncaught exception. The logging happens in django.core.handlers.base.handle_uncaught_exception. If a view manually sets a 500 response somewhere, this isn't logged.
  • The same was true of 404 responses (they were only logged if an uncaught Http404 was raised), until the change made in ticket:26504 inadvertently altered this behaviour. After that change, all 404s 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 to django.security but not to django.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 to django.request or change the documentation to list what specific responses are logged.

Change History (12)

comment:1 by Samir Shah, 8 years ago

Description: modified (diff)

comment:2 by Sergio Oliveira, 8 years ago

Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

The logging behavior is clearly inconsistent with the docs. The documented behavior is preferable over the current one so that's actually a bug.

comment:3 by Sergio Oliveira, 8 years ago

Cc: seocam@… added
Owner: changed from nobody to Sergio Oliveira
Status: newassigned

comment:5 by Carl Meyer, 8 years ago

Triage Stage: AcceptedReady for checkin

comment:6 by Tim Graham, 8 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

Patch needs a rebase and I left some additional comments for improvement.

comment:7 by Samir Shah, 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 was done by Sergio.

Last edited 7 years ago by Samir Shah (previous) (diff)

comment:8 by Samir Shah, 7 years ago

Patch needs improvement: unset

comment:9 by Tim Martin, 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 Samir Shah, 7 years ago

Patch needs improvement: unset

Tim - thanks for the review. I've updated the PR to address your comments.

comment:11 by Tim Martin, 7 years ago

Triage Stage: AcceptedReady for checkin

comment:12 by Tim Graham <timograham@…>, 7 years ago

Resolution: fixed
Status: assignedclosed

In 10b44e45:

Fixed #26688 -- Fixed HTTP request logging inconsistencies.

  • Added logging of 500 responses for instantiated responses.
  • Added logging of all 4xx and 5xx responses.
Note: See TracTickets for help on using tickets.
Back to Top