Closed Bug 1186599 Opened 9 years ago Closed 9 years ago

[mozlog] Re-implement LogLevelFilter as a property on BaseFormatter

Categories

(Testing :: Mozbase, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: ahal, Assigned: ahal)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

The current implementation of log levels [1] is problematic for a couple reasons. First, it obscures the handler class by wrapping it in an 'inner' variable. So often, you'll need code like this:

if hasattr(handler, 'inner'):
    formatter = handler.inner.formatter
else:
    formatter = handler.formatter

Second, it requires a fair bit of overhead to setup if not using commandline.setup_logging.

Removing LogLevelFilter and replacing it with a simple property that the BaseFormatter can handle will make it easier to fix bug 1027665.

[1] https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozlog/mozlog/handlers/base.py#47
The code example should read formatter.inner instead of handler.inner, but the point still stands.
Bug 1186599 - Re-implement LogLevelFilter as a property on BaseFormatter, r=jgraham
Attachment #8637479 - Flags: review?(james)
Can you explain the motivation here more clearly, with an example of what you're trying to do that the current system makes difficult. The theroy behind the current design is that forcing all this different functionality into a single class is poor seperation, and implementing the filter as a handler allows better composition because you can set up message handling pipelines (notice that there's almost no difference between a formatter and a handler other than a formatter typically doesn't pass the messages on any further). Of course the design might have rough edges in practice.

Note also that there is a message broadcast system in the mozlog handlers that you can use to send a message to any handler with a filter attached to do something like change the log level. I would certainly take a patch to add that message handler to the log level receiver.
I've had to work around the 'inner' thing several times in the past, but currently I'm trying to scan all the installed formatters on a logger to see if any of them are suitable for a terminal.

e.g:
for handler in logger.handlers:
    if handler.formatter.__class__ in TERMINAL_FORMATTERS:
        self.log_manager.terminal_handler = handler
        self.log_manager.terminal_formatter = handler.formatter

Unfortunately this is a necessary step for the mach integration. It doesn't work under the current system because handler.formatter.__class__ may or may not be LogLevelFilter. Worse, if several filters like that are applied, there can be an arbitrary amount of 'inner' variables we need to step through.

I think the separation of concerns is a worthy goal, I just don't like that it's replacing the formatter class we care about. FWIW, I tried re-implementing this as some kind of decorator, but I couldn't get it to work.
Here's another possible solution that address the separation of concerns comment. Unforunately it's not possible to do:
instance.__call__ = wrap(instance.__call__)

in cPython, so I had to move the __call__ logic into a _handle function. The downside to this is that you can no longer use arbitrary functions as handlers, it needs to be a derivative of reader.LogHandler.
I'm not convinced the solutions here are much better than the current situation. I tried a more complex change, but that also had some downsides. Since it doesn't seem like this is a hard blocker for anything I don't think it's worth spending more cycles on for the moment.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
Attachment #8637479 - Flags: review?(james) → review-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: