Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#30858 closed Cleanup/optimization (fixed)

Ambiguous phrasing in the Error Reporting documentation

Reported by: ForgottenLords Owned by: Carlton Gibson
Component: Documentation Version: dev
Severity: Normal Keywords: Error Reporting, Documentation
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description (last modified by ForgottenLords)

In the Error Reporting documentation, the following phrasing is used (Emphasis Mine):

When DEBUG is False, Django will email the users listed in the ADMINS setting whenever your code raises an unhandled exception and results in an internal server error (HTTP status code 500)

Myself and several others in my development team read this as:
Whenever your code raises an unhandled exception resulting in an internal server error

when in actuality it should read more like the following to better reflect the correct behavior of Django:
Whenever your code raises an unhandled exception, or your view returns a response with a status code 500

The nuance is perhaps small, but we had been operating under the assumption that views returning a response with status_code=500 would be 'silent' and not generate emails as they are internal server errors, but have already been correctly and safely handled by our code yet we still want to send the client a 500 error to inform them that the response could not be completed as expected. We assumed based on that line that emails would ONLY be sent as a result of unhandled exceptions.

It may be worth expanding on the error handling to allow users to return 'safe' 500 responses that will not be logged, and in fact Django does check for the _has_been_logged property already before logging the response, which we can set in our code already so perhaps just exposing that to the documentation in a little note would be useful to people who want to send 500 responses without sending emails might be sufficient.

Change History (7)

comment:1 by ForgottenLords, 5 years ago

Description: modified (diff)

comment:2 by Carlton Gibson, 5 years ago

I think this is so edge-case to be marginal at best. I've suggested a small docs rewording but would be happy to say wontfix.

(Will Accept if we decide to take the PR.)

Last edited 5 years ago by Carlton Gibson (previous) (diff)

comment:3 by Carlton Gibson, 5 years ago

Owner: changed from nobody to Carlton Gibson
Status: newassigned

comment:4 by Carlton Gibson, 5 years ago

Triage Stage: UnreviewedAccepted

After a quick chat with Mariusz, looks like we'll take the PR. Thanks for the report!

It may be worth expanding on the error handling to allow users to return 'safe' 500 responses that will not be logged, and in fact Django does check for the _has_been_logged property already before logging the response, which we can set in our code already so perhaps just exposing that to the documentation in a little note would be useful to people who want to send 500 responses without sending emails might be sufficient.

I don't think that adding this complication is worth the gain. Users already find logging complicated enough. Those in this particular case can look at log_response(), as you have done.

comment:5 by Carlton Gibson <carlton@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In 94eae4e5:

Fixed #30858 -- Clarified that AdminEmailHandler processes all 5xx responses.

comment:6 by Carlton Gibson <carlton.gibson@…>, 5 years ago

In 71489a52:

[3.0.x] Fixed #30858 -- Clarified that AdminEmailHandler processes all 5xx responses.

Backport of 94eae4e5633fbf21f5dcae6e0472ce6a51dd3411 from master

comment:7 by Carlton Gibson <carlton.gibson@…>, 5 years ago

In 9763ed7a:

[2.2.x] Fixed #30858 -- Clarified that AdminEmailHandler processes all 5xx responses.

Backport of 94eae4e5633fbf21f5dcae6e0472ce6a51dd3411 from master

Note: See TracTickets for help on using tickets.
Back to Top