launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04588
[Merge] lp:~lifeless/launchpad/useoops into lp:launchpad
Robert Collins has proposed merging lp:~lifeless/launchpad/useoops into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~lifeless/launchpad/useoops/+merge/71506
Delete a bunch of too-big test contents and narrow the tests to testing the thing they are really interested in; refactor oops creation into three parts: make, filter and send. Upgrade to python-oops 0.0.3 which supports missing keys better.
--
https://code.launchpad.net/~lifeless/launchpad/useoops/+merge/71506
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/useoops into lp:launchpad.
=== modified file 'configs/development/launchpad-lazr.conf'
--- configs/development/launchpad-lazr.conf 2011-07-22 15:35:08 +0000
+++ configs/development/launchpad-lazr.conf 2011-08-15 04:48:24 +0000
@@ -30,7 +30,6 @@
[bzr_lpserve]
error_dir: /var/tmp/codehosting.test
oops_prefix: BZR
-copy_to_zlog: false
[canonical]
show_tracebacks: True
@@ -53,7 +52,6 @@
secret_path: configs/development/codebrowse-secret
error_dir: /var/tmp/codebrowse.launchpad.dev/errors
oops_prefix: CB
-copy_to_zlog: false
[codehosting]
launch: True
@@ -70,7 +68,6 @@
port: tcp:5022:interface=127.0.0.88
error_dir: /var/tmp/codehosting.test
oops_prefix: SMPSSH
-copy_to_zlog: false
bzr_lp_prefix: lp://dev/
lp_url_hosts: dev
access_log: /var/tmp/bazaar.launchpad.dev/codehosting-access.log
@@ -88,7 +85,6 @@
[codeimportworker]
error_dir: /var/tmp/codehosting.test
oops_prefix: CIW
-copy_to_zlog: false
[commercial]
voucher_proxy_url: http://launchpad.dev
@@ -121,7 +117,6 @@
[error_reports]
oops_prefix: X
error_dir: /var/tmp/lperr
-copy_to_zlog: True
[google]
# Development and the testrunner should use the stub service be default.
=== modified file 'configs/testrunner/launchpad-lazr.conf'
--- configs/testrunner/launchpad-lazr.conf 2011-07-07 10:26:19 +0000
+++ configs/testrunner/launchpad-lazr.conf 2011-08-15 04:48:24 +0000
@@ -56,7 +56,6 @@
[error_reports]
oops_prefix: T
error_dir: /var/tmp/lperr.test
-copy_to_zlog: false
[gina_target.hoary]
architectures: i386
=== modified file 'lib/canonical/config/schema-lazr.conf'
--- lib/canonical/config/schema-lazr.conf 2011-07-27 20:14:40 +0000
+++ lib/canonical/config/schema-lazr.conf 2011-08-15 04:48:24 +0000
@@ -55,9 +55,6 @@
# See [error_reports].
oops_prefix: none
-# See [error_reports].
-copy_to_zlog: false
-
[builddmaster]
# The database user which will be used by this process.
@@ -178,8 +175,6 @@
oops_prefix: none
# See [error_reports].
error_dir: none
-# See [error_reports].
-copy_to_zlog: false
[calculate_bug_heat]
@@ -188,7 +183,6 @@
dbuser: calculate-bug-heat
oops_prefix: none
error_dir: none
-copy_to_zlog: false
max_heat_age: 7
@@ -225,9 +219,6 @@
# See [error_reports].
oops_prefix: none
-# See [error_reports].
-copy_to_zlog: false
-
# datatype: integer
batch_query_threshold: 10
@@ -297,7 +288,7 @@
# datatype: string
secret_path:
-# See [error_reports].
+# Ignored; delete when lp-prod-configs updated.
copy_to_zlog: False
# See [error_reports].
@@ -417,9 +408,6 @@
# datatype: string
port: tcp:5022
-# See [error_reports].
-copy_to_zlog: False
-
# Private mirror hosts
# A comma separated list of domain names that should not be shown
# to people not associated with the branch. Any branches with a mirror
@@ -527,9 +515,6 @@
max_jobs_per_machine: 3
# See [error_reports].
-copy_to_zlog: False
-
-# See [error_reports].
error_dir: /var/tmp/codehosting.test/
# See [error_reports].
@@ -557,9 +542,6 @@
working_directory_root: /var/tmp/codeimport/data
# See [error_reports].
-copy_to_zlog: False
-
-# See [error_reports].
error_dir: /var/tmp/codehosting.test/
# See [error_reports].
@@ -597,9 +579,6 @@
# See [error_reports].
oops_prefix: none
-# See [error_reports].
-copy_to_zlog: false
-
[packaging_translations]
# The database user which will be used by this process.
# datatype: string
@@ -612,9 +591,6 @@
# See [error_reports].
oops_prefix: none
-# See [error_reports].
-copy_to_zlog: false
-
[cveupdater]
# The database user which will be used by this process.
@@ -742,9 +718,6 @@
# See [error_reports].
oops_prefix: none
-# See [error_reports].
-copy_to_zlog: false
-
[error_reports]
# A prefix for "OOPS" codes for this process instance.
@@ -757,11 +730,6 @@
# datatype: string
error_dir: none
-# Should exceptions be sent to the zope log as well as being
-# saved to disk?
-# datatype: boolean
-copy_to_zlog: False
-
[expiredmembershipsflagger]
# The database user which will be used by this process.
@@ -988,8 +956,6 @@
# See [error_reports].
oops_prefix: none
-# See [error_reports].
-copy_to_zlog: false
[IPlainPackageCopyJobSource]
module: lp.soyuz.interfaces.packagecopyjob
@@ -1004,8 +970,6 @@
# See [error_reports].
oops_prefix: none
-# See [error_reports].
-copy_to_zlog: false
[karmacacheupdater]
# The database user which will be used by this process.
@@ -1342,8 +1306,7 @@
oops_prefix: none
# Must be set per librarian instance.
error_dir: none
-# Must be false: librarian has no zlog.
-copy_to_zlog: False
+
[librarian_gc]
# The database user which will be used by this process.
@@ -1452,9 +1415,6 @@
# See [error_reports].
oops_prefix: none
-# See [error_reports].
-copy_to_zlog: false
-
# Whether Mailman should be built if it is not already.
# datatype: boolean
build: false
@@ -1588,9 +1548,6 @@
# See [error_reports].
oops_prefix: none
-# See [error_reports].
-copy_to_zlog: false
-
##
## TODO: delete mpcreationjobs section after 10.04 rollout.
@@ -1608,9 +1565,6 @@
# See [error_reports].
oops_prefix: none
-# See [error_reports].
-copy_to_zlog: false
-
[person_notification]
# User for person notification db access
@@ -1667,8 +1621,6 @@
oops_prefix: none
# See [error_reports].
error_dir: none
-# See [error_reports].
-copy_to_zlog: false
[poppy]
@@ -1701,9 +1653,6 @@
# datatype: string
ftp_port: 2121
-# See [error_reports].
-copy_to_zlog: False
-
# The poppy access log location. Information such as connection, SSH
# login and session start times will be logged here.
access_log: /tmp/poppy-access.log
@@ -1730,7 +1679,6 @@
dbuser: process-apport-blobs
oops_prefix: APPORTBLOB
error_dir: none
-copy_to_zlog: false
[productreleasefinder]
@@ -1785,14 +1733,12 @@
error_dir: none
# See [error_reports].
oops_prefix: none
-# See [error_reports].
-copy_to_zlog: false
[request_daily_builds]
dbuser: request-daily-builds
error_dir: none
oops_prefix: none
-copy_to_zlog: false
+
[revisionkarma]
# The database user which will be used by this process.
@@ -1853,8 +1799,6 @@
# See [error_reports].
oops_prefix: none
-# See [error_reports].
-copy_to_zlog: false
[translations_export_to_branch]
# See [error_reports].
@@ -1863,9 +1807,6 @@
# See [error_reports].
oops_prefix: none
-# See [error_reports].
-copy_to_zlog: false
-
[sendbranchmail]
# The database user which will be used by this process.
@@ -1880,9 +1821,6 @@
# See [error_reports].
oops_prefix: none
-# See [error_reports].
-copy_to_zlog: false
-
# For the personal standing updater cron script.
[standingupdater]
@@ -1920,9 +1858,6 @@
# See [error_reports].
oops_prefix: none
-# See [error_reports].
-copy_to_zlog: false
-
[supermirror_import_puller]
# See [error_reports].
@@ -1931,9 +1866,6 @@
# See [error_reports].
oops_prefix: none
-# See [error_reports].
-copy_to_zlog: false
-
[supermirror_mirror_puller]
# See [error_reports].
@@ -1942,9 +1874,6 @@
# See [error_reports].
oops_prefix: none
-# See [error_reports].
-copy_to_zlog: false
-
[supermirror_upload_puller]
# See [error_reports].
@@ -1953,9 +1882,6 @@
# See [error_reports].
oops_prefix: none
-# See [error_reports].
-copy_to_zlog: false
-
[targetnamecacheupdater]
# The database user which will be used by this process.
@@ -2001,9 +1927,6 @@
# See [error_reports].
oops_prefix: none
-# See [error_reports].
-copy_to_zlog: false
-
[upgrade_branches]
dbuser: upgrade-branches
@@ -2014,9 +1937,6 @@
# See [error_reports].
oops_prefix: none
-# See [error_reports].
-copy_to_zlog: false
-
[uploader]
# The database user which will be used by this process.
=== modified file 'lib/canonical/launchpad/webapp/errorlog.py'
--- lib/canonical/launchpad/webapp/errorlog.py 2011-08-12 21:38:36 +0000
+++ lib/canonical/launchpad/webapp/errorlog.py 2011-08-15 04:48:24 +0000
@@ -189,7 +189,6 @@
"""
if section_name is None:
section_name = self._default_config_section
- self.copy_to_zlog = config[section_name].copy_to_zlog
# Start a new UniqueFileAllocator to activate the new configuration.
self.log_namer = UniqueFileAllocator(
output_root=config[section_name].error_dir,
@@ -288,44 +287,59 @@
def _raising(self, info, request=None, now=None, informational=False):
"""Private method used by raising() and handling()."""
- entry, filename = self._makeErrorReport(info, request, now, informational)
- if entry is None:
+ report = self._makeReport(info, request, now, informational)
+ if self._filterReport(report):
return
- oops.serializer_rfc822.write(entry, open(filename, 'wb'))
+ self._sendReport(report, now=now)
+ if request:
+ request.oopsid = report['id']
+ request.oops = report
+ return report
+
+ def _sendReport(self, report, now=None):
+ if now is not None:
+ now = now.astimezone(UTC)
+ else:
+ now = datetime.datetime.now(UTC)
+ oopsid, filename = self.log_namer.newId(now)
+ report['id'] = oopsid
+ oops.serializer_rfc822.write(report, open(filename, 'wb'))
# Set file permission to: rw-r--r--
wanted_permission = (
stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP | stat.S_IROTH)
os.chmod(filename, wanted_permission)
- if request:
- request.oopsid = entry['id']
- request.oops = entry
-
- if self.copy_to_zlog:
- self._do_copy_to_zlog(
- entry['time'], entry['type'], entry['url'], info, entry['id'])
- notify(ErrorReportEvent(entry))
- return entry
-
- def _isIgnoredException(self, strtype, request=None, exception=None):
- """Should the given exception generate an OOPS or be ignored?
-
- Exceptions will be ignored if they
- - are specially tagged as being ignorable by having the marker
- interface IUnloggedException
- - are of a type included in self._ignored_exceptions, or
- - were requested with an off-site REFERRER header and are of a
- type included in self._ignored_exceptions_for_offsite_referer
+ notify(ErrorReportEvent(report))
+
+ def filter_session_statement(self, database_id, statement):
+ """Replace quoted strings with '%s' in statements on session DB."""
+ if database_id == 'SQL-' + PGSessionBase.store_name:
+ return re.sub("'[^']*'", "'%s'", statement)
+ else:
+ return statement
+
+ def _filterReport(self, report):
+ """Return True if the report should be filtered and not emitted.
+
+ Reports are filtered if:
+ - There is a key 'ignore':True in the report. This is set during
+ _makeReport.
+ - have a type listed in self._ignored_exceptions.
+ - have a missing or offset REFERER header with a type listed in
+ self._ignored_exceptions_for_offsite_referer
"""
- if IUnloggedException.providedBy(exception):
- return True
- if strtype in self._ignored_exceptions:
- return True
- if strtype in self._ignored_exceptions_for_offsite_referer:
- if request is not None:
- referer = request.get('HTTP_REFERER')
- # If there is no referrer then we can't tell if this exception
- # should be ignored or not, so we'll be conservative and
- # ignore it.
+ if report.get('ignore'):
+ return True
+ if report['type'] in self._ignored_exceptions:
+ return True
+ if report['type'] 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', ()))
+ 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
+ # saved bookmark. Any which way, its not a sign of a current
+ # broken-url-generator in LP: ignore it.
if referer is None:
return True
referer_parts = urlparse.urlparse(referer)
@@ -335,16 +349,8 @@
return True
return False
- def filter_session_statement(self, database_id, statement):
- """Replace quoted strings with '%s' in statements on session DB."""
- if database_id == 'SQL-' + PGSessionBase.store_name:
- return re.sub("'[^']*'", "'%s'", statement)
- else:
- return statement
-
- def _makeErrorReport(self, info, request=None, now=None,
- informational=False):
- """Return an ErrorReport for the supplied data.
+ def _makeReport(self, info, request=None, now=None, informational=False):
+ """Create an unallocated OOPS.
:param info: Output of sys.exc_info()
:param request: The IErrorReportRequest which provides context to the
@@ -358,82 +364,35 @@
now = now.astimezone(UTC)
else:
now = datetime.datetime.now(UTC)
- tb_text = None
-
- strtype = str(getattr(info[0], '__name__', info[0]))
- if self._isIgnoredException(strtype, request, info[1]):
- return None, None
-
+ report = {}
+ report['type'] = _safestr(getattr(info[0], '__name__', info[0]))
+ report['value'] = _safestr(info[1])
if not isinstance(info[2], basestring):
tb_text = ''.join(format_exception(*info,
**{'as_html': False}))
else:
tb_text = info[2]
- tb_text = _safestr(tb_text)
-
- url = None
- username = None
- req_vars = []
- pageid = ''
-
+ report['tb_text'] = _safestr(tb_text)
+ report['req_vars'] = []
+ report['time'] = now
+ report['informational'] = informational
+ report['branch_nick'] = versioninfo.branch_nick
+ report['revno'] = versioninfo.revno
+ # Because of IUnloggedException being a sidewards lookup we must
+ # capture this here to filter on later.
+ report['ignore'] = IUnloggedException.providedBy(info[1])
if request:
- # XXX jamesh 2005-11-22: Temporary fix, which Steve should
- # undo. URL is just too HTTPRequest-specific.
- if safe_hasattr(request, 'URL'):
- url = request.URL
-
- if WebServiceLayer.providedBy(request):
- webservice_error = getattr(
- info[1], '__lazr_webservice_error__', 500)
- if webservice_error / 100 != 5:
- request.oopsid = None
- # Return so the OOPS is not generated.
- return None, None
-
- missing = object()
- principal = getattr(request, 'principal', missing)
- if safe_hasattr(principal, 'getLogin'):
- login = principal.getLogin()
- elif principal is missing or principal is None:
- # Request has no principal.
- login = None
- else:
- # Request has an UnauthenticatedPrincipal.
- login = 'unauthenticated'
- if strtype in (
- self._ignored_exceptions_for_unauthenticated_users):
- return None, None
-
- if principal is not None and principal is not missing:
- username = _safestr(
- ', '.join([
- unicode(login),
- unicode(request.principal.id),
- unicode(request.principal.title),
- unicode(request.principal.description)]))
-
- if getattr(request, '_orig_env', None):
- pageid = request._orig_env.get('launchpad.pageid', '')
-
- for key, value in request.items():
- if _is_sensitive(request, key):
- req_vars.append((_safestr(key), '<hidden>'))
- else:
- req_vars.append((_safestr(key), _safestr(value)))
- if IXMLRPCRequest.providedBy(request):
- args = request.getPositionalArguments()
- req_vars.append(('xmlrpc args', _safestr(args)))
+ self._gather_request(report, request, info)
# XXX AaronBentley 2009-11-26 bug=488950: There should be separate
# storage for oops messages.
- req_vars.extend(
+ report['req_vars'].extend(
('<oops-message-%d>' % key, str(message)) for key, message
in self._oops_messages.iteritems())
- req_vars.sort()
- strv = _safestr(info[1])
-
- strurl = _safestr(url)
-
- duration = get_request_duration()
+ report['req_vars'].sort()
+
+ # More generic than HTTP requests - e.g. how long a script was running
+ # for.
+ report['duration'] = get_request_duration()
# In principle the timeline is per-request, but see bug=623199 -
# at this point the request is optional, but get_request_timeline
# does not care; when it starts caring, we will always have a
@@ -446,26 +405,60 @@
detail = self.filter_session_statement(category, detail)
statements.append(
(start, end, _safestr(category), _safestr(detail)))
-
- oopsid, filename = self.log_namer.newId(now)
-
- result = {
- 'id': oopsid,
- 'type': strtype,
- 'value': strv,
- 'time': now,
- 'pageid': pageid,
- 'tb_text': tb_text,
- 'username': username,
- 'url': strurl,
- 'duration': duration,
- 'req_vars': req_vars,
- 'db_statements': statements,
- 'informational': informational,
- 'branch_nick': versioninfo.branch_nick,
- 'revno': versioninfo.revno,
- }
- return result, filename
+ report['db_statements'] = statements
+ return report
+
+ def _gather_request(self, report, request, info):
+ """Add request metadata into the error report."""
+ # XXX jamesh 2005-11-22: Temporary fix, which Steve should
+ # undo. URL is just too HTTPRequest-specific.
+ if safe_hasattr(request, 'URL'):
+ report['url'] = _safestr(request.URL)
+
+ if WebServiceLayer.providedBy(request):
+ webservice_error = getattr(
+ info[1], '__lazr_webservice_error__', 500)
+ if webservice_error / 100 != 5:
+ request.oopsid = None
+ # Tell the oops machinery to ignore this error
+ report['ignore'] = True
+
+ missing = object()
+ principal = getattr(request, 'principal', missing)
+ if safe_hasattr(principal, 'getLogin'):
+ login = principal.getLogin()
+ elif principal is missing or principal is None:
+ # Request has no principal (e.g. scriptrequest)
+ login = None
+ else:
+ # Request has an UnauthenticatedPrincipal.
+ login = 'unauthenticated'
+ if report['type'] in (
+ self._ignored_exceptions_for_unauthenticated_users):
+ report['ignore'] = True
+
+ if principal is not None and principal is not missing:
+ username = _safestr(
+ ', '.join([
+ unicode(login),
+ unicode(request.principal.id),
+ unicode(request.principal.title),
+ unicode(request.principal.description)]))
+ report['username'] = username
+
+ if getattr(request, '_orig_env', None):
+ report['pageid'] = request._orig_env.get(
+ 'launchpad.pageid', '')
+
+ for key, value in request.items():
+ if _is_sensitive(request, key):
+ report['req_vars'].append((_safestr(key), '<hidden>'))
+ else:
+ report['req_vars'].append(
+ (_safestr(key), _safestr(value)))
+ if IXMLRPCRequest.providedBy(request):
+ args = request.getPositionalArguments()
+ report['req_vars'].append(('xmlrpc args', _safestr(args)))
def handling(self, info, request=None, now=None):
"""Flag ErrorReport as informational only.
@@ -480,29 +473,6 @@
return self._raising(
info, request=request, now=now, informational=True)
- def _do_copy_to_zlog(self, now, strtype, url, info, oopsid):
- distant_past = datetime.datetime(1970, 1, 1, 0, 0, 0, tzinfo=UTC)
- when = _rate_restrict_pool.get(strtype, distant_past)
- if now > when:
- next_when = max(
- when, now - _rate_restrict_burst * _rate_restrict_period)
- next_when += _rate_restrict_period
- _rate_restrict_pool[strtype] = next_when
- # Sometimes traceback information can be passed in as a string. In
- # those cases, we don't (can't!) log the traceback. The traceback
- # information is still preserved in the actual OOPS report.
- traceback = info[2]
- if not isinstance(traceback, types.TracebackType):
- traceback = None
- # The logging module doesn't provide a way to pass in exception
- # info, so we temporarily raise the exception so it can be logged.
- # We disable the pylint warning for the blank except.
- try:
- raise info[0], info[1], traceback
- except info[0]:
- logging.getLogger('SiteError').exception(
- '%s (%s)' % (url, oopsid))
-
@contextlib.contextmanager
def oopsMessage(self, message):
"""Add an oops message to be included in oopses from this context."""
=== modified file 'lib/canonical/launchpad/webapp/tests/test_errorlog.py'
--- lib/canonical/launchpad/webapp/tests/test_errorlog.py 2011-08-13 04:07:10 +0000
+++ lib/canonical/launchpad/webapp/tests/test_errorlog.py 2011-08-15 04:48:24 +0000
@@ -30,7 +30,6 @@
from zope.publisher.interfaces import NotFound
from zope.publisher.interfaces.xmlrpc import IXMLRPCRequest
from zope.security.interfaces import Unauthorized
-from zope.testing.loggingsupport import InstalledHandler
from canonical.config import config
from lp.app import versioninfo
@@ -161,7 +160,6 @@
# current error directory.
test_data = dedent("""
[error_reports]
- copy_to_zlog: true
error_dir: %s
""" % tempfile.mkdtemp())
config.push('test_data', test_data)
@@ -186,8 +184,6 @@
utility.oops_prefix)
self.assertEqual(config.error_reports.error_dir,
utility.log_namer._output_root)
- self.assertEqual(
- config.error_reports.copy_to_zlog, utility.copy_to_zlog)
# Some external processes may use another config section to
# provide the error log configuration.
utility.configure(section_name='branchscanner')
@@ -195,8 +191,6 @@
utility.oops_prefix)
self.assertEqual(config.branchscanner.error_dir,
utility.log_namer._output_root)
- self.assertEqual(
- config.branchscanner.copy_to_zlog, utility.copy_to_zlog)
# The default error section can be restored.
utility.configure()
@@ -204,8 +198,6 @@
utility.oops_prefix)
self.assertEqual(config.error_reports.error_dir,
utility.log_namer._output_root)
- self.assertEqual(
- config.error_reports.copy_to_zlog, utility.copy_to_zlog)
def test_setOopsToken(self):
"""Test ErrorReportingUtility.setOopsToken()."""
@@ -217,19 +209,18 @@
utility.setOopsToken('1')
self.assertEqual('T1', utility.oops_prefix)
- def test_raising(self):
+ def test_raising_permissions(self):
"""Test ErrorReportingUtility.raising() with no request"""
utility = ErrorReportingUtility()
+ report = {'id': 'OOPS-91T1'}
now = datetime.datetime(2006, 04, 01, 00, 30, 00, tzinfo=UTC)
- # Set up default file creation mode to rwx------.
+ # Set up default file creation mode to rwx------ as some restrictive
+ # servers do.
umask_permission = stat.S_IRWXG | stat.S_IRWXO
old_umask = os.umask(umask_permission)
-
- try:
- raise ArbitraryException('xyz')
- except ArbitraryException:
- utility.raising(sys.exc_info(), now=now)
+ self.addCleanup(os.umask, old_umask)
+ utility._sendReport(report, now)
errorfile = os.path.join(
utility.log_namer.output_dir(now), '01800.T1')
@@ -242,36 +233,6 @@
# Get only the permission bits for this file.
file_permission = stat.S_IMODE(st.st_mode)
self.assertEqual(file_permission, wanted_permission)
- # Restore the umask to the original value.
- os.umask(old_umask)
-
- lines = open(errorfile, 'r').readlines()
-
- # the header
- self.assertEqual(lines[0], 'Oops-Id: OOPS-91T1\n')
- self.assertEqual(lines[1], 'Exception-Type: ArbitraryException\n')
- self.assertEqual(lines[2], 'Exception-Value: xyz\n')
- self.assertEqual(lines[3], 'Date: 2006-04-01T00:30:00+00:00\n')
- self.assertEqual(lines[4], 'Page-Id: \n')
- self.assertEqual(lines[5], 'Branch: %s\n' % versioninfo.branch_nick)
- self.assertEqual(lines[6], 'Revision: %s\n' % versioninfo.revno)
- self.assertEqual(lines[7], 'User: None\n')
- self.assertEqual(lines[8], 'URL: None\n')
- self.assertEqual(lines[9], 'Duration: -1\n')
- self.assertEqual(lines[10], 'Informational: False\n')
- self.assertEqual(lines[11], '\n')
-
- # no request vars
- self.assertEqual(lines[12], '\n')
-
- # no database statements
- self.assertEqual(lines[13], '\n')
-
- # traceback
- self.assertEqual(lines[14], 'Traceback (most recent call last):\n')
- # Module canonical.launchpad.webapp.ftests.test_errorlog, ...
- # raise ArbitraryException(\'xyz\')
- self.assertEqual(lines[17], 'ArbitraryException: xyz\n')
def test_raising_with_request(self):
"""Test ErrorReportingUtility.raising() with a request"""
@@ -354,16 +315,11 @@
directlyProvides(request, IXMLRPCRequest)
request.getPositionalArguments = lambda: (1, 2)
utility = ErrorReportingUtility()
- now = datetime.datetime(2006, 04, 01, 00, 30, 00, tzinfo=UTC)
try:
raise ArbitraryException('xyz\nabc')
except ArbitraryException:
- utility.raising(sys.exc_info(), request, now=now)
- errorfile = os.path.join(
- utility.log_namer.output_dir(now), '01800.T1')
- self.assertTrue(os.path.exists(errorfile))
- lines = open(errorfile, 'r').readlines()
- self.assertEqual(lines[16], 'xmlrpc args=(1, 2)\n')
+ report = utility.raising(sys.exc_info(), request)
+ self.assertEqual(('xmlrpc args', '(1, 2)'), report['req_vars'][-1])
def test_raising_with_webservice_request(self):
# Test ErrorReportingUtility.raising() with a WebServiceRequest
@@ -405,120 +361,47 @@
def test_raising_for_script(self):
"""Test ErrorReportingUtility.raising with a ScriptRequest."""
utility = ErrorReportingUtility()
- now = datetime.datetime(2006, 04, 01, 00, 30, 00, tzinfo=UTC)
+ req_vars = [
+ ('name2', 'value2'), ('name1', 'value1'),
+ ('name1', 'value3')]
+ url='https://launchpad.net/example'
try:
raise ArbitraryException('xyz\nabc')
except ArbitraryException:
# Do not test escaping of request vars here, it is already tested
# in test_raising_with_request.
- request = ScriptRequest([
- ('name2', 'value2'), ('name1', 'value1'),
- ('name1', 'value3')], URL='https://launchpad.net/example')
- utility.raising(sys.exc_info(), request, now=now)
-
- errorfile = os.path.join(
- utility.log_namer.output_dir(now), '01800.T1')
- self.assertTrue(os.path.exists(errorfile))
- lines = open(errorfile, 'r').readlines()
-
- # the header
- self.assertEqual(lines[0], 'Oops-Id: OOPS-91T1\n')
- self.assertEqual(lines[1], 'Exception-Type: ArbitraryException\n')
- self.assertEqual(lines[2], 'Exception-Value: xyz abc\n')
- self.assertEqual(lines[3], 'Date: 2006-04-01T00:30:00+00:00\n')
- self.assertEqual(lines[4], 'Page-Id: \n')
- self.assertEqual(lines[5], 'Branch: %s\n' % versioninfo.branch_nick)
- self.assertEqual(lines[6], 'Revision: %s\n' % versioninfo.revno)
- self.assertEqual(lines[7], 'User: None\n')
- self.assertEqual(lines[8], 'URL: https://launchpad.net/example\n')
- self.assertEqual(lines[9], 'Duration: -1\n')
- self.assertEqual(lines[10], 'Informational: False\n')
- self.assertEqual(lines[11], '\n')
-
- # request vars
- self.assertEqual(lines[12], 'name1=value1\n')
- self.assertEqual(lines[13], 'name1=value3\n')
- self.assertEqual(lines[14], 'name2=value2\n')
- self.assertEqual(lines[15], '\n')
-
- # no database statements
- self.assertEqual(lines[16], '\n')
-
- # traceback
- self.assertEqual(lines[17], 'Traceback (most recent call last):\n')
- # Module canonical.launchpad.webapp.ftests.test_errorlog, ...
- # raise ArbitraryException(\'xyz\')
- self.assertEqual(lines[20], 'ArbitraryException: xyz\n')
-
- # verify that the oopsid was set on the request
- self.assertEqual(request.oopsid, 'OOPS-91T1')
+ request = ScriptRequest(req_vars, URL=url)
+ report = utility.raising(sys.exc_info(), request)
+
+ self.assertEqual(url, report['url'])
+ self.assertEqual(sorted(req_vars), report['req_vars'])
def test_raising_with_unprintable_exception(self):
- # Test ErrorReportingUtility.raising() with an unprintable exception.
- utility = ErrorReportingUtility()
- now = datetime.datetime(2006, 01, 01, 00, 30, 00, tzinfo=UTC)
-
class UnprintableException(Exception):
-
def __str__(self):
raise RuntimeError('arrgh')
__repr__ = __str__
- log = InstalledHandler('SiteError')
+ utility = ErrorReportingUtility()
try:
raise UnprintableException()
except UnprintableException:
- utility.raising(sys.exc_info(), now=now)
- log.uninstall()
-
- errorfile = os.path.join(
- utility.log_namer.output_dir(now), '01800.T1')
- self.assertTrue(os.path.exists(errorfile))
- lines = open(errorfile, 'r').readlines()
-
- # the header
- self.assertEqual(lines[0], 'Oops-Id: OOPS-1T1\n')
- self.assertEqual(lines[1], 'Exception-Type: UnprintableException\n')
- self.assertEqual(
- lines[2],
- 'Exception-Value: <unprintable UnprintableException object>\n')
- self.assertEqual(lines[3], 'Date: 2006-01-01T00:30:00+00:00\n')
- self.assertEqual(lines[4], 'Page-Id: \n')
- self.assertEqual(lines[5], 'Branch: %s\n' % versioninfo.branch_nick)
- self.assertEqual(lines[6], 'Revision: %s\n' % versioninfo.revno)
- self.assertEqual(lines[7], 'User: None\n')
- self.assertEqual(lines[8], 'URL: None\n')
- self.assertEqual(lines[9], 'Duration: -1\n')
- self.assertEqual(lines[10], 'Informational: False\n')
- self.assertEqual(lines[11], '\n')
-
- # no request vars
- self.assertEqual(lines[12], '\n')
-
- # no database statements
- self.assertEqual(lines[13], '\n')
-
- # traceback
- self.assertEqual(lines[14], 'Traceback (most recent call last):\n')
- # Module canonical.launchpad.webapp.ftests.test_errorlog, ...
- # raise UnprintableException()
- self.assertEqual(
- lines[17],
- 'UnprintableException:'
- ' <unprintable UnprintableException object>\n')
+ report = utility.raising(sys.exc_info())
+
+ unprintable = '<unprintable UnprintableException object>'
+ self.assertEqual(unprintable, report['value'])
+ self.assertIn( 'UnprintableException: ' + unprintable,
+ report['tb_text'])
def test_raising_unauthorized_without_request(self):
"""Unauthorized exceptions are logged when there's no request."""
utility = ErrorReportingUtility()
- now = datetime.datetime(2006, 04, 01, 00, 30, 00, tzinfo=UTC)
try:
raise Unauthorized('xyz')
except Unauthorized:
- utility.raising(sys.exc_info(), now=now)
- errorfile = os.path.join(
- utility.log_namer.output_dir(now), '01800.T1')
- self.failUnless(os.path.exists(errorfile))
+ oops = utility.raising(sys.exc_info())
+ self.assertNotEqual(None, oops)
def test_raising_unauthorized_without_principal(self):
"""Unauthorized exceptions are logged when the request has no
@@ -681,9 +564,8 @@
def test_raising_with_string_as_traceback(self):
# ErrorReportingUtility.raising() can be called with a string in the
# place of a traceback. This is useful when the original traceback
- # object is unavailable.
- utility = ErrorReportingUtility()
- now = datetime.datetime(2006, 04, 01, 00, 30, 00, tzinfo=UTC)
+ # object is unavailable - e.g. when logging a failure reported by a
+ # non-oops-enabled service.
try:
raise RuntimeError('hello')
@@ -694,76 +576,21 @@
# one generated by format_exc is sometimes passed instead.
exc_tb = traceback.format_exc()
- utility.raising((exc_type, exc_value, exc_tb), now=now)
- errorfile = os.path.join(
- utility.log_namer.output_dir(now), '01800.T1')
-
- self.assertTrue(os.path.exists(errorfile))
- lines = open(errorfile, 'r').readlines()
-
- # the header
- self.assertEqual(lines[0], 'Oops-Id: OOPS-91T1\n')
- self.assertEqual(lines[1], 'Exception-Type: RuntimeError\n')
- self.assertEqual(lines[2], 'Exception-Value: hello\n')
- self.assertEqual(lines[3], 'Date: 2006-04-01T00:30:00+00:00\n')
- self.assertEqual(lines[4], 'Page-Id: \n')
- self.assertEqual(lines[5], 'Branch: %s\n' % versioninfo.branch_nick)
- self.assertEqual(lines[6], 'Revision: %s\n' % versioninfo.revno)
- self.assertEqual(lines[7], 'User: None\n')
- self.assertEqual(lines[8], 'URL: None\n')
- self.assertEqual(lines[9], 'Duration: -1\n')
- self.assertEqual(lines[10], 'Informational: False\n')
- self.assertEqual(lines[11], '\n')
-
- # no request vars
- self.assertEqual(lines[12], '\n')
-
- # no database statements
- self.assertEqual(lines[13], '\n')
-
- # traceback
- self.assertEqual(''.join(lines[14:18]), exc_tb)
+ utility = ErrorReportingUtility()
+ report = utility.raising((exc_type, exc_value, exc_tb))
+ # traceback is what we supplied.
+ self.assertEqual(exc_tb, report['tb_text'])
def test_handling(self):
"""Test ErrorReportingUtility.handling()."""
utility = ErrorReportingUtility()
- now = datetime.datetime(2006, 04, 01, 00, 30, 00, tzinfo=UTC)
try:
raise ArbitraryException('xyz')
except ArbitraryException:
- utility.handling(sys.exc_info(), now=now)
-
- errorfile = os.path.join(
- utility.log_namer.output_dir(now), '01800.T1')
- self.assertTrue(os.path.exists(errorfile))
- lines = open(errorfile, 'r').readlines()
-
- # the header
- self.assertEqual(lines[0], 'Oops-Id: OOPS-91T1\n')
- self.assertEqual(lines[1], 'Exception-Type: ArbitraryException\n')
- self.assertEqual(lines[2], 'Exception-Value: xyz\n')
- self.assertEqual(lines[3], 'Date: 2006-04-01T00:30:00+00:00\n')
- self.assertEqual(lines[4], 'Page-Id: \n')
- self.assertEqual(lines[5], 'Branch: %s\n' % versioninfo.branch_nick)
- self.assertEqual(lines[6], 'Revision: %s\n' % versioninfo.revno)
- self.assertEqual(lines[7], 'User: None\n')
- self.assertEqual(lines[8], 'URL: None\n')
- self.assertEqual(lines[9], 'Duration: -1\n')
- self.assertEqual(lines[10], 'Informational: True\n')
- self.assertEqual(lines[11], '\n')
-
- # no request vars
- self.assertEqual(lines[12], '\n')
-
- # no database statements
- self.assertEqual(lines[13], '\n')
-
- # traceback
- self.assertEqual(lines[14], 'Traceback (most recent call last):\n')
- # Module canonical.launchpad.webapp.ftests.test_errorlog, ...
- # raise ArbitraryException(\'xyz\')
- self.assertEqual(lines[17], 'ArbitraryException: xyz\n')
+ report = utility.handling(sys.exc_info())
+
+ self.assertEqual(report['informational'], True)
def test_oopsMessage(self):
"""oopsMessage pushes and pops the messages."""
@@ -790,7 +617,7 @@
raise ArbitraryException('foo')
except ArbitraryException:
info = sys.exc_info()
- oops, _ = utility._makeErrorReport(info)
+ oops = utility._makeReport(info)
self.assertEqual(
[('<oops-message-0>', "{'a': 'b', 'c': 'd'}")],
oops['req_vars'])
@@ -804,7 +631,7 @@
raise ArbitraryException('foo')
except ArbitraryException:
info = sys.exc_info()
- oops, _ = utility._makeErrorReport(info, request)
+ oops = utility._makeReport(info, request)
self.assertEqual(
[('<oops-message-0>', "{'a': 'b'}"), ('c', 'd')],
oops['req_vars'])
@@ -837,7 +664,7 @@
raise ArbitraryException('foo')
except ArbitraryException:
info = sys.exc_info()
- oops, _ = utility._makeErrorReport(info)
+ oops = utility._makeReport(info)
self.assertEqual("SELECT '%s'", oops['db_statements'][0][3])
finally:
clear_request_started()
@@ -897,9 +724,9 @@
self.assertTrue(
report.tb_text.startswith('Traceback (most recent call last):\n'),
report.tb_text)
- self.assertEqual('', report.pageid)
- self.assertEqual('None', report.username)
- self.assertEqual('None', report.url)
+ self.assertEqual(None, report.pageid)
+ self.assertEqual(None, report.username)
+ self.assertEqual(None, report.url)
self.assertEqual([], report.req_vars)
self.assertEqual([], report.db_statements)
@@ -943,37 +770,45 @@
# A request originating from another site that generates a NotFound
# (404) is ignored (i.e., no OOPS is logged).
utility = ErrorReportingUtility()
- request = dict(HTTP_REFERER='example.com')
- self.assertTrue(utility._isIgnoredException('NotFound', request))
+ report = {'type': 'NotFound',
+ 'url': 'http://example.com',
+ 'req_vars': [('HTTP_REFERER', 'example.com')]}
+ self.assertTrue(utility._filterReport(report))
def test_onsite_404_not_ignored(self):
# A request originating from a local site that generates a NotFound
# (404) produces an OOPS.
utility = ErrorReportingUtility()
- request = dict(HTTP_REFERER='canonical.com')
- self.assertTrue(utility._isIgnoredException('NotFound', request))
+ report = {'type': 'NotFound',
+ 'url': 'http://example.com',
+ 'req_vars': [('HTTP_REFERER', 'http://launchpad.dev/')]}
+ self.assertFalse(utility._filterReport(report))
def test_404_without_referer_is_ignored(self):
# If a 404 is generated and there is no HTTP referer, we don't produce
# an OOPS.
utility = ErrorReportingUtility()
- request = dict()
- self.assertTrue(utility._isIgnoredException('NotFound', request))
+ report = {'type': 'NotFound',
+ 'url': 'http://example.com',
+ 'req_vars': []}
+ self.assertTrue(utility._filterReport(report))
+
+ def test_ignored_report_filtered(self):
+ utility = ErrorReportingUtility()
+ report = {'ignore': True}
+ self.assertTrue(utility._filterReport(report))
def test_marked_exception_is_ignored(self):
- # If an exception has been marked as ignorable, then it is ignored.
- utility = ErrorReportingUtility()
- exception = Exception()
- directlyProvides(exception, IUnloggedException)
- self.assertTrue(
- utility._isIgnoredException('RuntimeError', exception=exception))
-
- def test_unmarked_exception_generates_oops(self):
- # If an exception has not been marked as ignorable, then it is not.
- utility = ErrorReportingUtility()
- exception = Exception()
- self.assertFalse(
- utility._isIgnoredException('RuntimeError', exception=exception))
+ # If an exception has been marked as ignorable, then it is ignored in
+ # the report.
+ utility = ErrorReportingUtility()
+ try:
+ raise ArbitraryException('xyz\nabc')
+ except ArbitraryException:
+ exc_info = sys.exc_info()
+ directlyProvides(exc_info[1], IUnloggedException)
+ report = utility._makeReport(exc_info)
+ self.assertTrue(report['ignore'])
class TestWrappedParameterConverter(testtools.TestCase):
=== modified file 'lib/lp/services/job/runner.py'
--- lib/lp/services/job/runner.py 2011-08-12 21:38:36 +0000
+++ lib/lp/services/job/runner.py 2011-08-15 04:48:24 +0000
@@ -612,9 +612,8 @@
def main(self):
section = self.config_section
if (getattr(section, 'error_dir', None) is not None
- and getattr(section, 'oops_prefix', None) is not None
- and getattr(section, 'copy_to_zlog', None) is not None):
- # If the three variables are not set, we will let the error
+ and getattr(section, 'oops_prefix', None) is not None):
+ # If the two variables are not set, we will let the error
# utility default to using the [error_reports] config.
errorlog.globalErrorUtility.configure(self.config_name)
job_source = getUtility(self.source_interface)
=== modified file 'lib/lp/services/scripts/base.py'
--- lib/lp/services/scripts/base.py 2011-08-10 09:23:35 +0000
+++ lib/lp/services/scripts/base.py 2011-08-15 04:48:24 +0000
@@ -395,9 +395,6 @@
# Scripts can override this if they want.
globalErrorUtility.configure()
- # Scripts do not have a zlog.
- globalErrorUtility.copy_to_zlog = False
-
# WARN and higher log messages should generate OOPS reports.
# self.name is used instead of the name argument, since it may have
# have been overridden by command-line parameters or by
=== modified file 'lib/lp/services/twistedsupport/tests/test_loggingsupport.py'
--- lib/lp/services/twistedsupport/tests/test_loggingsupport.py 2011-08-11 03:15:16 +0000
+++ lib/lp/services/twistedsupport/tests/test_loggingsupport.py 2011-08-15 04:48:24 +0000
@@ -51,7 +51,6 @@
[error_reports]
oops_prefix: O
error_dir: %s
- copy_to_zlog: False
""" % self.temp_dir))
globalErrorUtility.configure()
self.log_stream = StringIO.StringIO()
=== modified file 'versions.cfg'
--- versions.cfg 2011-08-12 06:40:54 +0000
+++ versions.cfg 2011-08-15 04:48:24 +0000
@@ -49,7 +49,7 @@
mocker = 0.10.1
mozrunner = 1.3.4
oauth = 1.0
-oops = 0.0.2
+oops = 0.0.3
paramiko = 1.7.4
Paste = 1.7.2
PasteDeploy = 1.3.3
Follow ups
