[Twisted-Python] Style/testing for log-related changes

[Daniel Sutcliffe]

I'm also finding myself looking at doing some of this so thought it
might be worth rejuvenating this thread...

On Jun 6, 2016, at 04:21, Phil Mayers <p.mayers at imperial.ac.uk> wrote:
> > I'd like to submit a patch to convert t.conch.ssh to the new logging. The
> > main reason is that the conch code logs a *lot* of really, really, really
> > boring crap that I want to throw away because it just clutters up the logs
> > e.g.
> > https://github.com/twisted/twisted/blob/twisted-16.2.0/twisted/conch/ssh/connection.py#L454

On Mon, Jun 6, 2016 at 4:40 PM, Glyph <glyph at twistedmatrix.com> wrote:
> No need to justify it - any work to move us internally to new APIs so we can
> finally get to the business of deprecating the old ones would be great!

Looking through the code it doesn't actually seem like there's that
much work to get there, but as someone who is new to this project I
feel a bit more guidance would be useful. There seems to be little in
the way of examples of this being done, and when it has been done
there seem to be 2 approaches:

- one with a style similar to the examples in the docs:
    _log = twisted.logger.Logger()
    self._log.emit()
    https://github.com/twisted/twisted/commit/ff7a05da

- and a more recent one that uses this style:
    twisted.logger._loggerFor(self).emit()
    https://github.com/twisted/twisted/commit/c575f1d

Appreciating consistancy is important and not wanting to waste time
doing the former when the latter is now thought of as a better idea

> > Moving it to the new logging would, at very least, let me trivially write an
> > observer which throws away these by module.
>
> No need to write one!  This is an explicit use-case for new logging: see
> https://twistedmatrix.com/documents/16.2.0/api/twisted.logger.LogLevelFilterPredicate.html
> and
> https://twistedmatrix.com/documents/16.2.0/api/twisted.logger.FilteringLogObserver.html
>
> (You may also be interested in figuring out a solution to
> https://twistedmatrix.com/trac/ticket/7969 )

That looks like an interesting and worthwhile challenge, but first a
little practice porting the basics to the new logger

> Does anyone have an example ticket/commit for a conversion to the new
> logging showing the general style, and the technique used for writing tests
> for that?

This is what I came up with while trying to get twistd related
messages all emitted through new logger and thus not have [-] in
standard textual log:
  https://github.com/twisted/twisted/compare/bb0d1d67...dansut:logger-update
Probably did some really daft stuff here but comments appreciated on
my forks branch to get me working in a way which will be acceptable
for PRs in future.

> twistd itself was converted over -
> https://twistedmatrix.com/trac/ticket/8235 - but of course that's mostly
> from the consumer side rather than emitting logs.

That's where I found use of _loggerFor ...

> It shouldn't be too complex, honestly; just get rid of all manual string
> formatting, and convert any %()s format strings to {}.

That side of things is pretty clear, there are a few places where
longer strings are being logged that might be questionable, and a
bunch of places where utility function log.callWithLogger is used that
I'm not sure how to handle.

> The testing support is the same as for the old logging system (add a global
> observer, remove it in an addCleanup, assert about the things it caught)
> because it's still just key-value pairs, they're just better-defined now.

Any chance of a pointer to a good clear example to emulate?

Also still trying to get my head around whether there should be one
Trac ticket to cover all logger porting effort, or individual tickets
for each porting effort.

Sorry for being such a newb, all help and pointers appreciated.
Cheers
/dan
-- 
Daniel Sutcliffe <dansut at gmail.com>