Launchpad itself

Merge lp:~stub/launchpad/bug-354035 into lp:launchpad

  1. bug-354035
  2. Merge into devel
Proposed by Stuart Bishop
Status: Merged
Merged at revision: not available
Proposed branch: lp:~stub/launchpad/bug-354035
Merge into: lp:launchpad
Diff against target: None lines
To merge this branch: bzr merge lp:~stub/launchpad/bug-354035
Reviewer Review Type Date Requested Status
Diogo Matsubara (community) qa Approve
Gavin Panella (community) code Approve
Review via email: mp+9419@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Stuart Bishop (stub) wrote :

Address Bug #354035

This branch adds an identifier for the Store being used to the SQL statement logs. At the moment, this will normally be something like 'launchpad-main-master'. launchpad is the config section in launchpad-lazr.conf being used for database configuration, 'main' is the Store realm, 'master' is the Store flavour. The format of the identifier may change in the future, but it should remain a simple string.

Care has been taken to ensure old format OOPS reports are still loaded. The Store identifier is None for these OOPS reports.

I expect this will break some of the external OOPS handling tools, so requesting a review from matsubara to help identify what this will break and what to do about it.

There were some minor drive by lint and code standard cleanups.

Revision history for this message
Gavin Panella (allenap) wrote :
Download full text (10.7 KiB)

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)
> + star...

review: Approve (code)
Revision history for this message
Gavin Panella (allenap) wrote :

> start, end, db_id, statement = re.match(
> r'^(\d+)-(\d+)(?:@([\w-]+))? (.*)', '123-456').groups()

Or even:

    start, end, db_id, statement = re.match(
        r'^(\d+)-(\d+)(?:@([\w-]+))? (.*)', line).groups()

Revision history for this message
Stuart Bishop (stub) wrote :

On Wed, Jul 29, 2009 at 7:40 PM, Gavin
Panella<email address hidden> wrote:
> Review: Approve code
> Hi stub,
>
> Only a couple of minute comments. Looks good :)

 I've taken all your suggestions on board. Thanks!

--
Stuart Bishop <email address hidden>
http://www.stuartbishop.net/

Revision history for this message
Diogo Matsubara (matsubara) wrote :

Hi Stuart,

thanks for subscribing me. I checked your branch and indeed the changes break oops-tools. I'm working on a fix that should be ready by Monday. I'm marking as needs fixing and will add a bug task for oops-tools to the bug report. Once I have the fix for oops-tools ready, it'll be ok to land this branch.

Thanks.

Diogo

review: Needs Fixing (qa)
Revision history for this message
Diogo Matsubara (matsubara) wrote :

Hi Stuart,

I've updated oops-tools to cope with database_id on SQL statement logs. Feel free to land your branch. Thanks!

