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
1=== modified file 'lib/canonical/launchpad/webapp/adapter.py'
2--- lib/canonical/launchpad/webapp/adapter.py 2009-07-28 22:35:01 +0000
3+++ lib/canonical/launchpad/webapp/adapter.py 2009-07-29 11:37:51 +0000
4@@ -134,7 +134,7 @@
5 def get_request_statements():
6 """Get the list of executed statements in the request.
7
8- The list is composed of (starttime, endtime, statement) tuples.
9+ The list is composed of (starttime, endtime, db_id, statement) tuples.
10 Times are given in milliseconds since the start of the request.
11 """
12 return getattr(_local, 'request_statements', [])
13@@ -165,7 +165,11 @@
14 # convert times to integer millisecond values
15 starttime = int((starttime - request_starttime) * 1000)
16 endtime = int((endtime - request_starttime) * 1000)
17- _local.request_statements.append((starttime, endtime, statement))
18+ # A string containing no whitespace that lets us identify which Store
19+ # is being used.
20+ database_identifier = connection_wrapper._database.name
21+ _local.request_statements.append(
22+ (starttime, endtime, database_identifier, statement))
23
24 # store the last executed statement as an attribute on the current
25 # thread
26@@ -274,6 +278,8 @@
27 # opinion on what uri is.
28 # pylint: disable-msg=W0231
29 self._uri = uri
30+ # A unique name for this database connection.
31+ self.name = uri.database
32
33 def raw_connect(self):
34 # Prevent database connections from the main thread if
35@@ -348,6 +354,9 @@
36
37 class LaunchpadSessionDatabase(Postgres):
38
39+ # A unique name for this database connection.
40+ name = 'session'
41+
42 def raw_connect(self):
43 self._dsn = 'dbname=%s user=%s' % (config.launchpad_session.dbname,
44 config.launchpad_session.dbuser)
45
46=== modified file 'lib/canonical/launchpad/webapp/errorlog.py'
47--- lib/canonical/launchpad/webapp/errorlog.py 2009-07-23 05:02:47 +0000
48+++ lib/canonical/launchpad/webapp/errorlog.py 2009-07-29 11:37:51 +0000
49@@ -167,9 +167,9 @@
50 fp.write('%s=%s\n' % (urllib.quote(key, safe_chars),
51 urllib.quote(value, safe_chars)))
52 fp.write('\n')
53- for (start, end, statement) in self.db_statements:
54- fp.write('%05d-%05d %s\n' % (start, end,
55- _normalise_whitespace(statement)))
56+ for (start, end, database_id, statement) in self.db_statements:
57+ fp.write('%05d-%05d@%s %s\n' % (
58+ start, end, database_id, _normalise_whitespace(statement)))
59 fp.write('\n')
60 fp.write(self.tb_text)
61
62@@ -206,9 +206,15 @@
63 line = line.strip()
64 if line == '':
65 break
66- startend, statement = line.split(' ', 1)
67- start, end = startend.split('-')
68- statements.append((int(start), int(end), statement))
69+ preamble, statement = line.split(' ', 1)
70+ start, suffix = preamble.split('-', 1)
71+ try:
72+ end, db_id = suffix.split('@', 1)
73+ db_id = intern(database_id) # This string is repeated lots.
74+ except ValueError:
75+ end, db_id = suffix, None
76+ statements.append(
77+ (int(start), int(end), db_id, statement))
78
79 # The rest is traceback.
80 tb_text = ''.join(lines)
81@@ -471,9 +477,10 @@
82
83 duration = get_request_duration()
84
85- statements = sorted((start, end, _safestr(statement))
86- for (start, end, statement)
87- in get_request_statements())
88+ statements = sorted(
89+ (start, end, _safestr(database_id), _safestr(statement))
90+ for (start, end, database_id, statement)
91+ in get_request_statements())
92
93 oopsid, filename = self.newOopsId(now)
94
95
96=== modified file 'lib/canonical/launchpad/webapp/tests/test_errorlog.py'
97--- lib/canonical/launchpad/webapp/tests/test_errorlog.py 2009-07-17 00:26:05 +0000
98+++ lib/canonical/launchpad/webapp/tests/test_errorlog.py 2009-07-29 11:05:49 +0000
99@@ -57,8 +57,8 @@
100 'pageid', 'traceback-text', 'username', 'url', 42,
101 [('name1', 'value1'), ('name2', 'value2'),
102 ('name1', 'value3')],
103- [(1, 5, 'SELECT 1'),
104- (5, 10, 'SELECT 2')])
105+ [(1, 5, 'store_a', 'SELECT 1'),
106+ (5, 10, 'store_b', 'SELECT 2')])
107 self.assertEqual(entry.id, 'id')
108 self.assertEqual(entry.type, 'exc-type')
109 self.assertEqual(entry.value, 'exc-value')
110@@ -74,8 +74,12 @@
111 self.assertEqual(entry.req_vars[1], ('name2', 'value2'))
112 self.assertEqual(entry.req_vars[2], ('name1', 'value3'))
113 self.assertEqual(len(entry.db_statements), 2)
114- self.assertEqual(entry.db_statements[0], (1, 5, 'SELECT 1'))
115- self.assertEqual(entry.db_statements[1], (5, 10, 'SELECT 2'))
116+ self.assertEqual(
117+ entry.db_statements[0],
118+ (1, 5, 'store_a', 'SELECT 1'))
119+ self.assertEqual(
120+ entry.db_statements[1],
121+ (5, 10, 'store_b', 'SELECT 2'))
122
123 def test_write(self):
124 """Test ErrorReport.write()"""
125@@ -88,8 +92,8 @@
126 [('HTTP_USER_AGENT', 'Mozilla/5.0'),
127 ('HTTP_REFERER', 'http://localhost:9000/'),
128 ('name=foo', 'hello\nworld')],
129- [(1, 5, 'SELECT 1'),
130- (5, 10, 'SELECT\n2')])
131+ [(1, 5, 'store_a', 'SELECT 1'),
132+ (5, 10,'store_b', 'SELECT\n2')])
133 fp = StringIO.StringIO()
134 entry.write(fp)
135 self.assertEqual(fp.getvalue(), dedent("""\
136@@ -108,13 +112,60 @@
137 HTTP_REFERER=http://localhost:9000/
138 name%%3Dfoo=hello%%0Aworld
139
140- 00001-00005 SELECT 1
141- 00005-00010 SELECT 2
142+ 00001-00005@store_a SELECT 1
143+ 00005-00010@store_b SELECT 2
144
145 traceback-text""" % (versioninfo.branch_nick, versioninfo.revno)))
146
147 def test_read(self):
148- """Test ErrorReport.read()"""
149+ """Test ErrorReport.read()."""
150+ fp = StringIO.StringIO(dedent("""\
151+ Oops-Id: OOPS-A0001
152+ Exception-Type: NotFound
153+ Exception-Value: error message
154+ Date: 2005-04-01T00:00:00+00:00
155+ Page-Id: IFoo:+foo-template
156+ User: Sample User
157+ URL: http://localhost:9000/foo
158+ Duration: 42
159+
160+ HTTP_USER_AGENT=Mozilla/5.0
161+ HTTP_REFERER=http://localhost:9000/
162+ name%3Dfoo=hello%0Aworld
163+
164+ 00001-00005@store_a SELECT 1
165+ 00005-00010@store_b SELECT 2
166+
167+ traceback-text"""))
168+ entry = ErrorReport.read(fp)
169+ self.assertEqual(entry.id, 'OOPS-A0001')
170+ self.assertEqual(entry.type, 'NotFound')
171+ self.assertEqual(entry.value, 'error message')
172+ # XXX jamesh 2005-11-30:
173+ # this should probably convert back to a datetime
174+ self.assertEqual(entry.time, datetime.datetime(2005, 4, 1))
175+ self.assertEqual(entry.pageid, 'IFoo:+foo-template')
176+ self.assertEqual(entry.tb_text, 'traceback-text')
177+ self.assertEqual(entry.username, 'Sample User')
178+ self.assertEqual(entry.url, 'http://localhost:9000/foo')
179+ self.assertEqual(entry.duration, 42)
180+ self.assertEqual(len(entry.req_vars), 3)
181+ self.assertEqual(entry.req_vars[0], ('HTTP_USER_AGENT',
182+ 'Mozilla/5.0'))
183+ self.assertEqual(entry.req_vars[1], ('HTTP_REFERER',
184+ 'http://localhost:9000/'))
185+ self.assertEqual(entry.req_vars[2], ('name=foo', 'hello\nworld'))
186+ self.assertEqual(len(entry.db_statements), 2)
187+ self.assertEqual(
188+ entry.db_statements[0],
189+ (1, 5, 'store_a', 'SELECT 1'))
190+ self.assertEqual(
191+ entry.db_statements[1],
192+ (5, 10, 'store_b', 'SELECT 2'))
193+
194+
195+ def test_read_no_store_id(self):
196+ """Test ErrorReport.read() for old logs with no store_id."""
197 fp = StringIO.StringIO(dedent("""\
198 Oops-Id: OOPS-A0001
199 Exception-Type: NotFound
200@@ -152,8 +203,8 @@
201 'http://localhost:9000/'))
202 self.assertEqual(entry.req_vars[2], ('name=foo', 'hello\nworld'))
203 self.assertEqual(len(entry.db_statements), 2)
204- self.assertEqual(entry.db_statements[0], (1, 5, 'SELECT 1'))
205- self.assertEqual(entry.db_statements[1], (5, 10, 'SELECT 2'))
206+ self.assertEqual(entry.db_statements[0], (1, 5, None, 'SELECT 1'))
207+ self.assertEqual(entry.db_statements[1], (5, 10, None, 'SELECT 2'))
208
209
210 class TestErrorReportingUtility(unittest.TestCase):
211@@ -422,7 +473,7 @@
212 # Test ErrorReportingUtility.raising() with an XML-RPC request.
213 request = TestRequest()
214 directlyProvides(request, IXMLRPCRequest)
215- request.getPositionalArguments = lambda : (1,2)
216+ request.getPositionalArguments = lambda: (1, 2)
217 utility = ErrorReportingUtility()
218 now = datetime.datetime(2006, 04, 01, 00, 30, 00, tzinfo=UTC)
219 try:
220@@ -469,7 +520,6 @@
221 utility.raising(sys.exc_info(), request, now=now)
222 self.assertEqual(request.oopsid, None)
223
224-
225 def test_raising_for_script(self):
226 """Test ErrorReportingUtility.raising with a ScriptRequest."""
227 utility = ErrorReportingUtility()
228@@ -753,7 +803,7 @@
229 # logged will have OOPS reports generated for them.
230 error_message = self.factory.getUniqueString()
231 try:
232- 1/0
233+ ignored = 1/0
234 except ZeroDivisionError:
235 self.logger.exception(error_message)
236 oops_report = self.error_utility.getLastOopsReport()