I think I've done what you asked, I chose a name that made sense to me - please have a look at the incremental diff and let me know what you think: === modified file 'lib/canonical/launchpad/webapp/errorlog.py' --- lib/canonical/launchpad/webapp/errorlog.py 2010-06-27 01:17:40 +0000 +++ lib/canonical/launchpad/webapp/errorlog.py 2010-06-30 00:25:56 +0000 @@ -38,7 +38,7 @@ soft_timeout_expired) from canonical.launchpad.webapp.interfaces import ( IErrorReport, IErrorReportEvent, IErrorReportRequest) -from canonical.launchpad.webapp.lognamer import LogNamer +from lp.services.log.uniquefileallocator import UniqueFileAllocator from canonical.launchpad.webapp.opstats import OpStats UTC = pytz.utc @@ -253,8 +253,8 @@ if section_name is None: section_name = self._default_config_section self.copy_to_zlog = config[section_name].copy_to_zlog - # Start a new LogNamer to activate the new configuration. - self.log_namer = LogNamer( + # Start a new UniqueFileAllocator to activate the new configuration. + self.log_namer = UniqueFileAllocator( output_root=config[section_name].error_dir, log_type="OOPS", log_subtype=config[section_name].oops_prefix, === modified file 'lib/canonical/launchpad/webapp/tests/test_errorlog.py' --- lib/canonical/launchpad/webapp/tests/test_errorlog.py 2010-06-27 01:17:40 +0000 +++ lib/canonical/launchpad/webapp/tests/test_errorlog.py 2010-06-30 00:25:56 +0000 @@ -37,8 +37,8 @@ _is_sensitive) from canonical.launchpad.webapp.interfaces import ( NoReferrerError, TranslationUnavailable) -from canonical.launchpad.webapp.lognamer import LogNamer from lazr.restful.declarations import webservice_error +from lp.services.log.uniquefileallocator import UniqueFileAllocator from lp.services.osutils import remove_tree from lp.testing import TestCase @@ -230,9 +230,9 @@ reset_logging() super(TestErrorReportingUtility, self).tearDown() - def test_creates_LogNamer(self): + def test_sets_log_namer_to_a_UniqueFileAllocator(self): utility = ErrorReportingUtility() - self.assertIsInstance(utility.log_namer, LogNamer) + self.assertIsInstance(utility.log_namer, UniqueFileAllocator) def test_configure(self): """Test ErrorReportingUtility.setConfigSection().""" === added directory 'lib/lp/services/log' === added file 'lib/lp/services/log/__init__.py' --- lib/lp/services/log/__init__.py 1970-01-01 00:00:00 +0000 +++ lib/lp/services/log/__init__.py 2010-06-30 00:25:56 +0000 @@ -0,0 +1,11 @@ +# Copyright 2010 Canonical Ltd. This software is licensed under the +# GNU Affero General Public License version 3 (see the file LICENSE). + +"""lp.services.log provides log-like facilities. + +Consider putting infrastructure that is used for getting logs or diagnostics +out of Launchpad and onto some external store in here. + +Because this part of lp.services, packages in this namespace can only use +general LAZR or library functionality. +""" === added directory 'lib/lp/services/log/tests' === added file 'lib/lp/services/log/tests/__init__.py' --- lib/lp/services/log/tests/__init__.py 1970-01-01 00:00:00 +0000 +++ lib/lp/services/log/tests/__init__.py 2010-06-30 00:25:56 +0000 @@ -0,0 +1,4 @@ +# Copyright 2010 Canonical Ltd. This software is licensed under the +# GNU Affero General Public License version 3 (see the file LICENSE). + +"""Tests for lp.services.log.""" === renamed file 'lib/canonical/launchpad/webapp/tests/test_lognamer.py' => 'lib/lp/services/log/tests/test_uniquefileallocator.py' --- lib/canonical/launchpad/webapp/tests/test_lognamer.py 2010-06-27 01:17:40 +0000 +++ lib/lp/services/log/tests/test_uniquefileallocator.py 2010-06-30 00:25:56 +0000 @@ -15,22 +15,22 @@ import pytz import testtools -from canonical.launchpad.webapp.lognamer import LogNamer +from lp.services.log.uniquefileallocator import UniqueFileAllocator UTC = pytz.timezone('UTC') -class TestLogNamer(testtools.TestCase): +class TestUniqueFileAllocator(testtools.TestCase): def setUp(self): - super(TestLogNamer, self).setUp() + super(TestUniqueFileAllocator, self).setUp() tempdir = tempfile.mkdtemp() self._tempdir = tempdir self.addCleanup(shutil.rmtree, tempdir, ignore_errors=True) def test_setToken(self): - namer = LogNamer("/any-old/path/", 'OOPS', 'T') + namer = UniqueFileAllocator("/any-old/path/", 'OOPS', 'T') self.assertEqual('T', namer._log_infix()) # Some scripts will append a string token to the prefix. @@ -42,8 +42,8 @@ namer.setToken('1') self.assertEqual('T1', namer._log_infix()) - def assertLogNamed(self, namer, now, expected_id, expected_last_id, - expected_suffix, expected_lastdir): + def assertUniqueFileAllocator(self, namer, now, expected_id, + expected_last_id, expected_suffix, expected_lastdir): logid, filename = namer.newId(now) self.assertEqual(logid, expected_id) self.assertEqual(filename, @@ -57,29 +57,31 @@ # reduce races with threads that are slow to use what they asked for, # when combined with configuration changes causing disk scans. That # would also permit using a completely stubbed out file system, - # reducing IO in tests that use LogNamer (such as all the pagetests in - # Launchpad. - namer = LogNamer(self._tempdir, 'OOPS', 'T') + # reducing IO in tests that use UniqueFileAllocator (such as all the + # pagetests in Launchpad. At that point an interface to obtain a + # factory of UniqueFileAllocator's would be useful to parameterise the + # entire test suite. + namer = UniqueFileAllocator(self._tempdir, 'OOPS', 'T') # first name of the day - self.assertLogNamed(namer, + self.assertUniqueFileAllocator(namer, datetime.datetime(2006, 04, 01, 00, 30, 00, tzinfo=UTC), 'OOPS-91T1', 1, '2006-04-01/01800.T1', '2006-04-01') # second name of the day - self.assertLogNamed(namer, + self.assertUniqueFileAllocator(namer, datetime.datetime(2006, 04, 01, 12, 00, 00, tzinfo=UTC), 'OOPS-91T2', 2, '2006-04-01/43200.T2', '2006-04-01') # first name of the following day sets a new dir and the id starts # over. - self.assertLogNamed(namer, + self.assertUniqueFileAllocator(namer, datetime.datetime(2006, 04, 02, 00, 30, 00, tzinfo=UTC), 'OOPS-92T1', 1, '2006-04-02/01800.T1', '2006-04-02') # Setting a token inserts the token into the filename. - namer.setToken('XXX') + namer.setToken('YYY') logid, filename = namer.newId( datetime.datetime(2006, 04, 02, 00, 30, 00, tzinfo=UTC)) - self.assertEqual(logid, 'OOPS-92TXXX2') + self.assertEqual(logid, 'OOPS-92TYYY2') # Setting a type controls the log id: namer.setToken('') @@ -94,14 +96,14 @@ def test_changeErrorDir(self): """Test changing the log output dur.""" - namer = LogNamer(self._tempdir, 'OOPS', 'T') + namer = UniqueFileAllocator(self._tempdir, 'OOPS', 'T') # First an id in the original error directory. - self.assertLogNamed(namer, + self.assertUniqueFileAllocator(namer, datetime.datetime(2006, 04, 01, 00, 30, 00, tzinfo=UTC), 'OOPS-91T1', 1, '2006-04-01/01800.T1', '2006-04-01') - # LogNamer uses the _output_root attribute to get the current output + # UniqueFileAllocator uses the _output_root attribute to get the current output # directory. new_output_dir = tempfile.mkdtemp() self.addCleanup(shutil.rmtree, new_output_dir, ignore_errors=True) @@ -119,7 +121,7 @@ os.path.join(new_output_dir, '2006-04-01')) def test_findHighestSerial(self): - namer = LogNamer(self._tempdir, "OOPS", "T") + namer = UniqueFileAllocator(self._tempdir, "OOPS", "T") # Creates the dir using now as the timestamp. output_dir = namer.output_dir() # write some files, in non-serial order. === renamed file 'lib/canonical/launchpad/webapp/lognamer.py' => 'lib/lp/services/log/uniquefileallocator.py' --- lib/canonical/launchpad/webapp/lognamer.py 2010-06-27 06:38:33 +0000 +++ lib/lp/services/log/uniquefileallocator.py 2010-06-30 00:25:56 +0000 @@ -4,7 +4,7 @@ """Create uniquely named log files on disk.""" -__all__ = ['LogNamer'] +__all__ = ['UniqueFileAllocator'] __metaclass__ = type @@ -18,21 +18,21 @@ import pytz UTC = pytz.utc -# the section of the Log ID before the instance identifier is the +# the section of the ID before the instance identifier is the # days since the epoch, which is defined as the start of 2006. epoch = datetime.datetime(2006, 01, 01, 00, 00, 00, tzinfo=UTC) -class LogNamer: +class UniqueFileAllocator: """Assign unique file names to logs being written from an app/script. - LogNamer causes logs written from one process to be uniquely named. It is not - safe for use in multiple processes with the same output root - each process - must have a unique output root. + UniqueFileAllocator causes logs written from one process to be uniquely + named. It is not safe for use in multiple processes with the same output + root - each process must have a unique output root. """ def __init__(self, output_root, log_type, log_subtype): - """Create a LogNamer. + """Create a UniqueFileAllocator. :param output_root: The root directory that logs should be placed in. :param log_type: A string to use as a prefix in the ID assigned to new @@ -96,7 +96,11 @@ def getFilename(self, log_serial, time): """Get the filename for a given log serial and time.""" log_subtype = self._log_infix() - output_dir = self.output_dir(time) #XXX locks and scans, bad. + # TODO: Calling output_dir causes a global lock to be taken and a + # directory scan, which is bad for performance. It would be better + # to have a split out 'directory name for time' function which the + # 'want to use this directory now' function can call. + output_dir = self.output_dir(time) second_in_day = time.hour * 3600 + time.minute * 60 + time.second return os.path.join( output_dir, '%05d.%s%s' % (second_in_day, log_subtype, log_serial)) @@ -157,10 +161,12 @@ except OSError, e: if e.errno != errno.EEXIST: raise - # XXX: Note that only one process can do this safely: its not + # TODO: Note that only one process can do this safely: its not # cross-process safe, and also not entirely threadsafe: another # thread that has a new log and hasn't written it could then - # use that serial number. + # use that serial number. We should either make it really safe, + # or remove the contention entirely and log uniquely per thread + # of execution. self._last_serial = self._findHighestSerial(result) finally: self._lock.release()