launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #21814
[Merge] lp:~cjwatson/launchpad/upgrade-oops into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/upgrade-oops into lp:launchpad.
Commit message:
Upgrade to oops 0.0.13 and compatible versions of related packages.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/upgrade-oops/+merge/330118
This depends on https://code.launchpad.net/~cjwatson/python-oops-twisted/new-oops-publisher-api/+merge/330117.
Aside from it probably being a good idea to catch up a bit, the main benefit of this is that we can get rid of pymongo from our dependency graph. That was an obstruction to converting to pip, because pymongo installs bson as a top-level module and thus conflicts with the bson package. We got away with that with buildout because each package was installed as a separate egg, but with pip they all end up in the same site-packages directory so we can no longer ignore the problem.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/upgrade-oops into lp:launchpad.
=== modified file 'lib/lp/codehosting/codeimport/tests/test_workermonitor.py'
--- lib/lp/codehosting/codeimport/tests/test_workermonitor.py 2017-05-12 08:08:26 +0000
+++ lib/lp/codehosting/codeimport/tests/test_workermonitor.py 2017-09-02 13:43:11 +0000
@@ -582,7 +582,8 @@
# loggingsupport.set_up_oops_reporting).
errorlog.globalErrorUtility.configure(
config_factory=oops_twisted.Config,
- publisher_adapter=oops_twisted.defer_publisher)
+ publisher_adapter=oops_twisted.defer_publisher,
+ publisher_helpers=oops_twisted.publishers)
self.addCleanup(errorlog.globalErrorUtility.configure)
worker_monitor = self.WorkerMonitor(defer.fail(RuntimeError()))
return worker_monitor.run().addCallback(
@@ -611,7 +612,8 @@
# callFinishJob logs a failure from the child process.
errorlog.globalErrorUtility.configure(
config_factory=oops_twisted.Config,
- publisher_adapter=oops_twisted.defer_publisher)
+ publisher_adapter=oops_twisted.defer_publisher,
+ publisher_helpers=oops_twisted.publishers)
self.addCleanup(errorlog.globalErrorUtility.configure)
failure_msg = "test_callFinishJob_logs_failure expected failure"
worker_monitor = self.WorkerMonitor(
=== modified file 'lib/lp/services/scripts/tests/cronscript-crash.py'
--- lib/lp/services/scripts/tests/cronscript-crash.py 2012-01-01 02:58:52 +0000
+++ lib/lp/services/scripts/tests/cronscript-crash.py 2017-09-02 13:43:11 +0000
@@ -1,5 +1,5 @@
#!/usr/bin/python -S
-# Copyright 2010 Canonical Ltd. This software is licensed under the
+# Copyright 2010-2017 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Cronscript that raises an unhandled exception."""
@@ -17,7 +17,12 @@
def main(self):
self.oopses = []
- globalErrorUtility._oops_config.publishers[:] = [self.oopses.append]
+
+ def publish(report):
+ self.oopses.append(report)
+ return []
+
+ globalErrorUtility._oops_config.publisher = publish
self.logger.debug("This is debug level")
# Debug messages do not generate an OOPS.
=== modified file 'lib/lp/services/twistedsupport/loggingsupport.py'
--- lib/lp/services/twistedsupport/loggingsupport.py 2015-07-08 16:05:11 +0000
+++ lib/lp/services/twistedsupport/loggingsupport.py 2017-09-02 13:43:11 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Integration between the normal Launchpad logging and Twisted's."""
@@ -55,7 +55,8 @@
errorlog.globalErrorUtility.configure(
configuration,
config_factory=oops_twisted.Config,
- publisher_adapter=oops_twisted.defer_publisher)
+ publisher_adapter=oops_twisted.defer_publisher,
+ publisher_helpers=oops_twisted.publishers)
log_observer = RotatableFileLogObserver(logfile)
oops_observer = OOPSObserver(errorlog.globalErrorUtility._oops_config,
log_observer)
=== modified file 'lib/lp/services/webapp/errorlog.py'
--- lib/lp/services/webapp/errorlog.py 2016-09-14 11:55:45 +0000
+++ lib/lp/services/webapp/errorlog.py 2017-09-02 13:43:11 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Error logging facilities."""
@@ -89,7 +89,7 @@
if not report.get('id'):
report['id'] = str(id(report))
notify(ErrorReportEvent(report))
- return report['id']
+ return [report['id']]
def attach_adapter_duration(report, context):
@@ -268,7 +268,7 @@
index for index, _ignored in enumerate(repeat(None)))
def configure(self, section_name=None, config_factory=oops.Config,
- publisher_adapter=None):
+ publisher_adapter=None, publisher_helpers=oops.publishers):
"""Configure the utility using the named section from the config.
The 'error_reports' section is used if section_name is None.
@@ -312,26 +312,37 @@
# And any active feature flags.
self._oops_config.on_create.append(attach_feature_info)
- def add_publisher(publisher):
+ def adapted_publisher(publisher):
if publisher_adapter is not None:
- publisher = publisher_adapter(publisher)
- self._oops_config.publishers.append(publisher)
+ return publisher_adapter(publisher)
+ else:
+ return publisher
+ # Exposed for introspection by tests. Don't expect modifying these
+ # to be useful.
+ self._main_publishers = []
+ self._all_publishers = []
# If amqp is configured we want to publish over amqp.
if (config.error_reports.error_exchange and rabbit.is_configured()):
exchange = config.error_reports.error_exchange
routing_key = config.error_reports.error_queue_key
- amqp_publisher = oops_amqp.Publisher(
- rabbit.connect, exchange, routing_key)
- add_publisher(amqp_publisher)
+ # Exposed for tests.
+ self._main_publishers.append(adapted_publisher(oops_amqp.Publisher(
+ rabbit.connect, exchange, routing_key)))
# We want to publish reports to disk for gathering to the central
# analysis server, but only if we haven't already published to rabbit.
self._oops_datedir_repo = DateDirRepo(
config[self._default_config_section].error_dir)
- add_publisher(oops.publish_new_only(self._oops_datedir_repo.publish))
+ self._main_publishers.append(
+ adapted_publisher(self._oops_datedir_repo.publish))
+ self._all_publishers.append(
+ publisher_helpers.publish_with_fallback(*self._main_publishers))
# And send everything within the zope application server (only for
# testing).
- add_publisher(notify_publisher)
+ self._all_publishers.append(adapted_publisher(notify_publisher))
+ self._oops_config.publisher = publisher_helpers.publish_to_many(
+ *self._all_publishers)
+
#
# Reports are filtered if:
# - There is a key 'ignore':True in the report. This is set during
=== modified file 'lib/lp/services/webapp/tests/test_errorlog.py'
--- lib/lp/services/webapp/tests/test_errorlog.py 2016-09-14 11:12:13 +0000
+++ lib/lp/services/webapp/tests/test_errorlog.py 2017-09-02 13:43:11 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Tests for error logging & OOPS reporting."""
@@ -92,22 +92,21 @@
self.assertEqual(config.error_reports.oops_prefix,
utility.oops_prefix)
- # We should have had three publishers setup:
- oops_config = utility._oops_config
- self.assertEqual(3, len(oops_config.publishers))
- # - a rabbit publisher
- self.assertIsInstance(oops_config.publishers[0], oops_amqp.Publisher)
- # - a datedir publisher wrapped in a publish_new_only wrapper
- datedir_repo = utility._oops_datedir_repo
- publisher = oops_config.publishers[1].func_closure[0].cell_contents
- self.assertEqual(publisher, datedir_repo.publish)
+ # We should have had two publishers set up:
+ self.assertEqual(2, len(utility._all_publishers))
+ # - a fallback publisher chaining a rabbit publisher and a datedir
+ # publisher
+ self.assertIsInstance(
+ utility._main_publishers[0], oops_amqp.Publisher)
+ self.assertEqual(
+ utility._main_publishers[1], utility._oops_datedir_repo.publish)
# - a notify publisher
- self.assertEqual(oops_config.publishers[2], notify_publisher)
+ self.assertEqual(utility._all_publishers[1], notify_publisher)
def test_multiple_raises_in_request(self):
"""An OOPS links to the previous OOPS in the request, if any."""
utility = ErrorReportingUtility()
- del utility._oops_config.publishers[0]
+ utility._main_publishers[0].__call__ = lambda report: []
request = TestRequestWithPrincipal()
try:
@@ -128,7 +127,7 @@
def test_raising_with_request(self):
"""Test ErrorReportingUtility.raising() with a request"""
utility = ErrorReportingUtility()
- del utility._oops_config.publishers[0]
+ utility._main_publishers[0].__call__ = lambda report: []
request = TestRequestWithPrincipal(
environ={
@@ -174,7 +173,7 @@
directlyProvides(request, IXMLRPCRequest)
request.getPositionalArguments = lambda: (1, 2)
utility = ErrorReportingUtility()
- del utility._oops_config.publishers[:]
+ utility._oops_config.publisher = None
try:
raise ArbitraryException('xyz\nabc')
except ArbitraryException:
@@ -186,7 +185,7 @@
# still be unicode (or utf8).
request = TestRequest(form={'foo\x85': 'bar'})
utility = ErrorReportingUtility()
- del utility._oops_config.publishers[:]
+ utility._oops_config.publisher = None
try:
raise ArbitraryException('foo')
except ArbitraryException:
@@ -203,7 +202,7 @@
request = TestRequest()
directlyProvides(request, WebServiceLayer)
utility = ErrorReportingUtility()
- del utility._oops_config.publishers[:]
+ utility._oops_config.publisher = None
# Exceptions that don't use error_status result in OOPSes.
try:
@@ -236,7 +235,7 @@
def test_raising_for_script(self):
"""Test ErrorReportingUtility.raising with a ScriptRequest."""
utility = ErrorReportingUtility()
- del utility._oops_config.publishers[:]
+ utility._oops_config.publisher = None
# A list because code using ScriptRequest expects that - ScriptRequest
# translates it to a dict for now.
@@ -264,7 +263,7 @@
__repr__ = __str__
utility = ErrorReportingUtility()
- del utility._oops_config.publishers[:]
+ utility._oops_config.publisher = None
try:
raise UnprintableException()
except UnprintableException:
@@ -278,7 +277,7 @@
def test_raising_unauthorized_without_request(self):
"""Unauthorized exceptions are logged when there's no request."""
utility = ErrorReportingUtility()
- del utility._oops_config.publishers[:]
+ utility._oops_config.publisher = None
try:
raise Unauthorized('xyz')
except Unauthorized:
@@ -289,7 +288,7 @@
"""Unauthorized exceptions are logged when the request has no
principal."""
utility = ErrorReportingUtility()
- del utility._oops_config.publishers[:]
+ utility._oops_config.publisher = None
request = ScriptRequest([('name2', 'value2')])
try:
raise Unauthorized('xyz')
@@ -301,7 +300,7 @@
"""Unauthorized exceptions are not logged when the request has an
unauthenticated principal."""
utility = ErrorReportingUtility()
- del utility._oops_config.publishers[:]
+ utility._oops_config.publisher = None
request = TestRequestWithUnauthenticatedPrincipal()
try:
raise Unauthorized('xyz')
@@ -312,7 +311,7 @@
"""Unauthorized exceptions are logged when the request has an
authenticated principal."""
utility = ErrorReportingUtility()
- del utility._oops_config.publishers[:]
+ utility._oops_config.publisher = None
request = TestRequestWithPrincipal()
try:
raise Unauthorized('xyz')
@@ -328,7 +327,7 @@
raised.
"""
utility = ErrorReportingUtility()
- del utility._oops_config.publishers[:]
+ utility._oops_config.publisher = None
self.assertTrue(
TranslationUnavailable.__name__ in utility._ignored_exceptions,
'TranslationUnavailable is not in _ignored_exceptions.')
@@ -340,7 +339,7 @@
def test_ignored_exceptions_for_offsite_referer(self):
# Exceptions caused by bad URLs that may not be an Lp code issue.
utility = ErrorReportingUtility()
- del utility._oops_config.publishers[:]
+ utility._oops_config.publisher = None
errors = set([
GoneError.__name__, InvalidBatchSizeError.__name__,
NotFound.__name__])
@@ -351,7 +350,7 @@
# Oopses are reported when Launchpad is the referer for a URL
# that caused an exception.
utility = ErrorReportingUtility()
- del utility._oops_config.publishers[:]
+ utility._oops_config.publisher = None
request = TestRequest(
environ={
'SERVER_URL': 'http://launchpad.dev/fnord',
@@ -366,7 +365,7 @@
# Oopses are reported when a Launchpad vhost is the referer for a URL
# that caused an exception.
utility = ErrorReportingUtility()
- del utility._oops_config.publishers[:]
+ utility._oops_config.publisher = None
request = TestRequest(
environ={
'SERVER_URL': 'http://launchpad.dev/fnord',
@@ -381,7 +380,7 @@
# Oopses are reported when a Launchpad referer for a bad URL on a
# vhost that caused an exception.
utility = ErrorReportingUtility()
- del utility._oops_config.publishers[:]
+ utility._oops_config.publisher = None
request = TestRequest(
environ={
'SERVER_URL': 'http://bazaar.launchpad.dev/fnord',
@@ -395,7 +394,7 @@
def test_ignored_exceptions_for_offsite_referer_not_reported(self):
# Oopses are not reported when Launchpad is not the referer.
utility = ErrorReportingUtility()
- del utility._oops_config.publishers[:]
+ utility._oops_config.publisher = None
# There is no HTTP_REFERER header in this request
request = TestRequest(
environ={'SERVER_URL': 'http://launchpad.dev/fnord'})
@@ -412,7 +411,7 @@
raised.
"""
utility = ErrorReportingUtility()
- del utility._oops_config.publishers[:]
+ utility._oops_config.publisher = None
try:
raise NoReferrerError('xyz')
except NoReferrerError:
@@ -434,7 +433,7 @@
exc_tb = traceback.format_exc()
utility = ErrorReportingUtility()
- del utility._oops_config.publishers[:]
+ utility._oops_config.publisher = None
report = utility.raising((exc_type, exc_value, exc_tb))
# traceback is what we supplied.
self.assertEqual(exc_tb, report['tb_text'])
@@ -442,7 +441,7 @@
def test_oopsMessage(self):
"""oopsMessage pushes and pops the messages."""
utility = ErrorReportingUtility()
- del utility._oops_config.publishers[:]
+ utility._oops_config.publisher = None
with utility.oopsMessage({'a': 'b', 'c': 'd'}):
self.assertEqual(
{0: {'a': 'b', 'c': 'd'}}, utility._oops_messages)
@@ -460,7 +459,7 @@
def test__makeErrorReport_includes_oops_messages(self):
"""The error report should include the oops messages."""
utility = ErrorReportingUtility()
- del utility._oops_config.publishers[:]
+ utility._oops_config.publisher = None
with utility.oopsMessage(dict(a='b', c='d')):
try:
raise ArbitraryException('foo')
@@ -474,7 +473,7 @@
def test__makeErrorReport_combines_request_and_error_vars(self):
"""The oops messages should be distinct from real request vars."""
utility = ErrorReportingUtility()
- del utility._oops_config.publishers[:]
+ utility._oops_config.publisher = None
request = ScriptRequest([('c', 'd')])
with utility.oopsMessage(dict(a='b')):
try:
@@ -504,7 +503,7 @@
def test_session_queries_filtered(self):
"""Test that session queries are filtered."""
utility = ErrorReportingUtility()
- del utility._oops_config.publishers[:]
+ utility._oops_config.publisher = None
timeline = Timeline()
timeline.start("SQL-session", "SELECT 'gone'").finish()
try:
@@ -567,7 +566,7 @@
# A request originating from another site that generates a NotFound
# (404) is ignored (i.e., no OOPS is logged).
utility = ErrorReportingUtility()
- del utility._oops_config.publishers[:]
+ utility._oops_config.publisher = None
report = {'type': 'NotFound',
'url': 'http://example.com',
'req_vars': {'HTTP_REFERER': 'example.com'}}
@@ -577,7 +576,7 @@
# A request originating from a local site that generates a NotFound
# (404) produces an OOPS.
utility = ErrorReportingUtility()
- del utility._oops_config.publishers[:]
+ utility._oops_config.publisher = None
report = {'type': 'NotFound',
'url': 'http://example.com',
'req_vars': {'HTTP_REFERER': 'http://launchpad.dev/'}}
@@ -587,7 +586,7 @@
# If a 404 is generated and there is no HTTP referer, we don't produce
# an OOPS.
utility = ErrorReportingUtility()
- del utility._oops_config.publishers[:]
+ utility._oops_config.publisher = None
report = {'type': 'NotFound',
'url': 'http://example.com',
'req_vars': {}}
@@ -595,7 +594,7 @@
def test_ignored_report_filtered(self):
utility = ErrorReportingUtility()
- del utility._oops_config.publishers[:]
+ utility._oops_config.publisher = None
report = {'ignore': True}
self.assertEqual(None, utility._oops_config.publish(report))
=== modified file 'lib/lp/testing/fixture.py'
--- lib/lp/testing/fixture.py 2015-10-02 11:35:31 +0000
+++ lib/lp/testing/fixture.py 2017-09-02 13:43:11 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Launchpad test fixtures that have no better home."""
@@ -298,7 +298,7 @@
self.channel = self.connection.channel()
self.addCleanup(self.channel.close)
self.oops_config = oops.Config()
- self.oops_config.publishers.append(self._add_oops)
+ self.oops_config.publisher = self._add_oops
self.setUpQueue()
def setUpQueue(self):
@@ -325,6 +325,7 @@
if report['id'] not in self.oops_ids:
self.oopses.append(report)
self.oops_ids.add(report['id'])
+ return [report['id']]
@adapter(ErrorReportEvent)
def _recordOops(self, event):
=== modified file 'lib/lp/testing/tests/test_fixture.py'
--- lib/lp/testing/tests/test_fixture.py 2015-10-06 00:02:50 +0000
+++ lib/lp/testing/tests/test_fixture.py 2017-09-02 13:43:11 +0000
@@ -245,14 +245,14 @@
def test_subscribes_to_events(self):
capture = self.useFixture(CaptureOops())
- publishers = globalErrorUtility._oops_config.publishers[:]
+ publisher = globalErrorUtility._oops_config.publisher
try:
- globalErrorUtility._oops_config.publishers[:] = [notify_publisher]
+ globalErrorUtility._oops_config.publisher = notify_publisher
id = globalErrorUtility.raising(sys.exc_info())['id']
self.assertEqual(id, capture.oopses[0]['id'])
self.assertEqual(1, len(capture.oopses))
finally:
- globalErrorUtility._oops_config.publishers[:] = publishers
+ globalErrorUtility._oops_config.publisher = publisher
class TestCaptureOopsRabbit(TestCase):
@@ -272,9 +272,9 @@
amqp_publisher = oops_amqp.Publisher(
factory, exchange, routing_key, inherit_id=True)
oops = {'id': 'fnor', 'foo': 'dr'}
- self.assertEqual('fnor', amqp_publisher(oops))
+ self.assertEqual(['fnor'], amqp_publisher(oops))
oops2 = {'id': 'quux', 'foo': 'strangelove'}
- self.assertEqual('quux', amqp_publisher(oops2))
+ self.assertEqual(['quux'], amqp_publisher(oops2))
capture.sync()
self.assertEqual([oops, oops2], capture.oopses)
=== modified file 'versions.cfg'
--- versions.cfg 2017-09-01 22:43:16 +0000
+++ versions.cfg 2017-09-02 13:43:11 +0000
@@ -70,11 +70,11 @@
mock = 1.0.1
mocker = 1.1.1
oauth = 1.0
-oops = 0.0.10
-oops-amqp = 0.0.6
-oops-datedir-repo = 0.0.14
+oops = 0.0.13
+oops-amqp = 0.0.8b1
+oops-datedir-repo = 0.0.23
oops-timeline = 0.0.1
-oops-twisted = 0.0.6
+oops-twisted = 0.0.7
oops-wsgi = 0.0.8
ordereddict = 1.1
oslo.config = 1.1.1
@@ -90,7 +90,6 @@
pygpgme = 0.2
pyinotify = 0.9.4
pymacaroons = 0.9.2
-pymongo = 2.1.1
pyOpenSSL = 0.13
pystache = 0.5.3
python-dateutil = 1.5
Follow ups
