launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

Julian Edwards has proposed merging lp:~julian-edwards/txlongpoll/fix-logging into lp:txlongpoll.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~julian-edwards/txlongpoll/fix-logging/+merge/77566

Fix the logging so it only goes to a single log file.  This entailed a lot of work because I needed to pull an unreleased patch from upstream Twisted trunk that adds a --logger option to twistd, the tarball for that is now in LP's download-cache.

--logger takes an importable function that returns a logfile observer - it's a horrendous change IMO but we have to live with it.  To make it cope with a configurable log name, I call it early from the make-service code to set up a log observer which will cache it for when the code is called again from the Twisted application module.

Making this change has highlighted a problem in oops_twisted, because its fallback observer (which is the log observer we create here) that is there to write OOPS data to the log file ends up duplicating everything that is logged.  Therefore I've removed the fallback log observer until that problem is fixed in oops_twisted.
-- 
https://code.launchpad.net/~julian-edwards/txlongpoll/fix-logging/+merge/77566
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/txlongpoll/fix-logging into lp:txlongpoll.

=== modified file 'setup.py'
--- setup.py	2011-09-29 14:55:35 +0000
+++ setup.py	2011-09-29 16:31:24 +0000
@@ -18,7 +18,7 @@
         'oops_datedir_repo',
         'oops_twisted >= 0.0.3',
         'setproctitle',
-        'Twisted',
+        'Twisted == 11.0.0_and_r32397',
         'txamqp',
         'zope.interface',
         ],

=== modified file 'twisted/plugins/longpoll.py'
--- twisted/plugins/longpoll.py	2011-09-29 13:57:01 +0000
+++ twisted/plugins/longpoll.py	2011-09-29 16:31:24 +0000
@@ -2,6 +2,7 @@
 # the GNU Affero General Public License version 3 (see the file LICENSE).
 
 import signal
+import sys
 
 from zope.interface import implements
 
@@ -13,7 +14,12 @@
     )
 import setproctitle
 from twisted.application.internet import TCPServer, TCPClient
-from twisted.application.service import IServiceMaker, MultiService
+from twisted.application.service import (
+    Application,
+    IServiceMaker,
+    MultiService,
+    )
+from twisted.internet import reactor
 from twisted.plugin import IPlugin
 from twisted.python import log, usage
 from twisted.python.log import (
@@ -22,6 +28,7 @@
     )
 from twisted.python.logfile import LogFile
 from twisted.web.server import Site
+from twisted.plugins.longpolllogger import rotatable_log
 
 from txlongpoll.client import AMQFactory
 from txlongpoll.frontend import QueueManager, FrontEndAjax
@@ -29,17 +36,21 @@
 
 def getRotatableLogFileObserver(filename):
     """Setup a L{LogFile} for the given application."""
-    logfile = LogFile.fromFullPath(
-        filename, rotateLength=None, defaultMode=0644)
-
-    def signal_handler(*args):
-        logfile.reopen()
-
-    signal.signal(signal.SIGUSR1, signal_handler)
+    if filename != '-':
+        logfile = LogFile.fromFullPath(
+            filename, rotateLength=None, defaultMode=0644)
+
+        def signal_handler(*args):
+            reactor.callFromThread(logfile.reopen)
+
+        signal.signal(signal.SIGUSR1, signal_handler)
+    else:
+        logfile = sys.stdout
+
     return FileLogObserver(logfile)
 
 
-def setUpOopsHandler(options):
+def setUpOopsHandler(options, logfile):
     """Add OOPS handling based on the passed command line options."""
     config = oops_config()
 
@@ -49,10 +60,10 @@
         repo = DateDirRepo(options["oops-dir"], options["oops-prefix"])
         config.publishers.append(defer_publisher(repo.publish))
 
-    # Add the log file observers. The second observer is to put OOPSes
-    # in the log too.
-    logfile = getRotatableLogFileObserver(options["logfile"])
-    observer = OOPSObserver(config, logfile.emit)
+    # This just makes everything get logged twice.  We need oops_twisted
+    # to only log the OOPSes to the fallback observer.
+    # observer = OOPSObserver(config, logfile)
+    observer = OOPSObserver(config)
     addObserver(observer.emit)
 
 
@@ -94,7 +105,15 @@
         setproctitle.setproctitle(
             "txlongpoll: accepting connections on %s" % 
                 options["frontendport"])
-        setUpOopsHandler(options)
+
+        # rotatable_log() depends on twistd being called with "--logger="
+        # Here, we're priming it with the right log file name before
+        # the twistd apoplication code imports it and calls it.
+        logfile = rotatable_log(options["logfile"])
+
+        setUpOopsHandler(options, logfile)
+        application = Application(self.tapname)
+        application.addComponent(logfile)
 
         broker_port = options["brokerport"]
         broker_host = options["brokerhost"]

=== added file 'twisted/plugins/longpolllogger.py'
--- twisted/plugins/longpolllogger.py	1970-01-01 00:00:00 +0000
+++ twisted/plugins/longpolllogger.py	2011-09-29 16:31:24 +0000
@@ -0,0 +1,34 @@
+# Copyright 2005-2011 Canonical Ltd.  This software is licensed under
+# the GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""
+A rotatable log file for use with twisted's --logger option. Call twisted
+with --logger=logger.rotatable_log
+"""
+
+import signal
+
+from twisted.internet import reactor
+from twisted.python.log import FileLogObserver
+from twisted.python.logfile import LogFile
+
+log_observer = None
+
+def rotatable_log(filename="txlongpoll.log"):
+    global log_observer
+
+    if log_observer is not None:
+        return log_observer
+
+    logfile = LogFile.fromFullPath(
+        filename, rotateLength=None, defaultMode=0644)
+
+    def signal_handler(*args):
+        reactor.callFromThread(logfile.reopen)
+
+    if not signal.getsignal(signal.SIGUSR1):
+        # Override if signal is set to None or SIG_DFL (0)
+        signal.signal(signal.SIGUSR1, signal_handler)
+
+    log_observer = FileLogObserver(logfile).emit
+    return log_observer