-
Notifications
You must be signed in to change notification settings - Fork 654
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
Improve logging #327
Improve logging #327
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.
Looks good to me! What do you think about removing the SMTP handler and move the rotating file handler to a 'default' or 'example' config file?
Sounds good, if no log file path is given, then we can prompt for the default. Alternatively, I think fileConfig can be called multiple times to configure iteratively and we could store as many as the user wants |
fc16b06
to
89ca312
Compare
# Conflicts: # flaskbb/utils/requirements.py
# Conflicts: # flaskbb/utils/populate.py
# Conflicts: # flaskbb/configs/default.py # flaskbb/management/models.py # flaskbb/utils/helpers.py
formatter = logging.Formatter( | ||
'%(asctime)s %(levelname)s: %(message)s ' | ||
'[in %(pathname)s:%(lineno)d]') | ||
app.config.get('LOG_FORMAT') or default_log_format |
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.
The dict.get(...) or ...
pattern prevents things like the key exists but it's set to None or ''
@@ -615,6 +616,14 @@ def generate_config(development, output, force): | |||
click.style("Mail Admin Email", fg="magenta"), | |||
default=default_conf.get("mail_admin_address")) | |||
|
|||
click.secho("Optional filepath to load a logging configuration file from. " |
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 I add the other new log config options here as well?
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 that would be an overkill for our guided setup - just the file is fine imo
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.
Alright, I'll update the docs to call these out then.
Adds a bunch of loggers and an option to load a log configuration file (either through envvars, a .cfg file, or the python config file). This puts more power in the hands of users to control their logging rather than just rotating files or SMTP (which are preserved).
What do we log now?