Merge lp:~lifeless/launchpad/oops into lp:launchpad
- oops
- Merge into devel
Proposed by
Robert Collins
Status: | Merged | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Approved by: | Robert Collins | ||||||||||||
Approved revision: | no longer in the source branch. | ||||||||||||
Merged at revision: | 11497 | ||||||||||||
Proposed branch: | lp:~lifeless/launchpad/oops | ||||||||||||
Merge into: | lp:launchpad | ||||||||||||
Diff against target: |
824 lines (+463/-70) 17 files modified
lib/canonical/launchpad/pagetests/basics/page-request-summaries.txt (+7/-6) lib/canonical/launchpad/pagetests/basics/user-requested-oops.txt (+3/-3) lib/canonical/launchpad/webapp/adapter.py (+53/-44) lib/canonical/launchpad/webapp/errorlog.py (+13/-7) lib/canonical/launchpad/webapp/ftests/test_adapter.txt (+12/-0) lib/canonical/launchpad/webapp/servers.py (+3/-3) lib/canonical/launchpad/webapp/tests/test_publication.py (+2/-1) lib/canonical/launchpad/webapp/tests/test_user_requested_oops.py (+0/-5) lib/lp/app/browser/tests/base-layout.txt (+1/-1) lib/lp/services/timeline/__init__.py (+14/-0) lib/lp/services/timeline/requesttimeline.py (+51/-0) lib/lp/services/timeline/tests/__init__.py (+4/-0) lib/lp/services/timeline/tests/test_requesttimeline.py (+58/-0) lib/lp/services/timeline/tests/test_timedaction.py (+61/-0) lib/lp/services/timeline/tests/test_timeline.py (+50/-0) lib/lp/services/timeline/timedaction.py (+69/-0) lib/lp/services/timeline/timeline.py (+62/-0) |
||||||||||||
To merge this branch: | bzr merge lp:~lifeless/launchpad/oops | ||||||||||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Jeroen T. Vermeulen (community) | Approve | ||
Review via email: mp+34272@code.launchpad.net |
Commit message
Create lp.services.
Description of the change
Create a more generic environment for accruing/reporting on external/blocking actions.
To post a comment you must log in.
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'lib/canonical/launchpad/pagetests/basics/page-request-summaries.txt' |
2 | --- lib/canonical/launchpad/pagetests/basics/page-request-summaries.txt 2009-09-18 18:04:16 +0000 |
3 | +++ lib/canonical/launchpad/pagetests/basics/page-request-summaries.txt 2010-09-03 03:44:47 +0000 |
4 | @@ -1,12 +1,13 @@ |
5 | -Launchpad pages end with a comment that displays how many queries we've issued |
6 | -to render the page. Note that we say "at least" because unfortunately |
7 | -some queries may be issued after the page may finish rendering -- the |
8 | -authoritative source is in OOPS reports or in the web app's stderr. |
9 | +Launchpad pages end with a comment that displays how many actions outside of |
10 | +the regular python code we've issued to satisfy the request. Note that we say "at |
11 | +least" because unfortunately some queries may be issued after the page may |
12 | +finish rendering -- the authoritative source is in OOPS reports or in the web |
13 | +app's stderr. |
14 | |
15 | >>> browser.open('http://launchpad.dev/') |
16 | >>> print browser.contents |
17 | <!DOCTYPE... |
18 | - ...<!--... At least ... queries issued in ... seconds ...-->... |
19 | + ...<!--... At least ... actions issued in ... seconds ...-->... |
20 | |
21 | It's available for any page: |
22 | |
23 | @@ -14,4 +15,4 @@ |
24 | >>> browser.open('http://launchpad.dev/~mark/') |
25 | >>> print browser.contents |
26 | <!DOCTYPE... |
27 | - ...<!--... At least ... queries issued in ... seconds ...-->... |
28 | + ...<!--... At least ... actions issued in ... seconds ...-->... |
29 | |
30 | === modified file 'lib/canonical/launchpad/pagetests/basics/user-requested-oops.txt' |
31 | --- lib/canonical/launchpad/pagetests/basics/user-requested-oops.txt 2010-06-17 16:56:04 +0000 |
32 | +++ lib/canonical/launchpad/pagetests/basics/user-requested-oops.txt 2010-09-03 03:44:47 +0000 |
33 | @@ -3,7 +3,7 @@ |
34 | OOPSes happen from time to time in Launchpad, and they contain lots of |
35 | interesting information for helping debug. Sometimes though it is useful to |
36 | get some of this information even if there hasn't been a problem. The major |
37 | -use case for this is to get a dump of the database queries that a page does. |
38 | +use case for this is to get a dump of the blocking activities that a page does. |
39 | |
40 | A user can request an oops for a page by using the ++oops++ namespace in any |
41 | page traversal. |
42 | @@ -15,7 +15,7 @@ |
43 | >>> (page, summary) = browser.contents.split('</html>') |
44 | >>> print summary |
45 | <!-- ... |
46 | - At least ... queries issued in ... seconds OOPS-... |
47 | + At least ... actions issued in ... seconds OOPS-... |
48 | <BLANKLINE> |
49 | r... |
50 | --> |
51 | @@ -26,7 +26,7 @@ |
52 | >>> (page, summary) = browser.contents.split('</html>') |
53 | >>> print summary |
54 | <!-- ... |
55 | - At least ... queries issued in ... seconds OOPS-... |
56 | + At least ... actions issued in ... seconds OOPS-... |
57 | <BLANKLINE> |
58 | r... |
59 | --> |
60 | |
61 | === modified file 'lib/canonical/launchpad/webapp/adapter.py' |
62 | --- lib/canonical/launchpad/webapp/adapter.py 2010-08-20 20:31:18 +0000 |
63 | +++ lib/canonical/launchpad/webapp/adapter.py 2010-09-03 03:44:47 +0000 |
64 | @@ -6,6 +6,7 @@ |
65 | |
66 | __metaclass__ = type |
67 | |
68 | +import datetime |
69 | import os |
70 | import re |
71 | import sys |
72 | @@ -22,6 +23,7 @@ |
73 | ISOLATION_LEVEL_SERIALIZABLE, |
74 | QueryCanceledError, |
75 | ) |
76 | +import pytz |
77 | from storm.database import register_scheme |
78 | from storm.databases.postgres import ( |
79 | Postgres, |
80 | @@ -63,7 +65,12 @@ |
81 | SLAVE_FLAVOR, |
82 | ) |
83 | from canonical.launchpad.webapp.opstats import OpStats |
84 | -from canonical.lazr.utils import safe_hasattr |
85 | +from canonical.lazr.utils import get_current_browser_request, safe_hasattr |
86 | +from lp.services.timeline.timeline import Timeline |
87 | +from lp.services.timeline.requesttimeline import ( |
88 | + get_request_timeline, |
89 | + set_request_timeline, |
90 | + ) |
91 | |
92 | |
93 | __all__ = [ |
94 | @@ -80,6 +87,8 @@ |
95 | ] |
96 | |
97 | |
98 | +UTC = pytz.utc |
99 | + |
100 | classImplements(TimeoutError, IRequestExpired) |
101 | |
102 | |
103 | @@ -123,9 +132,9 @@ |
104 | pass |
105 | |
106 | def afterCompletion(self, txn): |
107 | - now = time() |
108 | - _log_statement( |
109 | - now, now, None, 'Transaction completed, status: %s' % txn.status) |
110 | + action = get_request_timeline(get_current_browser_request()).start( |
111 | + "SQL-nostore", 'Transaction completed, status: %s' % txn.status) |
112 | + action.finish() |
113 | |
114 | |
115 | def set_request_started( |
116 | @@ -151,10 +160,14 @@ |
117 | if starttime is None: |
118 | starttime = time() |
119 | _local.request_start_time = starttime |
120 | - if request_statements is None: |
121 | - _local.request_statements = [] |
122 | - else: |
123 | - _local.request_statements = request_statements |
124 | + request = get_current_browser_request() |
125 | + if request_statements is not None: |
126 | + # Specify a specific sequence object for the timeline. |
127 | + set_request_timeline(request, Timeline(request_statements)) |
128 | + else: |
129 | + # Ensure a timeline is created, so that time offset for actions is |
130 | + # reasonable. |
131 | + set_request_timeline(request, Timeline()) |
132 | _local.current_statement_timeout = None |
133 | _local.enable_timeout = enable_timeout |
134 | if txn is not None: |
135 | @@ -167,9 +180,9 @@ |
136 | """ |
137 | if getattr(_local, 'request_start_time', None) is None: |
138 | warnings.warn('clear_request_started() called outside of a request') |
139 | - |
140 | _local.request_start_time = None |
141 | - _local.request_statements = [] |
142 | + request = get_current_browser_request() |
143 | + set_request_timeline(request, Timeline()) |
144 | commit_logger = getattr(_local, 'commit_logger', None) |
145 | if commit_logger is not None: |
146 | _local.commit_logger.txn.unregisterSynch(_local.commit_logger) |
147 | @@ -179,7 +192,8 @@ |
148 | def summarize_requests(): |
149 | """Produce human-readable summary of requests issued so far.""" |
150 | secs = get_request_duration() |
151 | - statements = getattr(_local, 'request_statements', []) |
152 | + request = get_current_browser_request() |
153 | + timeline = get_request_timeline(request) |
154 | from canonical.launchpad.webapp.errorlog import ( |
155 | maybe_record_user_requested_oops) |
156 | oopsid = maybe_record_user_requested_oops() |
157 | @@ -187,14 +201,15 @@ |
158 | oops_str = "" |
159 | else: |
160 | oops_str = " %s" % oopsid |
161 | - log = "%s queries issued in %.2f seconds%s" % ( |
162 | - len(statements), secs, oops_str) |
163 | + log = "%s queries/external actions issued in %.2f seconds%s" % ( |
164 | + len(timeline.actions), secs, oops_str) |
165 | return log |
166 | |
167 | |
168 | def store_sql_statements_and_request_duration(event): |
169 | + actions = get_request_timeline(get_current_browser_request()).actions |
170 | event.request.setInWSGIEnvironment( |
171 | - 'launchpad.sqlstatements', len(get_request_statements())) |
172 | + 'launchpad.nonpythonactions', len(actions)) |
173 | event.request.setInWSGIEnvironment( |
174 | 'launchpad.requestduration', get_request_duration()) |
175 | |
176 | @@ -205,7 +220,16 @@ |
177 | The list is composed of (starttime, endtime, db_id, statement) tuples. |
178 | Times are given in milliseconds since the start of the request. |
179 | """ |
180 | - return getattr(_local, 'request_statements', []) |
181 | + result = [] |
182 | + request = get_current_browser_request() |
183 | + for action in get_request_timeline(request).actions: |
184 | + if not action.category.startswith("SQL-"): |
185 | + continue |
186 | + # Can't show incomplete requests in this API |
187 | + if action.duration is None: |
188 | + continue |
189 | + result.append(action.logTuple()) |
190 | + return result |
191 | |
192 | |
193 | def get_request_start_time(): |
194 | @@ -224,29 +248,6 @@ |
195 | return now - starttime |
196 | |
197 | |
198 | -def _log_statement(starttime, endtime, connection_wrapper, statement): |
199 | - """Log that a database statement was executed.""" |
200 | - request_starttime = getattr(_local, 'request_start_time', None) |
201 | - if request_starttime is None: |
202 | - return |
203 | - |
204 | - # convert times to integer millisecond values |
205 | - starttime = int((starttime - request_starttime) * 1000) |
206 | - endtime = int((endtime - request_starttime) * 1000) |
207 | - # A string containing no whitespace that lets us identify which Store |
208 | - # is being used. |
209 | - if connection_wrapper is not None: |
210 | - database_identifier = connection_wrapper._database.name |
211 | - else: |
212 | - database_identifier = None |
213 | - _local.request_statements.append( |
214 | - (starttime, endtime, database_identifier, statement)) |
215 | - |
216 | - # store the last executed statement as an attribute on the current |
217 | - # thread |
218 | - threading.currentThread().lp_last_sql_statement = statement |
219 | - |
220 | - |
221 | def _check_expired(timeout): |
222 | """Checks whether the current request has passed the given timeout.""" |
223 | if timeout is None or not getattr(_local, 'enable_timeout', True): |
224 | @@ -530,15 +531,23 @@ |
225 | if self._debug_sql or self._debug_sql_extra: |
226 | sys.stderr.write(statement + "\n") |
227 | sys.stderr.write("-" * 70 + "\n") |
228 | - |
229 | - now = time() |
230 | - connection._lp_statement_start_time = now |
231 | + # store the last executed statement as an attribute on the current |
232 | + # thread |
233 | + threading.currentThread().lp_last_sql_statement = statement |
234 | + request_starttime = getattr(_local, 'request_start_time', None) |
235 | + if request_starttime is None: |
236 | + return |
237 | + action = get_request_timeline(get_current_browser_request()).start( |
238 | + 'SQL-%s' % connection._database.name, statement) |
239 | + connection._lp_statement_action = action |
240 | |
241 | def connection_raw_execute_success(self, connection, raw_cursor, |
242 | statement, params): |
243 | - end = time() |
244 | - start = getattr(connection, '_lp_statement_start_time', end) |
245 | - _log_statement(start, end, connection, statement) |
246 | + action = getattr(connection, '_lp_statement_action', None) |
247 | + if action is not None: |
248 | + # action may be None if the tracer was installed the statement was |
249 | + # submitted. |
250 | + action.finish() |
251 | |
252 | def connection_raw_execute_error(self, connection, raw_cursor, |
253 | statement, params, error): |
254 | |
255 | === modified file 'lib/canonical/launchpad/webapp/errorlog.py' |
256 | --- lib/canonical/launchpad/webapp/errorlog.py 2010-08-26 02:47:21 +0000 |
257 | +++ lib/canonical/launchpad/webapp/errorlog.py 2010-09-03 03:44:47 +0000 |
258 | @@ -31,7 +31,7 @@ |
259 | from canonical.launchpad.layers import WebServiceLayer |
260 | from canonical.launchpad.webapp.adapter import ( |
261 | get_request_duration, |
262 | - get_request_statements, |
263 | + get_request_start_time, |
264 | soft_timeout_expired, |
265 | ) |
266 | from canonical.launchpad.webapp.interfaces import ( |
267 | @@ -42,7 +42,7 @@ |
268 | from canonical.launchpad.webapp.opstats import OpStats |
269 | from canonical.lazr.utils import safe_hasattr |
270 | from lp.services.log.uniquefileallocator import UniqueFileAllocator |
271 | - |
272 | +from lp.services.timeline.requesttimeline import get_request_timeline |
273 | |
274 | UTC = pytz.utc |
275 | |
276 | @@ -451,11 +451,17 @@ |
277 | strurl = _safestr(url) |
278 | |
279 | duration = get_request_duration() |
280 | - |
281 | - statements = sorted( |
282 | - (start, end, _safestr(database_id), _safestr(statement)) |
283 | - for (start, end, database_id, statement) |
284 | - in get_request_statements()) |
285 | + # In principle the timeline is per-request, but see bug=623199 - |
286 | + # at this point the request is optional, but get_request_timeline |
287 | + # does not care; when it starts caring, we will always have a |
288 | + # request object (or some annotations containing object). |
289 | + # RBC 20100901 |
290 | + timeline = get_request_timeline(request) |
291 | + statements = [] |
292 | + for action in timeline.actions: |
293 | + start, end, category, detail = action.logTuple() |
294 | + statements.append( |
295 | + (start, end, _safestr(category), _safestr(detail))) |
296 | |
297 | oopsid, filename = self.log_namer.newId(now) |
298 | |
299 | |
300 | === modified file 'lib/canonical/launchpad/webapp/ftests/test_adapter.txt' |
301 | --- lib/canonical/launchpad/webapp/ftests/test_adapter.txt 2010-03-25 11:58:42 +0000 |
302 | +++ lib/canonical/launchpad/webapp/ftests/test_adapter.txt 2010-09-03 03:44:47 +0000 |
303 | @@ -9,6 +9,7 @@ |
304 | >>> import threading |
305 | >>> import transaction |
306 | >>> from zope.component import getUtility |
307 | + >>> from lazr.restful.utils import get_current_browser_request |
308 | >>> from storm.zope.interfaces import IZStorm |
309 | >>> from canonical.launchpad.webapp.interfaces import ( |
310 | ... IStoreSelector, MAIN_STORE, MASTER_FLAVOR) |
311 | @@ -17,6 +18,7 @@ |
312 | >>> from canonical.launchpad.webapp.adapter import ( |
313 | ... clear_request_started, get_request_statements, |
314 | ... set_request_started) |
315 | + >>> from lp.services.timeline.requesttimeline import get_request_timeline |
316 | |
317 | There are several possible database connections available via the |
318 | IStoreSelector utility. |
319 | @@ -50,6 +52,16 @@ |
320 | SELECT 1 |
321 | SELECT 2 |
322 | |
323 | +A timeline is created too: |
324 | + |
325 | + >>> timeline = get_request_timeline(get_current_browser_request()) |
326 | + >>> for action in timeline.actions: |
327 | + ... if not action.category.startswith("SQL-"): |
328 | + ... continue |
329 | + ... print action.detail |
330 | + SELECT 1 |
331 | + SELECT 2 |
332 | + |
333 | After we complete the request, the statement log is cleared: |
334 | |
335 | >>> clear_request_started() |
336 | |
337 | === modified file 'lib/canonical/launchpad/webapp/servers.py' |
338 | --- lib/canonical/launchpad/webapp/servers.py 2010-09-03 03:12:39 +0000 |
339 | +++ lib/canonical/launchpad/webapp/servers.py 2010-09-03 03:44:47 +0000 |
340 | @@ -977,7 +977,7 @@ |
341 | request string (1st line of request) |
342 | response status |
343 | response bytes written |
344 | - number of sql statements |
345 | + number of nonpython statements (sql, email, memcache, rabbit etc) |
346 | request duration |
347 | number of ticks during traversal |
348 | number of ticks during publication |
349 | @@ -998,7 +998,7 @@ |
350 | bytes_written = task.bytes_written |
351 | userid = cgi_env.get('launchpad.userid', '') |
352 | pageid = cgi_env.get('launchpad.pageid', '') |
353 | - sql_statements = cgi_env.get('launchpad.sqlstatements', 0) |
354 | + nonpython_actions = cgi_env.get('launchpad.nonpythonactions', 0) |
355 | request_duration = cgi_env.get('launchpad.requestduration', 0) |
356 | traversal_ticks = cgi_env.get('launchpad.traversalticks', 0) |
357 | publication_ticks = cgi_env.get('launchpad.publicationticks', 0) |
358 | @@ -1016,7 +1016,7 @@ |
359 | first_line, |
360 | status, |
361 | bytes_written, |
362 | - sql_statements, |
363 | + nonpython_actions, |
364 | request_duration, |
365 | traversal_ticks, |
366 | publication_ticks, |
367 | |
368 | === modified file 'lib/canonical/launchpad/webapp/tests/test_publication.py' |
369 | --- lib/canonical/launchpad/webapp/tests/test_publication.py 2010-08-20 20:31:18 +0000 |
370 | +++ lib/canonical/launchpad/webapp/tests/test_publication.py 2010-09-03 03:44:47 +0000 |
371 | @@ -292,7 +292,8 @@ |
372 | next_oops = error_reporting_utility.getLastOopsReport() |
373 | |
374 | # Ensure the OOPS mentions the correct exception |
375 | - self.assertTrue(repr(next_oops).find("DisconnectionError") != -1) |
376 | + self.assertTrue(repr(next_oops).find("DisconnectionError") != -1, |
377 | + "next_oops was %r" % next_oops) |
378 | |
379 | # Ensure the OOPS is correctly marked as informational only. |
380 | self.assertEqual(next_oops.informational, 'True') |
381 | |
382 | === modified file 'lib/canonical/launchpad/webapp/tests/test_user_requested_oops.py' |
383 | --- lib/canonical/launchpad/webapp/tests/test_user_requested_oops.py 2010-08-20 20:31:18 +0000 |
384 | +++ lib/canonical/launchpad/webapp/tests/test_user_requested_oops.py 2010-09-03 03:44:47 +0000 |
385 | @@ -4,7 +4,6 @@ |
386 | |
387 | __metaclass__ = type |
388 | |
389 | -import unittest |
390 | |
391 | from lazr.restful.utils import get_current_browser_request |
392 | from zope.component import getUtility |
393 | @@ -84,7 +83,3 @@ |
394 | self.assertEqual(last_oops.type, 'UserRequestOops') |
395 | self.assertEqual(last_oops.informational, 'True') |
396 | |
397 | - |
398 | -def test_suite(): |
399 | - return unittest.TestLoader().loadTestsFromName(__name__) |
400 | - |
401 | |
402 | === modified file 'lib/lp/app/browser/tests/base-layout.txt' |
403 | --- lib/lp/app/browser/tests/base-layout.txt 2010-08-17 15:05:44 +0000 |
404 | +++ lib/lp/app/browser/tests/base-layout.txt 2010-09-03 03:44:47 +0000 |
405 | @@ -134,7 +134,7 @@ |
406 | Has global search: False |
407 | Has application tabs: False |
408 | Has side portlets: False |
409 | - At least ... queries issued in ... seconds |
410 | + At least ... queries... issued in ... seconds |
411 | Features: {} |
412 | in scopes {} |
413 | r... |
414 | |
415 | === added directory 'lib/lp/services/timeline' |
416 | === added file 'lib/lp/services/timeline/__init__.py' |
417 | --- lib/lp/services/timeline/__init__.py 1970-01-01 00:00:00 +0000 |
418 | +++ lib/lp/services/timeline/__init__.py 2010-09-03 03:44:47 +0000 |
419 | @@ -0,0 +1,14 @@ |
420 | +# Copyright 2010 Canonical Ltd. This software is licensed under the |
421 | +# GNU Affero General Public License version 3 (see the file LICENSE). |
422 | + |
423 | +"""lp.services.timeline provides a timeline for varied actions. |
424 | + |
425 | +This is used as part of determining where time goes in a request. |
426 | + |
427 | +NOTE that it is not LP's timeline-view for products, though they are similar in |
428 | +intent and concept (If a better name presents itself, this package may be |
429 | +renamed). |
430 | + |
431 | +Because this is part of lp.services, packages in this namespace can only use |
432 | +general LAZR or library functionality. |
433 | +""" |
434 | |
435 | === added file 'lib/lp/services/timeline/requesttimeline.py' |
436 | --- lib/lp/services/timeline/requesttimeline.py 1970-01-01 00:00:00 +0000 |
437 | +++ lib/lp/services/timeline/requesttimeline.py 2010-09-03 03:44:47 +0000 |
438 | @@ -0,0 +1,51 @@ |
439 | +# Copyright 2010 Canonical Ltd. This software is licensed under the |
440 | +# GNU Affero General Public License version 3 (see the file LICENSE). |
441 | + |
442 | +"""Manage a Timeline for a request.""" |
443 | + |
444 | +__all__ = [ |
445 | + 'get_request_timeline', |
446 | + 'set_request_timeline', |
447 | + ] |
448 | + |
449 | +__metaclass__ = type |
450 | + |
451 | +# XXX RobertCollins 2010-09-01 bug=623199: Undesirable but pragmatic. |
452 | +# Because of this bug, rather than using request.annotations we have |
453 | +# to work in with the webapp.adapter request model, which is |
454 | +# different to that used by get_current_browser_request. |
455 | +from canonical.launchpad import webapp |
456 | +from timeline import Timeline |
457 | + |
458 | + |
459 | +def get_request_timeline(request): |
460 | + """Get a `Timeline` for request. |
461 | + |
462 | + This should ideally return the request.annotations['timeline'], creating it |
463 | + if necessary. However due to bug 623199 it instead using the adapter |
464 | + context for 'requests'. Once bug 623199 is fixed it will instead use the |
465 | + request annotations. |
466 | + |
467 | + :param request: A zope/launchpad request object. |
468 | + :return: A lp.services.timeline.timeline.Timeline object for the request. |
469 | + """ |
470 | + try: |
471 | + return webapp.adapter._local.request_timeline |
472 | + except AttributeError: |
473 | + return set_request_timeline(request, Timeline()) |
474 | + # Disabled code path: bug 623199, ideally we would use this code path. |
475 | + return request.annotations.setdefault('timeline', Timeline()) |
476 | + |
477 | + |
478 | +def set_request_timeline(request, timeline): |
479 | + """Explicitly set a timeline for request. |
480 | + |
481 | + This is used by code which wants to manually assemble a timeline. |
482 | + |
483 | + :param request: A zope/launchpad request object. |
484 | + :param timeline: A Timeline. |
485 | + """ |
486 | + webapp.adapter._local.request_timeline = timeline |
487 | + return timeline |
488 | + # Disabled code path: bug 623199, ideally we would use this code path. |
489 | + request.annotations['timeline'] = timeline |
490 | |
491 | === added directory 'lib/lp/services/timeline/tests' |
492 | === added file 'lib/lp/services/timeline/tests/__init__.py' |
493 | --- lib/lp/services/timeline/tests/__init__.py 1970-01-01 00:00:00 +0000 |
494 | +++ lib/lp/services/timeline/tests/__init__.py 2010-09-03 03:44:47 +0000 |
495 | @@ -0,0 +1,4 @@ |
496 | +# Copyright 2010 Canonical Ltd. This software is licensed under the |
497 | +# GNU Affero General Public License version 3 (see the file LICENSE). |
498 | + |
499 | +"""Tests for lp.services.timeline.""" |
500 | |
501 | === added file 'lib/lp/services/timeline/tests/test_requesttimeline.py' |
502 | --- lib/lp/services/timeline/tests/test_requesttimeline.py 1970-01-01 00:00:00 +0000 |
503 | +++ lib/lp/services/timeline/tests/test_requesttimeline.py 2010-09-03 03:44:47 +0000 |
504 | @@ -0,0 +1,58 @@ |
505 | +# Copyright 2010 Canonical Ltd. This software is licensed under the |
506 | +# GNU Affero General Public License version 3 (see the file LICENSE). |
507 | + |
508 | +"""Tests of requesttimeline.""" |
509 | + |
510 | +__metaclass__ = type |
511 | + |
512 | +import testtools |
513 | +from zope.publisher.browser import TestRequest |
514 | + |
515 | +from canonical.launchpad import webapp |
516 | +from lp.services.timeline.requesttimeline import ( |
517 | + get_request_timeline, |
518 | + set_request_timeline, |
519 | + ) |
520 | +from lp.services.timeline.timeline import OverlappingActionError, Timeline |
521 | + |
522 | + |
523 | +class TestRequestTimeline(testtools.TestCase): |
524 | + |
525 | + # These disabled tests are for the desired API using request annotations. |
526 | + # bug=623199 describes some issues with why this doesn't work. |
527 | + def disabled_test_new_request_get_request_timeline_works(self): |
528 | + req = TestRequest() |
529 | + timeline = get_request_timeline(req) |
530 | + self.assertIsInstance(timeline, Timeline) |
531 | + |
532 | + def disabled_test_same_timeline_repeated_calls(self): |
533 | + req = TestRequest() |
534 | + timeline = get_request_timeline(req) |
535 | + self.assertEqual(timeline, get_request_timeline(req)) |
536 | + |
537 | + def disabled_test_set_timeline(self): |
538 | + req = TestRequest() |
539 | + timeline = Timeline() |
540 | + set_request_timeline(req, timeline) |
541 | + self.assertEqual(timeline, get_request_timeline(req)) |
542 | + |
543 | + # Tests while adapter._local contains the timeline --start--- |
544 | + # Delete when bug=623199 is fixed and the timeline store moved to |
545 | + # annotations. |
546 | + def test_new_request_get_request_timeline_uses_webapp(self): |
547 | + req = TestRequest() |
548 | + timeline = get_request_timeline(req) |
549 | + self.assertIsInstance(timeline, Timeline) |
550 | + self.assertTrue(webapp.adapter._local.request_timeline is timeline) |
551 | + |
552 | + def test_same_timeline_repeated_calls(self): |
553 | + req = TestRequest() |
554 | + timeline = get_request_timeline(req) |
555 | + self.assertEqual(timeline, get_request_timeline(req)) |
556 | + |
557 | + def test_set_timeline(self): |
558 | + req = TestRequest() |
559 | + timeline = Timeline() |
560 | + set_request_timeline(req, timeline) |
561 | + self.assertEqual(timeline, get_request_timeline(req)) |
562 | + # Tests while adapter._local contains the timeline ---end--- |
563 | |
564 | === added file 'lib/lp/services/timeline/tests/test_timedaction.py' |
565 | --- lib/lp/services/timeline/tests/test_timedaction.py 1970-01-01 00:00:00 +0000 |
566 | +++ lib/lp/services/timeline/tests/test_timedaction.py 2010-09-03 03:44:47 +0000 |
567 | @@ -0,0 +1,61 @@ |
568 | +# Copyright 2010 Canonical Ltd. This software is licensed under the |
569 | +# GNU Affero General Public License version 3 (see the file LICENSE). |
570 | + |
571 | +"""Tests of the TimedAction class.""" |
572 | + |
573 | +__metaclass__ = type |
574 | + |
575 | +import datetime |
576 | + |
577 | +import testtools |
578 | + |
579 | +from lp.services.timeline.timedaction import TimedAction |
580 | +from lp.services.timeline.timeline import Timeline |
581 | + |
582 | + |
583 | +class TestTimedAction(testtools.TestCase): |
584 | + |
585 | + def test_starts_now(self): |
586 | + action = TimedAction("Sending mail", None) |
587 | + self.assertIsInstance(action.start, datetime.datetime) |
588 | + |
589 | + def test_finish_sets_duration(self): |
590 | + action = TimedAction("Sending mail", None) |
591 | + self.assertEqual(None, action.duration) |
592 | + action.finish() |
593 | + self.assertIsInstance(action.duration, datetime.timedelta) |
594 | + |
595 | + def test__init__sets_category(self): |
596 | + action = TimedAction("Sending mail", None) |
597 | + self.assertEqual("Sending mail", action.category) |
598 | + |
599 | + def test__init__sets_detail(self): |
600 | + action = TimedAction(None, "fred.jones@example.com") |
601 | + self.assertEqual("fred.jones@example.com", action.detail) |
602 | + |
603 | + def test_logTuple(self): |
604 | + timeline = Timeline() |
605 | + action = TimedAction("foo", "bar", timeline) |
606 | + # Set variable for deterministic results |
607 | + action.start = timeline.baseline + datetime.timedelta(0, 0, 0, 2) |
608 | + action.duration = datetime.timedelta(0, 0, 0, 4) |
609 | + log_tuple = action.logTuple() |
610 | + self.assertEqual(4, len(log_tuple), "!= 4 elements %s" % (log_tuple,)) |
611 | + self.assertAlmostEqual(2, log_tuple[0]) |
612 | + self.assertAlmostEqual(4, log_tuple[1]) |
613 | + self.assertEqual("foo", log_tuple[2]) |
614 | + self.assertEqual("bar", log_tuple[3]) |
615 | + |
616 | + def test_logTupleIncomplete(self): |
617 | + # Things that start and hit a timeout *may* not get recorded as |
618 | + # finishing in normal operation. |
619 | + timeline = Timeline() |
620 | + action = TimedAction("foo", "bar", timeline) |
621 | + # Set variable for deterministic results |
622 | + action.start = timeline.baseline + datetime.timedelta(0, 0, 0, 2) |
623 | + log_tuple = action.logTuple() |
624 | + self.assertEqual(4, len(log_tuple), "!= 4 elements %s" % (log_tuple,)) |
625 | + self.assertAlmostEqual(2, log_tuple[0]) |
626 | + self.assertAlmostEqual(999999, log_tuple[1]) |
627 | + self.assertEqual("foo", log_tuple[2]) |
628 | + self.assertEqual("bar", log_tuple[3]) |
629 | |
630 | === added file 'lib/lp/services/timeline/tests/test_timeline.py' |
631 | --- lib/lp/services/timeline/tests/test_timeline.py 1970-01-01 00:00:00 +0000 |
632 | +++ lib/lp/services/timeline/tests/test_timeline.py 2010-09-03 03:44:47 +0000 |
633 | @@ -0,0 +1,50 @@ |
634 | +# Copyright 2010 Canonical Ltd. This software is licensed under the |
635 | +# GNU Affero General Public License version 3 (see the file LICENSE). |
636 | + |
637 | +"""Tests of the Timeline class.""" |
638 | + |
639 | +__metaclass__ = type |
640 | + |
641 | +import datetime |
642 | + |
643 | +import testtools |
644 | + |
645 | +from lp.services.timeline.timedaction import TimedAction |
646 | +from lp.services.timeline.timeline import OverlappingActionError, Timeline |
647 | + |
648 | + |
649 | +class TestTimeline(testtools.TestCase): |
650 | + |
651 | + def test_start_returns_action(self): |
652 | + timeline = Timeline() |
653 | + action = timeline.start("Sending mail", "Noone") |
654 | + self.assertIsInstance(action, TimedAction) |
655 | + self.assertEqual("Sending mail", action.category) |
656 | + self.assertEqual("Noone", action.detail) |
657 | + self.assertEqual(None, action.duration) |
658 | + self.assertEqual(timeline, action.timeline) |
659 | + |
660 | + def test_can_supply_list(self): |
661 | + actions = "foo" |
662 | + timeline = Timeline(actions) |
663 | + self.assertEqual(actions, timeline.actions) |
664 | + |
665 | + def test_start_with_unfinished_action_fails(self): |
666 | + # A design constraint of timeline says that overlapping actions are not |
667 | + # permitted. See the Timeline docstrings. |
668 | + timeline = Timeline() |
669 | + action = timeline.start("Sending mail", "Noone") |
670 | + self.assertRaises(OverlappingActionError, timeline.start, |
671 | + "Sending mail", "Noone") |
672 | + |
673 | + def test_start_after_finish_works(self): |
674 | + timeline = Timeline() |
675 | + action = timeline.start("Sending mail", "Noone") |
676 | + action.finish() |
677 | + action = timeline.start("Sending mail", "Noone") |
678 | + action.finish() |
679 | + self.assertEqual(2, len(timeline.actions)) |
680 | + |
681 | + def test_baseline(self): |
682 | + timeline = Timeline() |
683 | + self.assertIsInstance(timeline.baseline, datetime.datetime) |
684 | |
685 | === added file 'lib/lp/services/timeline/timedaction.py' |
686 | --- lib/lp/services/timeline/timedaction.py 1970-01-01 00:00:00 +0000 |
687 | +++ lib/lp/services/timeline/timedaction.py 2010-09-03 03:44:47 +0000 |
688 | @@ -0,0 +1,69 @@ |
689 | +# Copyright 2010 Canonical Ltd. This software is licensed under the |
690 | +# GNU Affero General Public License version 3 (see the file LICENSE). |
691 | + |
692 | +"""Time a single categorised action.""" |
693 | + |
694 | + |
695 | +__all__ = ['TimedAction'] |
696 | + |
697 | +__metaclass__ = type |
698 | + |
699 | + |
700 | +import datetime |
701 | + |
702 | +import pytz |
703 | + |
704 | + |
705 | +UTC = pytz.utc |
706 | + |
707 | + |
708 | +class TimedAction: |
709 | + """An individual action which has been timed. |
710 | + |
711 | + :ivar timeline: The timeline that this action took place within. |
712 | + :ivar start: A datetime object with tz for the start of the action. |
713 | + :ivar duration: A timedelta for the duration of the action. None for |
714 | + actions which have not completed. |
715 | + :ivar category: The category of the action. E.g. "sql". |
716 | + :ivar detail: The detail about the action. E.g. "SELECT COUNT(*) ..." |
717 | + """ |
718 | + |
719 | + def __init__(self, category, detail, timeline=None): |
720 | + """Create a TimedAction. |
721 | + |
722 | + New TimedActions have a start but no duration. |
723 | + |
724 | + :param category: The category for the action. |
725 | + :param detail: The detail about the action being timed. |
726 | + :param timeline: The timeline for the action. |
727 | + """ |
728 | + self.start = datetime.datetime.now(UTC) |
729 | + self.duration = None |
730 | + self.category = category |
731 | + self.detail = detail |
732 | + self.timeline = timeline |
733 | + |
734 | + def logTuple(self): |
735 | + """Return a 4-tuple suitable for errorlog's use.""" |
736 | + offset = self._td_to_ms(self.start - self.timeline.baseline) |
737 | + if self.duration is None: |
738 | + # This action wasn't finished: give it a duration that will stand |
739 | + # out. This is pretty normal when action ends are recorded by |
740 | + # callbacks rather than stack-like structures. E.g. storm |
741 | + # tracers in launchpad: |
742 | + # log-trace START : starts action |
743 | + # timeout-trace START : raises |
744 | + # log-trace FINISH is never called. |
745 | + length = 999999 |
746 | + else: |
747 | + length = self._td_to_ms(self.duration) |
748 | + return (offset, length, self.category, self.detail) |
749 | + |
750 | + def _td_to_ms(self, td): |
751 | + """Tweak on a backport from python 2.7""" |
752 | + return (td.microseconds + ( |
753 | + td.seconds + td.days * 24 * 3600) * 10**6) / 10**3 |
754 | + |
755 | + def finish(self): |
756 | + """Mark the TimedAction as finished.""" |
757 | + self.duration = datetime.datetime.now(UTC) - self.start |
758 | |
759 | === added file 'lib/lp/services/timeline/timeline.py' |
760 | --- lib/lp/services/timeline/timeline.py 1970-01-01 00:00:00 +0000 |
761 | +++ lib/lp/services/timeline/timeline.py 2010-09-03 03:44:47 +0000 |
762 | @@ -0,0 +1,62 @@ |
763 | +# Copyright 2010 Canonical Ltd. This software is licensed under the |
764 | +# GNU Affero General Public License version 3 (see the file LICENSE). |
765 | + |
766 | +"""Coordinate a sequence of non overlapping TimedActionss.""" |
767 | + |
768 | +__all__ = ['Timeline'] |
769 | + |
770 | +__metaclass__ = type |
771 | + |
772 | +import datetime |
773 | + |
774 | +from pytz import utc as UTC |
775 | + |
776 | +from timedaction import TimedAction |
777 | + |
778 | + |
779 | +class OverlappingActionError(Exception): |
780 | + """A new action was attempted without finishing the prior one.""" |
781 | + # To make analysis easy we do not permit overlapping actions: each |
782 | + # action that is being timed and accrued must complete before the next |
783 | + # is started. This means, for instance, that sending mail cannot do SQL |
784 | + # queries, as both are timed and accrued. OTOH it makes analysis and |
785 | + # serialisation of timelines simpler, and for the current use cases in |
786 | + # Launchpad this is sufficient. This constraint should not be considered |
787 | + # sacrosanct - if, in future, we desire timelines with overlapping actions, |
788 | + # as long as the OOPS analysis code is extended to generate sensible |
789 | + # reports in those situations, this can be changed. |
790 | + |
791 | + |
792 | +class Timeline: |
793 | + """A sequence of TimedActions. |
794 | + |
795 | + This is used for collecting expensive/external actions inside Launchpad |
796 | + requests. |
797 | + |
798 | + :ivar actions: The actions. |
799 | + :ivar baseline: The point the timeline starts at. |
800 | + """ |
801 | + |
802 | + def __init__(self, actions=None): |
803 | + """Create a Timeline. |
804 | + |
805 | + :param actions: An optional object to use to store the timeline. This |
806 | + must implement the list protocol. |
807 | + """ |
808 | + if actions is None: |
809 | + actions = [] |
810 | + self.actions = actions |
811 | + self.baseline = datetime.datetime.now(UTC) |
812 | + |
813 | + def start(self, category, detail): |
814 | + """Create a new TimedAction at the end of the timeline. |
815 | + |
816 | + :param category: the category for the action. |
817 | + :param detail: The detail for the action. |
818 | + :return: A TimedAction for that category and detail. |
819 | + """ |
820 | + result = TimedAction(category, detail, self) |
821 | + if self.actions and self.actions[-1].duration is None: |
822 | + raise OverlappingActionError(self.actions[-1], result) |
823 | + self.actions.append(result) |
824 | + return result |
This was not one of those rubber-stamp reviews. :-)
The biggest sticking point, if a small one, was the dead code left in there to document the expected future state of the code. Please resolve that situation as soon as possible, of course.
We found that Timeline.baseline was at risk of ambiguity, because the Timeline could in principle be created on-demand in the middle of handling a request. We considered always having a "Start timeline" action, but decided against it. Ultimately you went with the option of creating the Timeline early on when handling a request, so that the baseline is always more or less the time we start handling the request.