review: Approve (qa)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
134def get_request_statements():134def get_request_statements():
135 """Get the list of executed statements in the request.135 """Get the list of executed statements in the request.
136136
137 The list is composed of (starttime, endtime, statement) tuples.137 The list is composed of (starttime, endtime, db_id, statement) tuples.
138 Times are given in milliseconds since the start of the request.138 Times are given in milliseconds since the start of the request.
139 """139 """
140 return getattr(_local, 'request_statements', [])140 return getattr(_local, 'request_statements', [])
@@ -165,7 +165,11 @@
165 # convert times to integer millisecond values165 # convert times to integer millisecond values
166 starttime = int((starttime - request_starttime) * 1000)166 starttime = int((starttime - request_starttime) * 1000)
167 endtime = int((endtime - request_starttime) * 1000)167 endtime = int((endtime - request_starttime) * 1000)
168 _local.request_statements.append((starttime, endtime, statement))168 # A string containing no whitespace that lets us identify which Store
169 # is being used.
170 database_identifier = connection_wrapper._database.name
171 _local.request_statements.append(
172 (starttime, endtime, database_identifier, statement))
169173
170 # store the last executed statement as an attribute on the current174 # store the last executed statement as an attribute on the current
171 # thread175 # thread
@@ -274,6 +278,8 @@
274 # opinion on what uri is.278 # opinion on what uri is.
275 # pylint: disable-msg=W0231279 # pylint: disable-msg=W0231
276 self._uri = uri280 self._uri = uri
281 # A unique name for this database connection.
282 self.name = uri.database
277283
278 def raw_connect(self):284 def raw_connect(self):
279 # Prevent database connections from the main thread if285 # Prevent database connections from the main thread if
@@ -348,6 +354,9 @@
348354
349class LaunchpadSessionDatabase(Postgres):355class LaunchpadSessionDatabase(Postgres):
350356
357 # A unique name for this database connection.
358 name = 'session'
359
351 def raw_connect(self):360 def raw_connect(self):
352 self._dsn = 'dbname=%s user=%s' % (config.launchpad_session.dbname,361 self._dsn = 'dbname=%s user=%s' % (config.launchpad_session.dbname,
353 config.launchpad_session.dbuser)362 config.launchpad_session.dbuser)
354363
=== 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 @@
167 fp.write('%s=%s\n' % (urllib.quote(key, safe_chars),167 fp.write('%s=%s\n' % (urllib.quote(key, safe_chars),
168 urllib.quote(value, safe_chars)))168 urllib.quote(value, safe_chars)))
169 fp.write('\n')169 fp.write('\n')
170 for (start, end, statement) in self.db_statements:170 for (start, end, database_id, statement) in self.db_statements:
171 fp.write('%05d-%05d %s\n' % (start, end,171 fp.write('%05d-%05d@%s %s\n' % (
172 _normalise_whitespace(statement)))172 start, end, database_id, _normalise_whitespace(statement)))
173 fp.write('\n')173 fp.write('\n')
174 fp.write(self.tb_text)174 fp.write(self.tb_text)
175175
@@ -206,9 +206,15 @@
206 line = line.strip()206 line = line.strip()
207 if line == '':207 if line == '':
208 break208 break
209 startend, statement = line.split(' ', 1)209 preamble, statement = line.split(' ', 1)
210 start, end = startend.split('-')210 start, suffix = preamble.split('-', 1)
211 statements.append((int(start), int(end), statement))211 try:
212 end, db_id = suffix.split('@', 1)
213 db_id = intern(database_id) # This string is repeated lots.
214 except ValueError:
215 end, db_id = suffix, None
216 statements.append(
217 (int(start), int(end), db_id, statement))
212218
213 # The rest is traceback.219 # The rest is traceback.
214 tb_text = ''.join(lines)220 tb_text = ''.join(lines)
@@ -471,9 +477,10 @@
471477
472 duration = get_request_duration()478 duration = get_request_duration()
473479
474 statements = sorted((start, end, _safestr(statement))480 statements = sorted(
475 for (start, end, statement)481 (start, end, _safestr(database_id), _safestr(statement))
476 in get_request_statements())482 for (start, end, database_id, statement)
483 in get_request_statements())
477484
478 oopsid, filename = self.newOopsId(now)485 oopsid, filename = self.newOopsId(now)
479486
480487
=== 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 @@
57 'pageid', 'traceback-text', 'username', 'url', 42,57 'pageid', 'traceback-text', 'username', 'url', 42,
58 [('name1', 'value1'), ('name2', 'value2'),58 [('name1', 'value1'), ('name2', 'value2'),
59 ('name1', 'value3')],59 ('name1', 'value3')],
60 [(1, 5, 'SELECT 1'),60 [(1, 5, 'store_a', 'SELECT 1'),
61 (5, 10, 'SELECT 2')])61 (5, 10, 'store_b', 'SELECT 2')])
62 self.assertEqual(entry.id, 'id')62 self.assertEqual(entry.id, 'id')
63 self.assertEqual(entry.type, 'exc-type')63 self.assertEqual(entry.type, 'exc-type')
64 self.assertEqual(entry.value, 'exc-value')64 self.assertEqual(entry.value, 'exc-value')
@@ -74,8 +74,12 @@
74 self.assertEqual(entry.req_vars[1], ('name2', 'value2'))74 self.assertEqual(entry.req_vars[1], ('name2', 'value2'))
75 self.assertEqual(entry.req_vars[2], ('name1', 'value3'))75 self.assertEqual(entry.req_vars[2], ('name1', 'value3'))
76 self.assertEqual(len(entry.db_statements), 2)76 self.assertEqual(len(entry.db_statements), 2)
77 self.assertEqual(entry.db_statements[0], (1, 5, 'SELECT 1'))77 self.assertEqual(
78 self.assertEqual(entry.db_statements[1], (5, 10, 'SELECT 2'))78 entry.db_statements[0],
79 (1, 5, 'store_a', 'SELECT 1'))
80 self.assertEqual(
81 entry.db_statements[1],
82 (5, 10, 'store_b', 'SELECT 2'))
7983
80 def test_write(self):84 def test_write(self):
81 """Test ErrorReport.write()"""85 """Test ErrorReport.write()"""
@@ -88,8 +92,8 @@
88 [('HTTP_USER_AGENT', 'Mozilla/5.0'),92 [('HTTP_USER_AGENT', 'Mozilla/5.0'),
89 ('HTTP_REFERER', 'http://localhost:9000/'),93 ('HTTP_REFERER', 'http://localhost:9000/'),
90 ('name=foo', 'hello\nworld')],94 ('name=foo', 'hello\nworld')],
91 [(1, 5, 'SELECT 1'),95 [(1, 5, 'store_a', 'SELECT 1'),
92 (5, 10, 'SELECT\n2')])96 (5, 10,'store_b', 'SELECT\n2')])
93 fp = StringIO.StringIO()97 fp = StringIO.StringIO()
94 entry.write(fp)98 entry.write(fp)
95 self.assertEqual(fp.getvalue(), dedent("""\99 self.assertEqual(fp.getvalue(), dedent("""\
@@ -108,13 +112,60 @@
108 HTTP_REFERER=http://localhost:9000/112 HTTP_REFERER=http://localhost:9000/
109 name%%3Dfoo=hello%%0Aworld113 name%%3Dfoo=hello%%0Aworld
110114
111 00001-00005 SELECT 1115 00001-00005@store_a SELECT 1
112 00005-00010 SELECT 2116 00005-00010@store_b SELECT 2
113117
114 traceback-text""" % (versioninfo.branch_nick, versioninfo.revno)))118 traceback-text""" % (versioninfo.branch_nick, versioninfo.revno)))
115119
116 def test_read(self):120 def test_read(self):
117 """Test ErrorReport.read()"""121 """Test ErrorReport.read()."""
122 fp = StringIO.StringIO(dedent("""\
123 Oops-Id: OOPS-A0001
124 Exception-Type: NotFound
125 Exception-Value: error message
126 Date: 2005-04-01T00:00:00+00:00
127 Page-Id: IFoo:+foo-template
128 User: Sample User
129 URL: http://localhost:9000/foo
130 Duration: 42
131
132 HTTP_USER_AGENT=Mozilla/5.0
133 HTTP_REFERER=http://localhost:9000/
134 name%3Dfoo=hello%0Aworld
135
136 00001-00005@store_a SELECT 1
137 00005-00010@store_b SELECT 2
138
139 traceback-text"""))
140 entry = ErrorReport.read(fp)
141 self.assertEqual(entry.id, 'OOPS-A0001')
142 self.assertEqual(entry.type, 'NotFound')
143 self.assertEqual(entry.value, 'error message')
144 # XXX jamesh 2005-11-30:
145 # this should probably convert back to a datetime
146 self.assertEqual(entry.time, datetime.datetime(2005, 4, 1))
147 self.assertEqual(entry.pageid, 'IFoo:+foo-template')
148 self.assertEqual(entry.tb_text, 'traceback-text')
149 self.assertEqual(entry.username, 'Sample User')
150 self.assertEqual(entry.url, 'http://localhost:9000/foo')
151 self.assertEqual(entry.duration, 42)
152 self.assertEqual(len(entry.req_vars), 3)
153 self.assertEqual(entry.req_vars[0], ('HTTP_USER_AGENT',
154 'Mozilla/5.0'))
155 self.assertEqual(entry.req_vars[1], ('HTTP_REFERER',
156 'http://localhost:9000/'))
157 self.assertEqual(entry.req_vars[2], ('name=foo', 'hello\nworld'))
158 self.assertEqual(len(entry.db_statements), 2)
159 self.assertEqual(
160 entry.db_statements[0],
161 (1, 5, 'store_a', 'SELECT 1'))
162 self.assertEqual(
163 entry.db_statements[1],
164 (5, 10, 'store_b', 'SELECT 2'))
165
166
167 def test_read_no_store_id(self):
168 """Test ErrorReport.read() for old logs with no store_id."""
118 fp = StringIO.StringIO(dedent("""\169 fp = StringIO.StringIO(dedent("""\
119 Oops-Id: OOPS-A0001170 Oops-Id: OOPS-A0001
120 Exception-Type: NotFound171 Exception-Type: NotFound
@@ -152,8 +203,8 @@
152 'http://localhost:9000/'))203 'http://localhost:9000/'))
153 self.assertEqual(entry.req_vars[2], ('name=foo', 'hello\nworld'))204 self.assertEqual(entry.req_vars[2], ('name=foo', 'hello\nworld'))
154 self.assertEqual(len(entry.db_statements), 2)205 self.assertEqual(len(entry.db_statements), 2)
155 self.assertEqual(entry.db_statements[0], (1, 5, 'SELECT 1'))206 self.assertEqual(entry.db_statements[0], (1, 5, None, 'SELECT 1'))
156 self.assertEqual(entry.db_statements[1], (5, 10, 'SELECT 2'))207 self.assertEqual(entry.db_statements[1], (5, 10, None, 'SELECT 2'))
157208
158209
159class TestErrorReportingUtility(unittest.TestCase):210class TestErrorReportingUtility(unittest.TestCase):
@@ -422,7 +473,7 @@
422 # Test ErrorReportingUtility.raising() with an XML-RPC request.473 # Test ErrorReportingUtility.raising() with an XML-RPC request.
423 request = TestRequest()474 request = TestRequest()
424 directlyProvides(request, IXMLRPCRequest)475 directlyProvides(request, IXMLRPCRequest)
425 request.getPositionalArguments = lambda : (1,2)476 request.getPositionalArguments = lambda: (1, 2)
426 utility = ErrorReportingUtility()477 utility = ErrorReportingUtility()
427 now = datetime.datetime(2006, 04, 01, 00, 30, 00, tzinfo=UTC)478 now = datetime.datetime(2006, 04, 01, 00, 30, 00, tzinfo=UTC)
428 try:479 try:
@@ -469,7 +520,6 @@
469 utility.raising(sys.exc_info(), request, now=now)520 utility.raising(sys.exc_info(), request, now=now)
470 self.assertEqual(request.oopsid, None)521 self.assertEqual(request.oopsid, None)
471522
472
473 def test_raising_for_script(self):523 def test_raising_for_script(self):
474 """Test ErrorReportingUtility.raising with a ScriptRequest."""524 """Test ErrorReportingUtility.raising with a ScriptRequest."""
475 utility = ErrorReportingUtility()525 utility = ErrorReportingUtility()
@@ -753,7 +803,7 @@
753 # logged will have OOPS reports generated for them.803 # logged will have OOPS reports generated for them.
754 error_message = self.factory.getUniqueString()804 error_message = self.factory.getUniqueString()
755 try:805 try:
756 1/0806 ignored = 1/0
757 except ZeroDivisionError:807 except ZeroDivisionError:
758 self.logger.exception(error_message)808 self.logger.exception(error_message)
759 oops_report = self.error_utility.getLastOopsReport()809 oops_report = self.error_utility.getLastOopsReport()