Opened 7 years ago

Closed 6 years ago

#3052 enhancement closed fixed (fixed)

twistd logging options needs to be tested

Reported by: therve Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: dreid Branch: branches/twistd-logging-3052-2
(diff, github, buildbot, log)
Author: therve Launchpad Bug:

Description

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.

Change History (18)

comment:1 Changed 7 years ago by dreid

  • Cc dreid added

comment:2 Changed 7 years ago by therve

  • author set to therve
  • Branch set to branches/twistd-logging-3052

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

comment:3 Changed 7 years ago by therve

  • Keywords review added
  • Owner changed from therve to dreid

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

comment:4 Changed 7 years ago by radix

  • Keywords review removed
  • Owner changed from dreid to therve

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.

comment:5 Changed 7 years ago by therve

  • Branch changed from branches/twistd-logging-3052 to branches/twistd-logging-3052-2

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

comment:6 Changed 7 years ago by 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.

comment:7 Changed 7 years ago by therve

  • Keywords review added
  • Owner therve deleted

comment:8 follow-up: Changed 6 years ago by exarkun

  • Keywords review removed
  • Owner set to therve
  • 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.

comment:9 Changed 6 years ago by therve

(In [23523]) * Remove usage of im_self

  • Pass application to start for future customization

Refs #3052

comment:10 in reply to: ↑ 8 Changed 6 years ago by therve

  • Keywords review added
  • Owner therve deleted

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.

comment:11 Changed 6 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

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'

comment:12 Changed 6 years ago by therve

  • Keywords review added
  • Owner therve deleted

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

comment:13 follow-up: Changed 6 years ago by exarkun

  • Keywords review removed
  • Owner set to therve
  • 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).

comment:14 Changed 6 years ago by therve

(In [23724]) Process some review comments

Refs #3052

comment:15 in reply to: ↑ 13 Changed 6 years ago by therve

  • Keywords review added
  • Owner therve deleted

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.

comment:16 Changed 6 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

Looking awesome. Please merge.

comment:17 Changed 6 years ago by therve

  • Resolution set to fixed
  • Status changed from new to closed

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

comment:18 Changed 4 years ago by <automation>

  • Owner therve deleted
Note: See TracTickets for help on using tickets.