-
Notifications
You must be signed in to change notification settings - Fork 572
Logging overhaul, to solve issues caused by stdout/stderr redirection #4175
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,65 +40,96 @@ | |
|
||
class StreamToLogger(object): | ||
"""Custom class to log all stdout and stderr streams (from libopenshot / and other libraries)""" | ||
def __init__(self, logger, log_level=logging.INFO): | ||
self.logger = logger | ||
def __init__(self, parent_stream, log_level=logging.INFO): | ||
self.parent = parent_stream or sys.__stderr__ | ||
self.logger = logging.LoggerAdapter( | ||
logging.getLogger('OpenShot.stderr'), {'source': 'stream'}) | ||
self.log_level = log_level | ||
self.linebuf = '' | ||
self.logbuf = '' | ||
|
||
def write(self, buf): | ||
for line in buf.rstrip().splitlines(): | ||
self.logger.log(self.log_level, line.rstrip()) | ||
def write(self, text): | ||
self.logbuf += str(text) or "" | ||
self.parent.write(text) | ||
|
||
def flush(self): | ||
pass | ||
if self.logbuf.rstrip(): | ||
self.logger.log(self.log_level, self.logbuf.rstrip()) | ||
self.logbuf = '' | ||
|
||
def errors(self): | ||
pass | ||
|
||
|
||
# Create logger instance | ||
log = logging.Logger('OpenShot') | ||
class StreamFilter(logging.Filter): | ||
"""Filter out lines that originated on the output""" | ||
def __init__(self, predicate): | ||
self.predicate = predicate | ||
|
||
def filter(self, record): | ||
source = getattr(record, "source", "") | ||
return self.predicate(source) | ||
|
||
|
||
# Set up log formatters | ||
template = '%(levelname)s %(module)s: %(message)s' | ||
console_formatter = logging.Formatter(template) | ||
file_formatter = logging.Formatter('%(asctime)s ' + template, datefmt='%H:%M:%S') | ||
|
||
# Add normal stderr stream handler | ||
sh = logging.StreamHandler() | ||
sh.setFormatter(console_formatter) | ||
sh.setLevel(info.LOG_LEVEL_CONSOLE) | ||
log.addHandler(sh) | ||
handlers['stream'] = sh | ||
# Configure root logger for minimal logging | ||
logging.basicConfig(level=logging.ERROR) | ||
root_log = logging.getLogger() | ||
|
||
# Set up our top-level logging context | ||
log = root_log.getChild('OpenShot') | ||
log.setLevel(info.LOG_LEVEL_FILE) | ||
# Don't pass messages on to root logger | ||
log.propagate = False | ||
|
||
# Add rotating file handler | ||
# | ||
# Create rotating file handler | ||
# | ||
fh = logging.handlers.RotatingFileHandler( | ||
os.path.join(info.USER_PATH, 'openshot-qt.log'), encoding="utf-8", maxBytes=25*1024*1024, backupCount=3) | ||
fh.setFormatter(file_formatter) | ||
os.path.join(info.USER_PATH, 'openshot-qt.log'), | ||
encoding="utf-8", | ||
maxBytes=25*1024*1024, backupCount=3) | ||
fh.setLevel(info.LOG_LEVEL_FILE) | ||
fh.setFormatter(file_formatter) | ||
|
||
log.addHandler(fh) | ||
handlers['file'] = fh | ||
|
||
# | ||
# Create typical stream handler which logs to stderr | ||
# | ||
sh = logging.StreamHandler(sys.stderr) | ||
sh.setLevel(info.LOG_LEVEL_CONSOLE) | ||
sh.setFormatter(console_formatter) | ||
|
||
# Filter out redirected output on console, to avoid duplicates | ||
# | ||
# The lambda is used to test the "source" property of the | ||
# logging record, if present. | ||
# {"source": "stream"} will be attached to any records generated | ||
# by the redirection, thanks to LoggingAdapter) | ||
filt = StreamFilter(lambda x: x != "stream") | ||
sh.addFilter(filt) | ||
|
||
log.addHandler(sh) | ||
|
||
|
||
def reroute_output(): | ||
"""Route stdout and stderr to logger (custom handler)""" | ||
if not getattr(sys, 'frozen', False): | ||
# Hang on to the original objects | ||
streams.update({ | ||
'stderr': sys.stderr, | ||
'stdout': sys.stdout, | ||
}) | ||
# Re-route output streams | ||
handlers['stdout'] = StreamToLogger(log, logging.INFO) | ||
sys.stdout = handlers['stdout'] | ||
handlers['stderr'] = StreamToLogger(log, logging.ERROR) | ||
sys.stderr = handlers['stderr'] | ||
if (getattr(sys, 'frozen', False) | ||
or sys.stdout != sys.__stdout__): | ||
return | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't recall why this frozen block was needed. Do you recall? If so, we should add a comment here to demystify it, lol There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I honestly have never had any idea. I assumed it had something to do with the AppImage, but that's pure conjecture. |
||
sys.stdout = StreamToLogger(sys.stdout, logging.INFO) | ||
sys.stderr = StreamToLogger(sys.stderr, logging.WARNING) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we be using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Those are the levels the messages are emitted AT, not the filtering levels for the loggers, so I'd say no. (In theory sys.stderr could be |
||
|
||
|
||
def set_level_file(level=logging.INFO): | ||
handlers['file'].setLevel(level) | ||
"""Adjust the minimum log level written to our logfile""" | ||
fh.setLevel(level) | ||
|
||
|
||
def set_level_console(level=logging.INFO): | ||
handlers['stream'].setLevel(level) | ||
|
||
"""Adjust the minimum log level for output to the terminal""" | ||
sh.setLevel(level) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: is there a blank line at the end of this file? Or is the PR just not showing it... 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The file ENDS with a newline (my editor makes sure of that automatically), but I'm not sure there's a blank line. I can check. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, turns out there was a surprising amount of nuance to that question. Like I said, the file ended on a newline, my editor is set to make sure of that. (Actually, thanks to the $ echo "=======" >> logger.py; tail -3 logger.py
"""Adjust the minimum log level for output to the terminal"""
sh.setLevel(level)
======= You'd THINK it would also mean that an editor would let you position your cursor on line 136, since the last line of the file (line 135) ends with a newline. But surprisingly enough, the first two editors I checked ( So, I added a second newline to the end of the file, ensuring that editors would show a blank line at the end. ...Naturally, my own editor (Github Atom) doesn't eat the final newline, so in Atom it now appears as though there are two blank lines at the end. 🤣 : |
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.
💯
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 like it! Nice
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.
This is actually a bit of an overcomplicated design (as the fact that it requires 5 lines of comment to explain one line of code makes pretty clear)... originally I thought I was going to need two filters, one on the stream handler and one on the file handler, and I wanted to write it in a way that I could use the same class for both filters.
In the end, the issue was the propagation to the root logger, and one I fixed that I didn't need a file handler filter. (Which makes sense, we want ALL log messages to go to the logfile, so what's there to filter?) So really the filter class could just have
bool(source != "stream")
hardcoded into it, which would probably be clearer.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 did indeed end up eliminating it. The cleverness just didn't justify the complexity.