Customizing notification method for internal server errors
Jesse Young
django.core.handlers.base.handle_uncaught_exception calls mail_admins
for each internal server error.
So if a very high-traffic view has an internal server error, duplicate
emails will be sent at a very high rate. This isn't usually
desirable...
We worked around this by changing mail_admins directly in our copy of
Django to store the last time an email was sent on the filesystem and
avoid sending multiple emails in too short of a time interval. But it
seems like there should be a better way to customize this without
having to modify Django.
I suppose one could define a mail_admins replacement in our app and
assign it to django.core.mail.mail_admins. But that's rather hacky...
Also, BaseHandler says that handle_uncaught_exception can be
overridden, but its derived classes ModPythonHandler and WSGIHandler
are referenced directly in Django, so I don't really see how to
subclass it without changing the Django code.
I was thinking it would be useful to add a setting like
EXCEPTION_NOTIFIER = 'path.to.custom.notifier' , where the default
would look something like this:
def mail_exception_to_admins(request, exc_info):
from django.core.mail import mail_admins
subject = 'Error (%s IP): %s' %
((request.META.get('REMOTE_ADDR') in settings.INTERNAL_IPS and
'internal' or 'EXTERNAL'), request.path)
try:
request_repr = repr(request)
except:
request_repr = "Request repr() unavailable"
message = "%s\n\n%s" % (self._get_traceback(exc_info),
request_repr)
mail_admins(subject, message, fail_silently=True)
Thoughts?
Jeremy Dunck
...
> I was thinking it would be useful to add a setting like
> EXCEPTION_NOTIFIER = 'path.to.custom.notifier' , where the default
> would look something like this:
>
> def mail_exception_to_admins(request, exc_info):
> from django.core.mail import mail_admins
> subject = 'Error (%s IP): %s' %
> ((request.META.get('REMOTE_ADDR') in settings.INTERNAL_IPS and
> 'internal' or 'EXTERNAL'), request.path)
> try:
> request_repr = repr(request)
> except:
> request_repr = "Request repr() unavailable"
> message = "%s\n\n%s" % (self._get_traceback(exc_info),
> request_repr)
> mail_admins(subject, message, fail_silently=True)
I like the idea, but not the addition of yet another setting. Maybe
an on_exception signal could be added which can do its own thing and
return False if the default action shouldn't be taken?
Jesse Young
effectively do the same thing by adding a signal handler and making
settings.ADMINS the empty list so that mail_admins effectively becomes
a no-op.
Even so, it looks like mail_admins will open a SMTP connection to send
an email message even if the message has no recipients. So it seems
wrong to force every non-debug exception to go through that code path.
(If your servers are melting down, opening hundreds of SMTP
connections probably won't help the situation.)
-Jesse
Jeremy Dunck
>
> I see there is a got_request_exception signal already... so one could
> effectively do the same thing by adding a signal handler and making
> settings.ADMINS the empty list so that mail_admins effectively becomes
> a no-op.
>
> Even so, it looks like mail_admins will open a SMTP connection to send
> an email message even if the message has no recipients. So it seems
> wrong to force every non-debug exception to go through that code path.
> (If your servers are melting down, opening hundreds of SMTP
> connections probably won't help the situation.)
In handlers.base.get_response, theres:
"
receivers = dispatcher.send(signal=signals.got_request_exception,
request=request)
"
Return value cases:
In [2]: l = [] #no receivers
In [3]: l2 = [None] #only old receivers
In [4]: l3 = [False] #new receiver asking not to default handle
In [5]: l4 = [None, False] #old and new receiver
In [6]: l5 = [True, False] #mix of new receivers, one asking not to
default handle
The odd edge case is where a different signal handlers return both
True and False (where existing code would return None). What to do
there is a design choice; I'd vote for False wins.
So I'd add:
"
if receivers:
default_handler = not bool([ret_value for ret_value in receivers
if ret_value == False])
else:
default_handler = True
if default_handler:
...existing handling code here...
callback, param_dict = resolver.resolve500()
return callback(request, **param_dict)
"
Malcolm Tredinnick
On Wed, 2008-10-15 at 13:58 -0700, Jesse Young wrote:
> The built-in behavior for
> django.core.handlers.base.handle_uncaught_exception calls mail_admins
> for each internal server error.
>
> So if a very high-traffic view has an internal server error, duplicate
> emails will be sent at a very high rate. This isn't usually
> desirable...
So the reason handle_uncaught_exception is in its own method is, as
noted in the docstring for that method, so that you can have exactly
this type of control. I have one client, for example, who want to use
Python's logging module instead of sending email and no doubt lots of
other customisations are possible.
The idea is that you create your own subclass of which
django/core/handlers/* class you want (wsgi or modpython, typically) and
override the handle_uncaught_exception method. After all, you specify
the handler class to use when setting things up to talk to the
webserver, so it's just as easy to use your own subclass as one of
Django's built in versions.
This is done via subclassing rather than a setting to avoid settings
proliferation for something where there's no real common case of
customisations and so the ultimate configuration is provided: you write
Python code to do whatever you want. It's also meant to impose a slight
barrier to entry in the sense that you have to be able to write Python
code in order to customise this, which is reasonable for something as
core-functionality oriented as handling uncaught exceptions (and no
problems for people on django-dev, obviously).
Regards,
Malcolm
Jesse Young
Thanks for explaining the rationale behind this. It'd certainly be
possible to do what we want by overriding handle_uncaught_exception in
a ModPythonHandler subclass.
This method has a higher barrier to entry than I'd like, though. For
one, this kind of customization requires updating the apache
configuration file to point to the new handler. It's annoying to have
to update Apache config files in synchronization with the Python app
code.
Also, I suspect that a large number of Django developers use
"manage.py runserver" (which is hardcoded to use WSGIHandler) for
development and testing and use ModPythonHandler on the release
servers. To minimize unexpected bugs in the release environment that
never show up in the test environment, it's best not to introduce
additional differences between these environments. A custom
implementation of handle_uncaught_exception would be hard for a
developer using "manage.py runserver" to test.
My hunch is that even though there isn't a common way that users would
want to customize uncaught exceptions, it's probably common that users
would want to customize it in some way or another. So subclassing the
handler seems like a rather high barrier to entry.
-Jesse
On Oct 15, 6:15 pm, Malcolm Tredinnick <malc...@pointy-stick.com>
wrote:
oggie rob
object?
-rob
Jesse Young
place in Django that uses settings.ADMINS, I could do something like:
class AdminsObject(list):
def __iter__(self):
// do some custom notification
// manually write the friendly 500 error page to the output
stream somehow
raise Exception // can't raise StopIteration here or mail_admins
will connect to the SMTP server
ADMINS = AdminsObject()
That would be pretty hilarious. Not particularly straightforward or
maintainable though.
-Jesse
Ivan Sagalaev
> The built-in behavior for
> django.core.handlers.base.handle_uncaught_exception calls mail_admins
> for each internal server error.
>
> So if a very high-traffic view has an internal server error, duplicate
> emails will be sent at a very high rate. This isn't usually
> desirable...
We're working it around by not having settings.ADMINS set in production
environment. Instead we log unhandled exceptions by listening to
`signals.got_request_exception`. I don't even consider it as a
workaround actually but as a normal solution :-)
oggie rob
> Is your suggestion that, since mail_admins happens to be the only
> place in Django that uses settings.ADMINS, I could do something like:
>
> class AdminsObject(list):
> def __iter__(self):
> // do some custom notification
> // manually write the friendly 500 error page to the output
> stream somehow
> raise Exception // can't raise StopIteration here or mail_admins
> will connect to the SMTP server
> ADMINS = AdminsObject()
list.iter in mind. Something like:
class AdminsObject(list):
def __iter__(self):
if ok_to_send(): # enough time since last mail or whatever
return super(AdminsObject, self).__iter__()
return [].__iter__()
ADMINS = AdminsObject([
('Your Name', 'your_...@domain.com'),
])
I think your exception would propagate up to the
handle_uncaught_exception and the user would not see the "friendly
error message".
However, after seeing your comment regarding the exception I looked at
EmailMessage and was surprised to see that it attempts to send even
with an empty ADMINS list. It should return if not self.to and not
self.bcc, I think.
> That would be pretty hilarious. Not particularly straightforward or
> maintainable though.
it gives you more context.
-rob
Malcolm Tredinnick
On Wed, 2008-10-15 at 18:59 -0700, Jesse Young wrote:
> Hi Malcolm,
>
> Thanks for explaining the rationale behind this. It'd certainly be
> possible to do what we want by overriding handle_uncaught_exception in
> a ModPythonHandler subclass.
>
> This method has a higher barrier to entry than I'd like, though. For
> one, this kind of customization requires updating the apache
> configuration file to point to the new handler. It's annoying to have
> to update Apache config files in synchronization with the Python app
> code.
Well, not really. You have to set the Apache configuration once to point
to the right thing. Just like you do now.
> Also, I suspect that a large number of Django developers use
> "manage.py runserver" (which is hardcoded to use WSGIHandler) for
> development and testing and use ModPythonHandler on the release
> servers. To minimize unexpected bugs in the release environment that
> never show up in the test environment, it's best not to introduce
> additional differences between these environments. A custom
> implementation of handle_uncaught_exception would be hard for a
> developer using "manage.py runserver" to test.
Patches welcome to fix that if you see it as a real problem.
> My hunch is that even though there isn't a common way that users would
> want to customize uncaught exceptions, it's probably common that users
> would want to customize it in some way or another. So subclassing the
> handler seems like a rather high barrier to entry.
No it's not. Since there's no common way to customize, sub-classing is
the perfect way to do it, since people can then do whatever they want.
How is asking somebody to write a subclass in Python a high barrier to
entry? We're assuming knowledge of the language, but that's all.
I don't see any of your objections here a real issues except the need
for being able to simulate things in the development server.
Regards,
Malcolm
Jesse Young
> the perfect way to do it, since people can then do whatever they want.
> How is asking somebody to write a subclass in Python a high barrier to
> entry? We're assuming knowledge of the language, but that's all.
barrier to entry. I'm saying that the handler in particular is hard to
subclass. And I agree that subclassing it would be 'the perfect way',
if only it didn't require a configuration change (yes, only once, but
that is once more than required by basically any other change to a
Django app) and didn't introduce differences between the test and
release environments.
I mean, really, the notification method for errors should have nothing
to do with the type of web server interface you have (WSGI /
mod_python). It's just an artifact of the way the code is currently
factored.
> Patches welcome to fix that if you see it as a real problem.
send email if ADMINS is empty.
On Oct 16, 2:41 pm, Malcolm Tredinnick <malc...@pointy-stick.com>
wrote:
Malcolm Tredinnick
On Thu, 2008-10-16 at 15:15 -0700, Jesse Young wrote:
> > No it's not. Since there's no common way to customize, sub-classing is
> > the perfect way to do it, since people can then do whatever they want.
> > How is asking somebody to write a subclass in Python a high barrier to
> > entry? We're assuming knowledge of the language, but that's all.
>
> Well, I'm not saying that writing a subclass in Python is a high
> barrier to entry. I'm saying that the handler in particular is hard to
> subclass. And I agree that subclassing it would be 'the perfect way',
> if only it didn't require a configuration change (yes, only once, but
> that is once more than required by basically any other change to a
> Django app)
No, this isn't correct. You already have to specify which handler to
run. You still have to do that exactly once in each case.
> and didn't introduce differences between the test and
> release environments.
There's already differences between the test and release environments if
you mean runserver versus anything running with a real webserver.
Expecting complete parity there is a false goal and you absolutely must
do real production-preparedness tests against the real webserver you're
going to use in production. That's hardly a surprise. A patch to allow
something like --with-handler='my.handler.class' (making up an API on
the spot; that needs to be thought about) would be a good idea for
completeness, though, you're right.
> I mean, really, the notification method for errors should have nothing
> to do with the type of web server interface you have (WSGI /
> mod_python).
The method is called handle_uncaught_exception, not
notify_about_uncaught_exception. Right now it only happens to do
notification, but because it's extensible it could do stuff that is
handler dependent.
Malcolm
Jesse Young
me.
I just wanted to clarify a couple of points.
> No, this isn't correct. You already have to specify which handler to
> run. You still have to do that exactly once in each case.
already deployed your app using the default handler, and later want to
override handle_uncaught_exception.
> There's already differences between the test and release environments if
> you mean runserver versus anything running with a real webserver.
> Expecting complete parity there is a false goal
release environments more different than they already are.
-Jesse