launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05534
[Merge] lp:~lifeless/launchpad/oops-polish into lp:launchpad
Robert Collins has proposed merging lp:~lifeless/launchpad/oops-polish into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #888866 in Launchpad itself: "req_vars is documented as a list of tuples"
https://bugs.launchpad.net/launchpad/+bug/888866
Bug #890975 in Launchpad itself: "OOPS report urls are sent as unicode"
https://bugs.launchpad.net/launchpad/+bug/890975
Bug #890976 in Launchpad itself: "hard to determine causes of late evaluation"
https://bugs.launchpad.net/launchpad/+bug/890976
For more details, see:
https://code.launchpad.net/~lifeless/launchpad/oops-polish/+merge/82359
Merge in and integrate the latest set of OOPS improvements:
- req_vars is a dict (easier to work with)
- oops-tools is more cautious about unicode urls - so we should be emitting str urls.
- timeline grabs backtraces automatically
oops-tools is already running code ready to accept both req_vars as dicts and timelines with backtraces. It won't render the backtraces yet, but the raw data will be in the oops on disk, which is a good start.
--
https://code.launchpad.net/~lifeless/launchpad/oops-polish/+merge/82359
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/oops-polish into lp:launchpad.
=== modified file 'lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py'
--- lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py 2011-10-17 03:30:53 +0000
+++ lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py 2011-11-16 06:39:24 +0000
@@ -5456,7 +5456,7 @@
error_report = self.oopses[0]
self.assertEqual('ZeroDivisionError', error_report['type'])
self.assertStartsWith(
- dict(error_report['req_vars'])['error-explanation'],
+ error_report['req_vars']['error-explanation'],
'Exception while processing HWDB')
messages = [record.getMessage() for record in self.handler.records]
=== modified file 'lib/canonical/launchpad/webapp/adapter.py'
--- lib/canonical/launchpad/webapp/adapter.py 2011-10-11 07:09:47 +0000
+++ lib/canonical/launchpad/webapp/adapter.py 2011-11-16 06:39:24 +0000
@@ -737,7 +737,8 @@
action = getattr(connection, '_lp_statement_action', None)
if action is not None:
# action may be None if the tracer was installed after the
- # statement was submitted.
+ # statement was submitted or if the timeline tracer is not
+ # installed.
action.finish()
log_sql = getattr(_local, 'sql_logging', None)
if log_sql is not None or self._debug_sql or self._debug_sql_extra:
@@ -754,13 +755,16 @@
# Times are in milliseconds, to mirror actions.
start = start - logging_start
stop = stop - logging_start
- data = (start, stop, dbname, statement)
+ data = (start, stop, dbname, statement, None)
connection._lp_statement_info = None
if data is not None:
if log_sql and log_sql[-1]['sql'] is None:
log_sql[-1]['sql'] = data
if self._debug_sql or self._debug_sql_extra:
- sys.stderr.write('%d-%d@%s %s\n' % data)
+ # Don't print the backtrace from the data to stderr - too
+ # messy given that LP_DEBUG_SQL_EXTRA logs that
+ # separately anyhow.
+ sys.stderr.write('%d-%d@%s %s\n' % data[:4])
sys.stderr.write("-" * 70 + "\n")
def connection_raw_execute_error(self, connection, raw_cursor,
=== modified file 'lib/canonical/launchpad/webapp/errorlog.py'
--- lib/canonical/launchpad/webapp/errorlog.py 2011-11-14 11:08:10 +0000
+++ lib/canonical/launchpad/webapp/errorlog.py 2011-11-16 06:39:24 +0000
@@ -187,7 +187,14 @@
# XXX jamesh 2005-11-22: Temporary fix, which Steve should
# undo. URL is just too HTTPRequest-specific.
if safe_hasattr(request, 'URL'):
- report['url'] = oops.createhooks.safe_unicode(request.URL)
+ # URL's are byte strings, but possibly str() will fail - safe_unicode
+ # handles all those cases, and then we can safely encode it to utf8.
+ # This is strictly double handling as a URL should never have unicode
+ # characters in it anyway (though it may have them % encoded, which is
+ # fine). Better safe than sorry, and the safe_unicode handling won't
+ # cause double-encoding, so it is safe.
+ url = oops.createhooks.safe_unicode(request.URL).encode('utf8')
+ report['url'] = url
if WebServiceLayer.providedBy(request) and info is not None:
webservice_error = getattr(
@@ -225,12 +232,13 @@
for key, value in request.items():
if _is_sensitive(request, key):
- report['req_vars'].append((key, '<hidden>'))
- else:
- report['req_vars'].append((key, value))
+ value = '<hidden>'
+ report['req_vars'][key] = value
if IXMLRPCRequest.providedBy(request):
args = request.getPositionalArguments()
- report['req_vars'].append(('xmlrpc args', args))
+ # Request variables are strings: this could move to its own key and be
+ # raw.
+ report['req_vars']['xmlrpc args'] = unicode(args)
def attach_ignore_from_exception(report, context):
@@ -303,7 +311,7 @@
reporter = config[section_name].oops_prefix
self._oops_config.template['reporter'] = reporter
# Should go in an HTTP module.
- self._oops_config.template['req_vars'] = []
+ self._oops_config.template['req_vars'] = {}
# Exceptions, with the zope formatter.
self._oops_config.on_create.append(attach_exc_info)
# Ignore IUnloggedException exceptions
@@ -374,8 +382,6 @@
if timeline is not None:
context['timeline'] = timeline
report = self._oops_config.create(context)
- # req_vars should be a dict itself. Needs an oops-datedir-repo tweak.
- report['req_vars'].sort()
if self._oops_config.publish(report) is None:
return
if request:
@@ -388,7 +394,7 @@
if _get_type(report) in self._ignored_exceptions_for_offsite_referer:
was_http = report.get('url', '').lower().startswith('http')
if was_http:
- req_vars = dict(report.get('req_vars', ()))
+ req_vars = report.get('req_vars', {})
referer = req_vars.get('HTTP_REFERER')
# If there is no referrer then either the user has refer
# disabled, or its someone coming from offsite or from some
@@ -407,9 +413,9 @@
"""merges self._oops_messages into the report req_vars variable."""
# XXX AaronBentley 2009-11-26 bug=488950: There should be separate
# storage for oops messages.
- report['req_vars'].extend(
- ('<oops-message-%d>' % key, str(message)) for key, message
- in self._oops_messages.iteritems())
+ req_vars = report['req_vars']
+ for key, message in self._oops_messages.items():
+ req_vars['<oops-message-%d>' % key] = str(message)
@contextlib.contextmanager
def oopsMessage(self, message):
=== modified file 'lib/canonical/launchpad/webapp/tests/test_errorlog.py'
--- lib/canonical/launchpad/webapp/tests/test_errorlog.py 2011-11-14 10:19:17 +0000
+++ lib/canonical/launchpad/webapp/tests/test_errorlog.py 2011-11-16 06:39:24 +0000
@@ -64,8 +64,8 @@
"""Test ErrorReport.__init__()"""
entry = ErrorReport('id', 'exc-type', 'exc-value', 'timestamp',
'traceback-text', 'username', 'url', 42,
- [('name1', 'value1'), ('name2', 'value2'),
- ('name1', 'value3')],
+ {'name1': 'value1', 'name2': 'value2',
+ 'name3': 'value3'},
[(1, 5, 'store_a', 'SELECT 1'),
(5, 10, 'store_b', 'SELECT 2')],
topic='pageid',
@@ -80,10 +80,11 @@
self.assertEqual(entry.username, 'username')
self.assertEqual(entry.url, 'url')
self.assertEqual(entry.duration, 42)
- self.assertEqual(len(entry.req_vars), 3)
- self.assertEqual(entry.req_vars[0], ('name1', 'value1'))
- self.assertEqual(entry.req_vars[1], ('name2', 'value2'))
- self.assertEqual(entry.req_vars[2], ('name1', 'value3'))
+ self.assertEqual({
+ 'name1': 'value1',
+ 'name2': 'value2',
+ 'name3': 'value3',
+ }, entry.req_vars)
self.assertEqual(len(entry.timeline), 2)
self.assertEqual(entry.timeline[0], (1, 5, 'store_a', 'SELECT 1'))
self.assertEqual(entry.timeline[1], (5, 10, 'store_b', 'SELECT 2'))
@@ -120,12 +121,11 @@
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({
+ 'HTTP_USER_AGENT': 'Mozilla/5.0',
+ 'HTTP_REFERER': 'http://localhost:9000/',
+ 'name=foo': 'hello\nworld'},
+ entry.req_vars)
self.assertEqual(len(entry.timeline), 2)
self.assertEqual(entry.timeline[0], [1, 5, 'store_a', 'SELECT 1'])
self.assertEqual(entry.timeline[1], [5, 10, 'store_b', 'SELECT 2'])
@@ -243,7 +243,7 @@
'lp': '<hidden>',
'name1': 'value3 \xa7',
'name2': 'value2',
- }, dict(report['req_vars']))
+ }, report['req_vars'])
# verify that the oopsid was set on the request
self.assertEqual(request.oopsid, report['id'])
self.assertEqual(request.oops, report)
@@ -259,7 +259,7 @@
raise ArbitraryException('xyz\nabc')
except ArbitraryException:
report = utility.raising(sys.exc_info(), request)
- self.assertEqual(('xmlrpc args', (1, 2)), report['req_vars'][-1])
+ self.assertEqual("(1, 2)", report['req_vars']['xmlrpc args'])
def test_raising_with_webservice_request(self):
# Test ErrorReportingUtility.raising() with a WebServiceRequest
@@ -302,6 +302,8 @@
utility = ErrorReportingUtility()
del utility._oops_config.publishers[:]
+ # A list because code using ScriptRequest expects that - ScriptRequest
+ # translates it to a dict for now.
req_vars = [
('name2', 'value2'),
('name1', 'value1'),
@@ -317,7 +319,7 @@
report = utility.raising(sys.exc_info(), request)
self.assertEqual(url, report['url'])
- self.assertEqual(sorted(req_vars), report['req_vars'])
+ self.assertEqual(dict(req_vars), report['req_vars'])
def test_raising_with_unprintable_exception(self):
class UnprintableException(Exception):
@@ -530,7 +532,7 @@
info = sys.exc_info()
oops = utility._oops_config.create(dict(exc_info=info))
self.assertEqual(
- [('<oops-message-0>', "{'a': 'b', 'c': 'd'}")],
+ {'<oops-message-0>': "{'a': 'b', 'c': 'd'}"},
oops['req_vars'])
def test__makeErrorReport_combines_request_and_error_vars(self):
@@ -546,8 +548,8 @@
oops = utility._oops_config.create(
dict(exc_info=info, http_request=request))
self.assertEqual(
- [('<oops-message-0>', "{'a': 'b'}"), ('c', 'd')],
- sorted(oops['req_vars']))
+ {'<oops-message-0>': "{'a': 'b'}", 'c': 'd'},
+ oops['req_vars'])
def test_filter_session_statement(self):
"""Removes quoted strings if database_id is SQL-session."""
@@ -625,7 +627,7 @@
del utility._oops_config.publishers[:]
report = {'type': 'NotFound',
'url': 'http://example.com',
- 'req_vars': [('HTTP_REFERER', 'example.com')]}
+ 'req_vars': {'HTTP_REFERER': 'example.com'}}
self.assertEqual(None, utility._oops_config.publish(report))
def test_onsite_404_not_ignored(self):
@@ -635,7 +637,7 @@
del utility._oops_config.publishers[:]
report = {'type': 'NotFound',
'url': 'http://example.com',
- 'req_vars': [('HTTP_REFERER', 'http://launchpad.dev/')]}
+ 'req_vars': {'HTTP_REFERER': 'http://launchpad.dev/'}}
self.assertNotEqual(None, utility._oops_config.publish(report))
def test_404_without_referer_is_ignored(self):
@@ -645,7 +647,7 @@
del utility._oops_config.publishers[:]
report = {'type': 'NotFound',
'url': 'http://example.com',
- 'req_vars': []}
+ 'req_vars': {}}
self.assertEqual(None, utility._oops_config.publish(report))
def test_ignored_report_filtered(self):
=== modified file 'lib/lp/services/job/tests/test_runner.py'
--- lib/lp/services/job/tests/test_runner.py 2011-10-31 01:17:17 +0000
+++ lib/lp/services/job/tests/test_runner.py 2011-11-16 06:39:24 +0000
@@ -192,8 +192,7 @@
self.assertEqual(JobStatus.COMPLETED, job_2.job.status)
oops = self.oopses[-1]
self.assertIn('Fake exception. Foobar, I say!', oops['tb_text'])
- self.assertEqual(1, len(oops['req_vars']))
- self.assertEqual("{'foo': 'bar'}", oops['req_vars'][0][1])
+ self.assertEqual(["{'foo': 'bar'}"], oops['req_vars'].values())
def test_oops_messages_used_when_handling(self):
"""Oops messages should appear even when exceptions are handled."""
@@ -209,8 +208,7 @@
runner = JobRunner([job_1, job_2])
runner.runAll()
oops = self.oopses[-1]
- self.assertEqual(1, len(oops['req_vars']))
- self.assertEqual("{'foo': 'bar'}", oops['req_vars'][0][1])
+ self.assertEqual(["{'foo': 'bar'}"], oops['req_vars'].values())
def test_runAll_aborts_transaction_on_error(self):
"""runAll should abort the transaction on oops."""
=== modified file 'lib/lp/services/profile/profile.py'
--- lib/lp/services/profile/profile.py 2011-10-03 14:41:27 +0000
+++ lib/lp/services/profile/profile.py 2011-11-16 06:39:24 +0000
@@ -368,11 +368,13 @@
trace = da.stop_sql_logging() or ()
# The trace is a list of dicts, each with the keys "sql" and "stack".
# "sql" is a tuple of start time, stop time, database name (with a
- # "SQL-" prefix), and sql statement. "stack" is None or a tuple of
- # filename, line number, function name, text, module name, optional
- # supplement dict, and optional info string. The supplement dict has
- # keys 'source_url', 'line', 'column', 'expression', 'warnings' (an
- # iterable), and 'extra', any of which may be None.
+ # "SQL-" prefix), sql statement and possibly a backtrace (backtraces
+ # are included when the timeline storm trace is installed). "stack" is
+ # None or a tuple of filename, line number, function name, text, module
+ # name, optional supplement dict, and optional info string. The
+ # supplement dict has keys 'source_url', 'line', 'column',
+ # 'expression', 'warnings' (an iterable), and 'extra', any of which may
+ # be None.
top_sql = []
top_python = []
triggers = {}
@@ -383,7 +385,12 @@
# Set up an identifier for each trace step.
step['id'] = ix
step['sql'] = dict(zip(
- ('start', 'stop', 'name', 'statement'), step['sql']))
+ ('start', 'stop', 'name', 'statement', 'backtrace'),
+ step['sql']))
+ # NB: step['sql']['backtrace'] is a string backtrace, not a live
+ # stack: less flexible than what the profiler gathers, but cheaper
+ # to keep alive as it holds no references. For now, its not usable
+ # here.
if step['stack'] is not None:
# Divide up the stack into the more unique (app) and less
# unique (db) bits.
=== modified file 'lib/lp/soyuz/tests/test_processaccepted.py'
--- lib/lp/soyuz/tests/test_processaccepted.py 2011-10-25 04:31:06 +0000
+++ lib/lp/soyuz/tests/test_processaccepted.py 2011-11-16 06:39:24 +0000
@@ -95,7 +95,7 @@
error_report = self.oopses[0]
expected_error = "Failure processing queue_item"
self.assertStartsWith(
- dict(error_report['req_vars'])['error-explanation'],
+ error_report['req_vars']['error-explanation'],
expected_error)
def test_accept_copy_archives(self):
=== modified file 'versions.cfg'
--- versions.cfg 2011-11-14 07:51:26 +0000
+++ versions.cfg 2011-11-16 06:39:24 +0000
@@ -50,12 +50,12 @@
mocker = 0.10.1
mozrunner = 1.3.4
oauth = 1.0
-oops = 0.0.9
+oops = 0.0.10
oops-amqp = 0.0.4
-oops-datedir-repo = 0.0.10
-oops-wsgi = 0.0.5
+oops-datedir-repo = 0.0.12
+oops-wsgi = 0.0.7
oops-timeline = 0.0.1
-oops-twisted = 0.0.4
+oops-twisted = 0.0.5
ordereddict = 1.1
paramiko = 1.7.4
Paste = 1.7.2
@@ -95,7 +95,7 @@
storm = 0.19.0.99-lpwithnodatetime-r402
testresources = 0.2.4-r58
testtools = 0.9.12-r228
-timeline = 0.0.1
+timeline = 0.0.3
transaction = 1.0.0
txAMQP = 0.5
txfixtures = 0.1.4
