Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#30752 closed New feature (fixed)

Allow using ExceptionReporter subclass in django.views.debug.technical_500_response

Reported by: Carlton Gibson Owned by: Pavel Lysak
Component: Error reporting Version: dev
Severity: Normal Keywords:
Cc: Pavel Lysak Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description (last modified by Carlton Gibson)

#29714 allows using an ExceptionReporter subclass with AdminEmailHandler.

Ideally we'd make the similar available for the 500 debug error view.

Currently the use of `ExceptionReporter` is hardcoded.

* Move this to a parameter
* Provide an example of using, e.g., functools.partial to configure a subclass when specifying handler500.

Updated for comment:5

  • Add ExceptionReporter to the documentation, explaining the relation between the exception reporter class and the filter it uses, and showing a simple override of get_traceback_data().
  • Add a DEFAULT_EXCEPTION_REPORTER setting to allow specifying this.

(At that point we could review deprecating DEFAULT_EXCEPTION_REPORTER_FILTER, as was discussed on #25167 — but maybe that's unnecessary. Thoughts?)

Change History (12)

comment:1 by Carlton Gibson, 5 years ago

Description: modified (diff)

comment:2 by Mariusz Felisiak, 5 years ago

Triage Stage: UnreviewedAccepted

comment:3 by Carlton Gibson, 5 years ago

Description: modified (diff)

comment:4 by Pavel Lysak, 5 years ago

Cc: Pavel Lysak added
Owner: set to Pavel Lysak
Status: newassigned

comment:5 by Pavel Lysak, 5 years ago


But as i see handler500 and technical_500_response are different things. The first is view which we can set in settings and which runs when DEBUG = False.
The second one runs when DEBUG = True. And it called from response_for_exception and handle_uncaught_exception. So i don't think that we can use functools.partial when providing the handler500 in the URLConf.

Am i wrong?

comment:6 by Pavel Lysak, 5 years ago

Has patch: set

comment:7 by Carlton Gibson, 5 years ago

Description: modified (diff)
Needs documentation: set

Hi Pavel, thanks for your input.

You're absolutely correct re the handler500 suggestion. That's not sufficient at all. (I'm not sure quite what was in my head thinking about this.)
I've adjusted the description appropriately.

Looking at it, there's no nice way to pass down the exception reporter class so, I agree, a setting will be the easiest way forward.

Your PR looks more or less right, but I think we need to say more in the Custom error reports section of the docs. Specifically we should probably begin stating that the reports are created by the ExceptionReporter class, and that it uses an ExceptionReporterFilter to control the filtering. Then then existing filtering examples are OK — that's the first port of call ‚ before giving an override example for ExceptionReporter...

class CustomExceptionReporter(ExceptionReporter):
    def get_traceback_data(self):
        data = super().get_traceback_data()
        # ... remove/add something here...
        return data

That should be about it. (I'm not sure there's any demand to customise anything else on ExceptionReporter.)

Thanks again, super stuff.

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

comment:8 by Pavel Lysak, 5 years ago

HI, thank you for review.

I added some more to the docs. Also considered what you mentioned in PR on github.

Also i thought about deprecating DEFAULT_EXCEPTION_REPORTER_FILTER. I like that idea because it may be confusing to have these similar settings.
I think it would not be a problem to use something like this if we only want to change filtering behavior.

class ExceptionReporter:

    filter_class = SafeExceptionReporterFilter

    def __init__(self, request, exc_type, exc_value, tb, is_email=False):
        self.filter = self.filter_class(self.request)

class CustomExceptionReporter(ExceptionReporter):

    filter_class = CustomExceptionReporterFilter

This will allow us to get rid of DEFAULT_EXCEPTION_REPORTER_FILTER

comment:9 by Pavel Lysak, 5 years ago

Needs documentation: unset

comment:10 by Carlton Gibson, 5 years ago

Triage Stage: AcceptedReady for checkin

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

Resolution: fixed
Status: assignedclosed

In 13e4abf8:

Fixed #30752 -- Allowed using ExceptionReporter subclasses in error reports.

comment:12 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In 56071826:

Refs #30752 -- Doc'd error reporting related optional request attributes.

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