-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add loki log handler #173
base: develop
Are you sure you want to change the base?
Add loki log handler #173
Conversation
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 its ready to go.
There's one question, about having the LOKI url in ENV that needs to be answered, though.
But also believe this is an opportunity to improve the overal logging setup just a bit more even. If there's time for that, I think this would make the system just a bit better :)
apps/mainsite/settings.py
Outdated
if DOMAIN.startswith('acc') or DOMAIN.startswith('prod'): | ||
handlers = handlers | { | ||
'badgr_debug_loki': { | ||
'level': 'DEBUG', # Log level. Required |
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.
IMHO these comments don't really add much. Instead of documenting what it is, maybe consider commenting why its the value? For "debug" rather obvious and could possibly be gone, but for timeout it would clarify why we chose 1?
Same for # Tags and # Loki URL.
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.
Indeed
apps/mainsite/settings.py
Outdated
|
||
debug_handlers = ['badgr_debug'] | ||
SERVER_NAME = os.environ['SERVER_NAME'] | ||
LOKI_URL = 'http://195.169.124.131:3100/loki/api/v1/push' |
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.
Should this not also be an os.environ?
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.
Indeed, would be nicer.
SERVER_NAME = os.environ['SERVER_NAME'] | ||
LOKI_URL = 'http://195.169.124.131:3100/loki/api/v1/push' | ||
|
||
if DOMAIN.startswith('acc') or DOMAIN.startswith('prod'): |
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 do we have a "debug" in prod? Could we add that as comment?
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.
Yes
LOKI_URL = 'http://195.169.124.131:3100/loki/api/v1/push' | ||
|
||
if DOMAIN.startswith('acc') or DOMAIN.startswith('prod'): | ||
handlers = handlers | { |
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.
Would it not make more sense to:
- first set up a few handlers objects (console_handler, badgr_events_handler etc)
- then set up loggers using one or more of these handlers
- then use that list to set up the actual LOGGING for this env (prod, dev acc etc) using simple conditionals (prod, acc etc.
That would make it even easier to and cleaner than this PR already makes it, IMHO?
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.
Will look into it
Adding a loki log handler to setting py to automatically forward Badge.Debug logs to central loki server.