Hi stub, Only a couple of minute comments. Looks good :) Gavin. > === modified file 'lib/canonical/launchpad/webapp/adapter.py' > --- lib/canonical/launchpad/webapp/adapter.py 2009-07-28 22:35:01 +0000 > +++ lib/canonical/launchpad/webapp/adapter.py 2009-07-29 11:37:51 +0000 > @@ -134,7 +134,7 @@ > def get_request_statements(): > """Get the list of executed statements in the request. > > - The list is composed of (starttime, endtime, statement) tuples. > + The list is composed of (starttime, endtime, db_id, statement) tuples. > Times are given in milliseconds since the start of the request. > """ > return getattr(_local, 'request_statements', []) > @@ -165,7 +165,11 @@ > # convert times to integer millisecond values > starttime = int((starttime - request_starttime) * 1000) > endtime = int((endtime - request_starttime) * 1000) > - _local.request_statements.append((starttime, endtime, statement)) > + # A string containing no whitespace that lets us identify which Store > + # is being used. > + database_identifier = connection_wrapper._database.name > + _local.request_statements.append( > + (starttime, endtime, database_identifier, statement)) > > # store the last executed statement as an attribute on the current > # thread > @@ -274,6 +278,8 @@ > # opinion on what uri is. > # pylint: disable-msg=W0231 > self._uri = uri > + # A unique name for this database connection. > + self.name = uri.database > > def raw_connect(self): > # Prevent database connections from the main thread if > @@ -348,6 +354,9 @@ > > class LaunchpadSessionDatabase(Postgres): > > + # A unique name for this database connection. > + name = 'session' > + > def raw_connect(self): > self._dsn = 'dbname=%s user=%s' % (config.launchpad_session.dbname, > config.launchpad_session.dbuser) > > === modified file 'lib/canonical/launchpad/webapp/errorlog.py' > --- lib/canonical/launchpad/webapp/errorlog.py 2009-07-23 05:02:47 +0000 > +++ lib/canonical/launchpad/webapp/errorlog.py 2009-07-29 11:37:51 +0000 > @@ -167,9 +167,9 @@ > fp.write('%s=%s\n' % (urllib.quote(key, safe_chars), > urllib.quote(value, safe_chars))) > fp.write('\n') > - for (start, end, statement) in self.db_statements: > - fp.write('%05d-%05d %s\n' % (start, end, > - _normalise_whitespace(statement))) > + for (start, end, database_id, statement) in self.db_statements: > + fp.write('%05d-%05d@%s %s\n' % ( > + start, end, database_id, _normalise_whitespace(statement))) > fp.write('\n') > fp.write(self.tb_text) > > @@ -206,9 +206,15 @@ > line = line.strip() > if line == '': > break > - startend, statement = line.split(' ', 1) > - start, end = startend.split('-') > - statements.append((int(start), int(end), statement)) > + preamble, statement = line.split(' ', 1) > + start, suffix = preamble.split('-', 1) > + try: > + end, db_id = suffix.split('@', 1) > + db_id = intern(database_id) # This string is repeated lots. > + except ValueError: > + end, db_id = suffix, None > + statements.append( > + (int(start), int(end), db_id, statement)) This could be reduced to the shorter: start, end, db_id, statement = re.match( r'^(\d+)-(\d+)(?:@([\w-]+))? (.*)', '123-456').groups() But then people keep saying that regular expressions are another problem. > > # The rest is traceback. > tb_text = ''.join(lines) > @@ -471,9 +477,10 @@ > > duration = get_request_duration() > > - statements = sorted((start, end, _safestr(statement)) > - for (start, end, statement) > - in get_request_statements()) > + statements = sorted( > + (start, end, _safestr(database_id), _safestr(statement)) > + for (start, end, database_id, statement) > + in get_request_statements()) > > oopsid, filename = self.newOopsId(now) > > > === modified file 'lib/canonical/launchpad/webapp/tests/test_errorlog.py' > --- lib/canonical/launchpad/webapp/tests/test_errorlog.py 2009-07-17 00:26:05 +0000 > +++ lib/canonical/launchpad/webapp/tests/test_errorlog.py 2009-07-29 11:05:49 +0000 > @@ -57,8 +57,8 @@ > 'pageid', 'traceback-text', 'username', 'url', 42, > [('name1', 'value1'), ('name2', 'value2'), > ('name1', 'value3')], > - [(1, 5, 'SELECT 1'), > - (5, 10, 'SELECT 2')]) > + [(1, 5, 'store_a', 'SELECT 1'), > + (5, 10, 'store_b', 'SELECT 2')]) > self.assertEqual(entry.id, 'id') > self.assertEqual(entry.type, 'exc-type') > self.assertEqual(entry.value, 'exc-value') > @@ -74,8 +74,12 @@ > self.assertEqual(entry.req_vars[1], ('name2', 'value2')) > self.assertEqual(entry.req_vars[2], ('name1', 'value3')) > self.assertEqual(len(entry.db_statements), 2) > - self.assertEqual(entry.db_statements[0], (1, 5, 'SELECT 1')) > - self.assertEqual(entry.db_statements[1], (5, 10, 'SELECT 2')) > + self.assertEqual( > + entry.db_statements[0], > + (1, 5, 'store_a', 'SELECT 1')) > + self.assertEqual( > + entry.db_statements[1], > + (5, 10, 'store_b', 'SELECT 2')) > > def test_write(self): > """Test ErrorReport.write()""" > @@ -88,8 +92,8 @@ > [('HTTP_USER_AGENT', 'Mozilla/5.0'), > ('HTTP_REFERER', 'http://localhost:9000/'), > ('name=foo', 'hello\nworld')], > - [(1, 5, 'SELECT 1'), > - (5, 10, 'SELECT\n2')]) > + [(1, 5, 'store_a', 'SELECT 1'), > + (5, 10,'store_b', 'SELECT\n2')]) > fp = StringIO.StringIO() > entry.write(fp) > self.assertEqual(fp.getvalue(), dedent("""\ > @@ -108,13 +112,60 @@ > HTTP_REFERER=http://localhost:9000/ > name%%3Dfoo=hello%%0Aworld > > - 00001-00005 SELECT 1 > - 00005-00010 SELECT 2 > + 00001-00005@store_a SELECT 1 > + 00005-00010@store_b SELECT 2 > > traceback-text""" % (versioninfo.branch_nick, versioninfo.revno))) > > def test_read(self): > - """Test ErrorReport.read()""" > + """Test ErrorReport.read().""" > + fp = StringIO.StringIO(dedent("""\ > + Oops-Id: OOPS-A0001 > + Exception-Type: NotFound > + Exception-Value: error message > + Date: 2005-04-01T00:00:00+00:00 > + Page-Id: IFoo:+foo-template > + User: Sample User > + URL: http://localhost:9000/foo > + Duration: 42 > + > + HTTP_USER_AGENT=Mozilla/5.0 > + HTTP_REFERER=http://localhost:9000/ > + name%3Dfoo=hello%0Aworld > + > + 00001-00005@store_a SELECT 1 > + 00005-00010@store_b SELECT 2 > + > + traceback-text""")) > + entry = ErrorReport.read(fp) > + self.assertEqual(entry.id, 'OOPS-A0001') > + self.assertEqual(entry.type, 'NotFound') > + self.assertEqual(entry.value, 'error message') > + # XXX jamesh 2005-11-30: > + # this should probably convert back to a datetime This XXX looks like it's no longer relevant. > + self.assertEqual(entry.time, datetime.datetime(2005, 4, 1)) > + self.assertEqual(entry.pageid, 'IFoo:+foo-template') > + self.assertEqual(entry.tb_text, 'traceback-text') > + self.assertEqual(entry.username, 'Sample User') > + self.assertEqual(entry.url, 'http://localhost:9000/foo') > + self.assertEqual(entry.duration, 42) > + self.assertEqual(len(entry.req_vars), 3) > + self.assertEqual(entry.req_vars[0], ('HTTP_USER_AGENT', > + 'Mozilla/5.0')) > + self.assertEqual(entry.req_vars[1], ('HTTP_REFERER', > + 'http://localhost:9000/')) > + self.assertEqual(entry.req_vars[2], ('name=foo', 'hello\nworld')) > + self.assertEqual(len(entry.db_statements), 2) > + self.assertEqual( > + entry.db_statements[0], > + (1, 5, 'store_a', 'SELECT 1')) > + self.assertEqual( > + entry.db_statements[1], > + (5, 10, 'store_b', 'SELECT 2')) > + > + > + def test_read_no_store_id(self): > + """Test ErrorReport.read() for old logs with no store_id.""" > fp = StringIO.StringIO(dedent("""\ > Oops-Id: OOPS-A0001 > Exception-Type: NotFound > @@ -152,8 +203,8 @@ > 'http://localhost:9000/')) > self.assertEqual(entry.req_vars[2], ('name=foo', 'hello\nworld')) > self.assertEqual(len(entry.db_statements), 2) > - self.assertEqual(entry.db_statements[0], (1, 5, 'SELECT 1')) > - self.assertEqual(entry.db_statements[1], (5, 10, 'SELECT 2')) > + self.assertEqual(entry.db_statements[0], (1, 5, None, 'SELECT 1')) > + self.assertEqual(entry.db_statements[1], (5, 10, None, 'SELECT 2')) > > > class TestErrorReportingUtility(unittest.TestCase): > @@ -422,7 +473,7 @@ > # Test ErrorReportingUtility.raising() with an XML-RPC request. > request = TestRequest() > directlyProvides(request, IXMLRPCRequest) > - request.getPositionalArguments = lambda : (1,2) > + request.getPositionalArguments = lambda: (1, 2) > utility = ErrorReportingUtility() > now = datetime.datetime(2006, 04, 01, 00, 30, 00, tzinfo=UTC) > try: > @@ -469,7 +520,6 @@ > utility.raising(sys.exc_info(), request, now=now) > self.assertEqual(request.oopsid, None) > > - > def test_raising_for_script(self): > """Test ErrorReportingUtility.raising with a ScriptRequest.""" > utility = ErrorReportingUtility() > @@ -753,7 +803,7 @@ > # logged will have OOPS reports generated for them. > error_message = self.factory.getUniqueString() > try: > - 1/0 > + ignored = 1/0 > except ZeroDivisionError: > self.logger.exception(error_message) > oops_report = self.error_utility.getLastOopsReport()