twistd logging options needs to be tested

[therve]

This is the next step of #2571, after #2921. We should create a common structure for logging objects, that will allow us to test it better, and will ease future enhancements.

[therve]

(In [23116]) Branching to 'twistd-logging-3052'

[therve]

This deserves a first look, I'd say. I'm sure you're eager to do it :).

[radix]

I glanced through this branch. It looks like a major improvement.

The summary in the docstring of UnixAppLogger is incorrect; it manages more than just syslog.

The giant conditional around the entire contents of getLogObserver kind of makes me think that it should be in two different classes, one for syslog and one for the rest of the stuff. But maybe that's at odds with your general idea for the refactoring.

I would suggest making all the newly introduced ivars start with _, just so we don't get stuck with something we don't want to maintain.

It seems slightly awkward that the way to start a logger is by first asking the logger for an object and then passing that object to another method on the logger. Why not just make start() call self.getLogObserver() to get the observer?

The call to sys.stdout.flush() in AppLogger.start seems out of place. Can you explain why it's there? I realize it was in the old code, but I wonder if it's necessary or makes sense.

I'm sure you know there are a bunch of docstrings missing :)

Thanks, this looks like a step in the right direction.

[therve]

(In [23368]) Branching to 'twistd-logging-3052-2'

[therve]

I think I handled most of the previous comments. The only thing I didn't really change is the syslog management in _getLogObserver. That seems ok to me for now.

Back to review.

[exarkun]

• twisted/application/app.py
  • ApplicationRunner.loggerFactory and ApplicationRunner.logger are undocumented
  • There are a couple "disappearing API" incompatibilities:
    • ApplicationRunner.getLogObserver is no longer called, so subclasses which override it will have their definition ignored.
    • Same for ApplicationRunner.startLogging.
  • Is there a plan for allowing logging customization? There was a comment in ApplicationRunner.run in trunk about potential future direction. This branch deletes it and makes it harder to go in that direction.
• twisted/test/test_twistd.py
  • going through im_self (once or twice) is a bit crummy. Maybe the tests can actually write and make sure the bytes end up in the correct place?
  • test_getLogObserverDontOverrideSignalHandler doesn't assert anything about the signal for which the handler is requested
  • nothing tests the log rotation signal handler, just that something is installed.

[therve]

(In [23523]) * Remove usage of im_self

• Pass application to start for future customization

Refs #3052

[therve]

Replying to exarkun:

There are a couple "disappearing API" incompatibilities: * ApplicationRunner.getLogObserver is no longer called, so subclasses which override it will have their definition ignored. * Same for ApplicationRunner.startLogging.

I added a compatibility layer.

Is there a plan for allowing logging customization? There was a comment in ApplicationRunner.run in trunk about potential future direction. This branch deletes it and makes it harder to go in that direction.

My plan was to do it in AppLogger. I passed the application to the start method to ease that.

Everything else is done, I think.

[exarkun]

There some problem with the full test suite now. A lot of tests fail like this:

[ERROR]: twisted.trial.test.test_script.ForceGarbageCollection.test_unforceGc

Traceback (most recent call last):
  File "/home/exarkun/Projects/Twisted/trunk/twisted/trial/test/test_script.py", line 78, in test_unforceGc
    runner.run(self.test)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/trial/runner.py", line 772, in run
    result = self._makeResult()
  File "/home/exarkun/Projects/Twisted/trunk/twisted/trial/runner.py", line 705, in _makeResult
    self.rterrors)
  File "/home/exarkun/Projects/Twisted/trunk/twisted/trial/reporter.py", line 857, in __init__
    if colorizer.supported():
  File "/home/exarkun/Projects/Twisted/trunk/twisted/trial/reporter.py", line 746, in supported
    if not sys.stderr.isatty():
exceptions.AttributeError: StdioOnnaStick instance has no attribute 'isatty'

[therve]

Sorry, it should be better now. Maybe trial should report observers added during tests...

[exarkun]

• test_twistd has some trailing whitespace.
• Any way to use callDeprecated instead of assertWarns in test_legacyApplicationRunnerGetLogObserver?
• Is there a test for the deprecation of app.initialLog?
• maybe AppRunner.finalLog should be AppRunner.stop (to parallel AppRunner.start)?
• maybe AppRunner.initialLog should be private? Or just part of start?
• it might be cool if finalLog removed the observer that start adds. This would make start/finalLog pairs leave the system in basically the same state as they were initially, which is always nice.
• AppRunner.initialLog inherits the problem that app.initialLog had, the str of reactor classes is icky (it used to be nicer).

[therve]

(In [23724]) Process some review comments

Refs #3052

[therve]

Replying to exarkun:

test_twistd has some trailing whitespace.

Removed

Any way to use callDeprecated instead of assertWarns in test_legacyApplicationRunnerGetLogObserver?

Not an easy one. I don't want to modify the deprecated API in this branch: we need to be able to print a warning without attaching it to a method.

Is there a test for the deprecation of app.initialLog?

There is now.

maybe AppRunner.finalLog should be AppRunner.stop (to parallel AppRunner.start)?

Done.

maybe AppRunner.initialLog should be private? Or just part of start?

I made it private. I need a separate method so that app.initialLog still works.

it might be cool if finalLog removed the observer that start adds. This would make start/finalLog pairs leave the system in basically the same state as they were initially, which is always nice.

Done.

AppRunner.initialLog inherits the problem that app.initialLog had, the str of reactor classes is icky (it used to be nicer).

Done.

[exarkun]

Looking awesome. Please merge.

[therve]

(In [23738]) Merge twistd-logging-3052-2

Author: therve Reviewers: exarkun, radix Fixes #3052

Add tests to twistd logging options, with a refactoring to extract logging logic out the ApplicationRunner.