Skip to content

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

Merged
merged 3 commits into from
Jun 5, 2021
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 64 additions & 33 deletions src/classes/logger.py
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it! Nice

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

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
Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be using info.LOG_LEVEL_CONSOLE and info.LOG_LEVEL_FILE here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 info.ERROR, but then we'd have a lot more errors in the logs. 😆 )



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)
Copy link
Member

Choose a reason for hiding this comment

The 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... 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

@ferdnyc ferdnyc Jun 5, 2021

Choose a reason for hiding this comment

The 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 .editorconfig file at the root of the repo, so is any editor that respects those files.) That means that the file is protected against concatenation damage and other issues, so you could do this:

$ 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 (vim and gedit) didn't; they eat the final newline, so in those editors the last line of the file indeed appeared to be line 135.

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. 🤣 :

image