Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Use per-module loggers instead of the root logger.
- Loading branch information
Showing
13 changed files
with
75 additions
and
60 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,6 +49,7 @@ def connect(): | |
""" | ||
|
||
import datetime | ||
import logging | ||
import logging.handlers | ||
import re | ||
import sys | ||
|
Oops, something went wrong.
e391fd2
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 really don't like this change - why did we commit this? What is the goal?
e391fd2
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 goal is to let you configure verbosity (and maybe other things, but mostly verbosity) on a per-module basis. The tornado.httpclient module in particular is so noisy at the debug level that I end up doing all my application logging at info or higher so I never have to turn on --logging=debug. This change seems like a step in that direction (although I haven't yet tried to use any non-global logging configuration yet myself). The need to explicitly define a logger using redundant information is lame, but seems to be how the logging module was (over-)designed to be used. Did you have something else in mind for module-level logging configuration?
e391fd2
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.
So there's another problem with doing things this way. The root-level logging functions (logging.info(), etc) will call logging.basicConfig() for you if no logging configuration has been done. If you're using module-level loggers this doesn't happen, so if you're using tornado without tornado.options (which sets up logging in parse_command_line) the only log output you'll get is "no handlers could be found for logger "tornado.web"". The recommended fix (file:///opt/local/share/doc/python26-doc/library/logging.html#configuring-logging-for-a-library) is apparently to add a null handler to each logger, which is A) incredibly verbose since you're expected to write your own NullHandler class and B) not really what you want since it means that the library's log output will go to /dev/null in the absence of explicit logging configuration. This is absurd. It looks like the only way to get reasonable out-of-the-box behavior is to just use the root logger. It looks like handler.addFilter can be used to get the kind of per-module verbosity settings that I wanted without multiple loggers, so I think I'll switch everything back to the root logger and try adding a --vmodule-like filter.
e391fd2
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.
That's unfortunate: this made debugging and using Tornado significantly easier for me. Global logging configuration (e.g. 'logging.getLogger("tornado.web").setLevel(logging.ERROR)' seems to work fine.
e391fd2
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 trouble is that since IOLoop effectively swallows and logs all exceptions, I feel it's important to ensure that tornado's logging is visible by default out of the box. I don't see any good way to do that with per-module loggers without adding handlers at import time (which sqlalchemy does and it's annoying to work around).
Are you attaching different handlers to the different loggers, or are you just using them to set different levels? If it's the latter, I think a logging.Filter that inspects the record's module attribute is a good alternative (and will work automatically even for libraries that take the easy way out and just use the root logger).
e391fd2
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.
That make sense to me. In my application I'm configuring root handlers to log to stderr + scribe globally in my run script. I missed the "no loggers configured" message and the fact that module-logged messages aren't visible by default.
This isn't the first time the logging module has been a pain point for me. I started working on [what I believe is] a better logging library called "logit" (http://github.com/bickfordb/logit) if anyone reading this is interested in contributing or has any ideas.