| Message ID | 1443276388-10502-4-git-send-email-damien.lespiau@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
> We can use the built-in mechanism from django to send error emails when > failing to parse a mail. The error mails will have contain the full > guitly mail and the corresponding backtrace for debugging purposes. > > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> One nit. > --- > patchwork/bin/parsemail.py | 36 +++++++++++++++++++++++++++++++++++- > 1 file changed, 35 insertions(+), 1 deletion(-) > > diff --git a/patchwork/bin/parsemail.py b/patchwork/bin/parsemail.py > index e66b557..e743189 100755 > --- a/patchwork/bin/parsemail.py > +++ b/patchwork/bin/parsemail.py > @@ -28,12 +28,15 @@ import codecs > from email import message_from_file > from email.header import Header, decode_header > from email.utils import parsedate_tz, mktime_tz > +import logging > > from patchwork.parser import parse_patch > from patchwork.models import Patch, Project, Person, Comment, State, \ > get_default_initial_patch_state > import django > +from django.conf import settings > from django.contrib.auth.models import User > +from django.utils.log import AdminEmailHandler > > list_id_headers = ['List-ID', 'X-Mailing-List', 'X-list'] > > @@ -401,10 +404,41 @@ def parse_mail(mail): > > return 0 > > +extra_error_message = ''' > +== Mail > + > +%(mail)s > + > + > +== Traceback > + > +''' Can you put two spaces before the below function (PEP8 thing)? 'setup_error_handler' is small and specific enough that it could probably go into the main method, but that's not a big deal. > +# Send emails to settings.ADMINS when encountering errors > +def setup_error_handler(): > + if settings.DEBUG: > + return None > + > + mail_handler = AdminEmailHandler() > + mail_handler.setLevel(logging.ERROR) > + mail_handler.setFormatter(logging.Formatter(extra_error_message)) > + > + logger = logging.getLogger('patchwork') > + logger.addHandler(mail_handler) > + > + return logger > + > def main(args): > django.setup() > + logger = setup_error_handler() > mail = message_from_file(sys.stdin) > - return parse_mail(mail) > + try: > + return parse_mail(mail) > + except: > + if logger: > + logger.exception('Error when parsing incoming email', extra={ > + 'mail': mail.as_string(), > + }) > + raise > > if __name__ == '__main__': > sys.exit(main(sys.argv)) > -- > 2.1.0 > > _______________________________________________ > Patchwork mailing list > Patchwork@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/patchwork
On Sun, Sep 27, 2015 at 11:14:00PM +0100, Finucane, Stephen wrote: > > +extra_error_message = ''' > > +== Mail > > + > > +%(mail)s > > + > > + > > +== Traceback > > + > > +''' > > Can you put two spaces before the below function (PEP8 thing)? https://www.python.org/dev/peps/pep-0008/#a-foolish-consistency-is-the-hobgoblin-of-little-minds In this case, I think it'd be better to leave one blank line to be consistent with the rest of the file and project. > > +# Send emails to settings.ADMINS when encountering errors > > +def setup_error_handler(): > > + if settings.DEBUG: > > + return None > > + > > + mail_handler = AdminEmailHandler() > > + mail_handler.setLevel(logging.ERROR) > > + mail_handler.setFormatter(logging.Formatter(extra_error_message)) > > + > > + logger = logging.getLogger('patchwork') > > + logger.addHandler(mail_handler) > > + > > + return logger > > + > 'setup_error_handler' is small and specific enough that it could > probably go into the main method, but that's not a big deal. Not cluttering main() with details about logging that are better hidden behind a function wins. That's why functions exist after all?
> On Sun, Sep 27, 2015 at 11:14:00PM +0100, Finucane, Stephen wrote: > > > +extra_error_message = ''' > > > +== Mail > > > + > > > +%(mail)s > > > + > > > + > > > +== Traceback > > > + > > > +''' > > > > Can you put two spaces before the below function (PEP8 thing)? > > https://www.python.org/dev/peps/pep-0008/#a-foolish-consistency-is-the- > hobgoblin-of-little-minds > > In this case, I think it'd be better to leave one blank line to be > consistent with the rest of the file and project. True that. Precedence of File > Project > PEP8 > > > +# Send emails to settings.ADMINS when encountering errors > > > +def setup_error_handler(): > > > + if settings.DEBUG: > > > + return None > > > + > > > + mail_handler = AdminEmailHandler() > > > + mail_handler.setLevel(logging.ERROR) > > > + mail_handler.setFormatter(logging.Formatter(extra_error_message)) > > > + > > > + logger = logging.getLogger('patchwork') > > > + logger.addHandler(mail_handler) > > > + > > > + return logger > > > + > > 'setup_error_handler' is small and specific enough that it could > > probably go into the main method, but that's not a big deal. > > Not cluttering main() with details about logging that are better hidden > behind a function wins. That's why functions exist after all? Fair enough. Pending that one line: Acked-by: Stephen Finucane <stephen.finucane@intel.com> > -- > Damien
diff --git a/patchwork/bin/parsemail.py b/patchwork/bin/parsemail.py index e66b557..e743189 100755 --- a/patchwork/bin/parsemail.py +++ b/patchwork/bin/parsemail.py @@ -28,12 +28,15 @@ import codecs from email import message_from_file from email.header import Header, decode_header from email.utils import parsedate_tz, mktime_tz +import logging from patchwork.parser import parse_patch from patchwork.models import Patch, Project, Person, Comment, State, \ get_default_initial_patch_state import django +from django.conf import settings from django.contrib.auth.models import User +from django.utils.log import AdminEmailHandler list_id_headers = ['List-ID', 'X-Mailing-List', 'X-list'] @@ -401,10 +404,41 @@ def parse_mail(mail): return 0 +extra_error_message = ''' +== Mail + +%(mail)s + + +== Traceback + +''' +# Send emails to settings.ADMINS when encountering errors +def setup_error_handler(): + if settings.DEBUG: + return None + + mail_handler = AdminEmailHandler() + mail_handler.setLevel(logging.ERROR) + mail_handler.setFormatter(logging.Formatter(extra_error_message)) + + logger = logging.getLogger('patchwork') + logger.addHandler(mail_handler) + + return logger + def main(args): django.setup() + logger = setup_error_handler() mail = message_from_file(sys.stdin) - return parse_mail(mail) + try: + return parse_mail(mail) + except: + if logger: + logger.exception('Error when parsing incoming email', extra={ + 'mail': mail.as_string(), + }) + raise if __name__ == '__main__': sys.exit(main(sys.argv))
We can use the built-in mechanism from django to send error emails when failing to parse a mail. The error mails will have contain the full guitly mail and the corresponding backtrace for debugging purposes. Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> --- patchwork/bin/parsemail.py | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-)