891488 - Mozlog should have a more flexible interface for instantiating loggers

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Chris Manchester (limited bugmail, email directly)]

Mozlog has a convenient interface for instantiating loggers, via the getLogger method. Logging to stdout or a file is supported, but unfortunately if a different message handler is desired, a default console handler will still be added. Mozlog should be flexible enough to allow users to specify their own handlers without unnecessary inconvenience.

Comment 1

[Chris Manchester (limited bugmail, email directly)]

Attachment: Provide interface for specifying a handler to mozlog's getLogger method.

This provides a more flexible interface for users of mozlog that I think minimizes deviation from the current behavior. Parameters are still ignored if a logger already exists, which may be surprising behavior. Perhaps the parameters should result in handlers being added to the existing logger?

This patch applies on top of the patch from bug 888010.

Comment 2

[Andrew Halberstadt [:ahal]]

Comment on attachment 773722 [details] [diff] [review]
Provide interface for specifying a handler to mozlog's getLogger method.

Review of attachment 773722 [details] [diff] [review]:
-----------------------------------------------------------------

This solves the JSONFormatter problem and maintains backwards compatibility, which is nice. But like you mentioned, this is also quite confusing and non-intuitive (for which I take full responsibility). I don't think there are many (any?) production harnesses using mozlog. So I would be inclined to just not worry about backwards compatibility and we can update consumers as we run into problems. Yes, this could be a pain, but it'll be much less of a pain now than it will in the future. Just my 2 cents.

Comment 3

[Andrew Halberstadt [:ahal]]

I think either we could remove mozlog.getLogger completely (and just use the logging implementation) or perhaps just remove the logfile parameter as it can be passed in has a handler.

> Parameters are still ignored if a logger already exists, which may be surprising behavior. > Perhaps the parameters should result in handlers being added to the existing logger?

If we do the latter, then yes, we should fix this.

Comment 4

[Chris Manchester (limited bugmail, email directly)]

Attachment: Provide interface for specifying a handler to mozlog's getLogger method.

This gets rid of the logfile parameter in favor of a handler parameter. The advantage I see over using the built in getLogger is that existing and future users of mozlog can call getLogger and get a handler that logs to stdout with the MozFormatter, which may be a common use case and sensible default.

For those that want to use the MozFormatter with a different handler, and specifying it themselves, what about getting rid of the underscore prefix?

Comment 5

[Chris Manchester (limited bugmail, email directly)]

This patch erroneously results in a file named "testFile" being created without getting cleaned up. I'll fix this in the next iteration of the patch.

Comment 6

[Andrew Halberstadt [:ahal]]

Comment on attachment 774797 [details] [diff] [review]
Provide interface for specifying a handler to mozlog's getLogger method.

Review of attachment 774797 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah I think I tend to agree with you that we should set MozFormatter by default. And yes let's also remove the _prefix now. Thanks!

::: mozlog/mozlog/logger.py
@@ +157,1 @@
>      logger.addHandler(handler)

I wonder if we should remove any previously existing handlers here first or not. I can envision a consumer calling 'getLogger(name, handler=foo)' from multiple places and then getting confused when all their messages get output twice. This would probably be a good topic for next week's mozbase meeting.

Comment 7

[Chris Manchester (limited bugmail, email directly)]

Attachment: Provide interface for specifying a handler to mozlog's getLogger method, remove logfile parameter.

This implements the recommendation from today's mozbase meeting: if getLogger is called with the name of a logger that already exists, throw an exception, as this is likely an error in the logic of the program. In the case that multiple handlers are actually desired, users of mozlog can call 'addHandler' on their particular logger instance with their particular handler.

This is a breaking change for users of the 'logfile' parameter - I've made the appropriate changes to mozcrash. The only other user of the 'logfile' parameter discoverable by searching for 'mozlog' on mxr is peptest, so a similar change will need to be made there as well.

Comment 8

[Andrew Halberstadt [:ahal]]

Comment on attachment 776821 [details] [diff] [review]
Provide interface for specifying a handler to mozlog's getLogger method, remove logfile parameter.

Review of attachment 776821 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great, thanks! Just a few nits to fix up.

::: mozlog/mozlog/logger.py
@@ +133,5 @@
>      Returns the logger with the specified name.
>      If the logger doesn't exist, it is created.
> +    If no handler is specified, the logger will log to standard output
> +    with a default formatter. Otherwise no default handler will be
> +    installed.

nit: this comment is worded confusingly. Try "If handler is specified, add it to the logger. Otherwise a default handler that logs to standard output will be used."

@@ +140,5 @@
> +    :param handler: If specified, the logger will defer handling to the
> +                    specified handler. If a logger is found with the specified
> +                    name and a handler is specified, an exception is thrown. To
> +                    add an additional handler to an extant logger, call the
> +                    logger instance's addHandler method.

nit: again took me a few tries to parse this comment, redundant use of the word 'specified'. Also 'extant' should be 'existent'

@@ +147,5 @@
>  
>      if name in Logger.manager.loggerDict:
> +        if (handler):
> +            raise ValueError('The handler parameter requires that a ' + \
> +                             'logger by this name does not already exist')

nit: don't think ValueError is the proper thing to raise here. I'd probably just make a custom exception and raise that.

::: mozlog/tests/test_logger.py
@@ +46,5 @@
> +                                          handler=ListHandler())
> +        except ValueError as e:
> +            erred = True
> +            self.assertEqual(e.message, "The handler parameter requires " + \
> +                             "that a logger by this name does not already exist")

You can use assertRaises here: http://docs.python.org/2/library/unittest.html#unittest.TestCase.assertRaises

Comment 9

[Chris Manchester (limited bugmail, email directly)]

Attachment: Provide interface for specifying a handler to mozlog's getLogger method, remove logfile parameter.

Addressed nits and carrying forward r+. As discussed on irc, keeping ValueError.

Comment 10

[Chris Manchester (limited bugmail, email directly)]

https://github.com/mozilla/mozbase/commit/e3b3b142c80578fc1aaa426c369a54234bd16ac4