New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Post Django errors to Github #3322
Conversation
cfgov/alerts/post_django_errors.py
Outdated
@@ -0,0 +1,14 @@ | |||
import logging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the naming of the post_django_errors.py
file might want to be more consistent with other logging handler module names since it's just a Python logging handler, not Django-specific.
327af08
to
18404b8
Compare
cdc421c
to
7746d59
Compare
cfgov/alerts/logging_handlers.py
Outdated
|
||
def emit(self, record): | ||
GithubAlert({}).post( | ||
title=record.message[:30], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Truncating to 30 characters isn't ideal but I'm not sure how else to do this for now. Curious to see it in action before optimizing this.
@willbarton Ready for review again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worry about introducing a dependence on the GitHub API as part of the logging flow. See additional higher-level thoughts at GHE#1162.
cfgov/alerts/logging_handlers.py
Outdated
|
||
class CFGovErrorHandler(logging.Handler): | ||
def __init__(self): | ||
logging.Handler.__init__(self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this __init__
necessary, if it all it does is call the base class?
""" Test that calling CFGOVErrorHandler.emit | ||
makes a GithubAlert post with the right parameters | ||
""" | ||
message = ('Internal Server Error: /tést-page/' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to define this as a unicode string, e.g. u'/tést-page/'
vs '/tést-page/'
? Might be nice to have a from __future__ import unicode_literals
here.
@@ -41,6 +41,10 @@ | |||
'level': 'DEBUG', | |||
'class': 'logging.StreamHandler', | |||
}, | |||
'db': { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if you want to test this config, but one way of doing it might be to use the built-in django.test.utils.patch_logger
as demonstrated here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Withdrawing my hold on this after internal discussion.
7746d59
to
33bb7e3
Compare
I'm merging this @willbarton. We can iterate! |
ERROR