launchpad-reviewers team mailing list archive

[Colin Watson <mp+436331@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:pool-file-overwrite-error-enormous-oopses into launchpad:master.

Commit message:
Suppress timeline in PoolFileOverwriteError OOPSes

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/436331

The traceback for `PoolFileOverwriteError` (raised when the publisher attempts to overwrite an existing file in a pool with different content) tells us everything we need to know: we get both of the checksums and the target path.

However, the OOPSes also contain an enormous timeline mainly consisting of every SQL statement the publisher has issued as part of processing that archive.  These timelines tend to cause the OOPS processing system to back up, and provide essentially no information of any value.  Instead, just arrange for these OOPSes to be raised with a temporary empty timeline.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:pool-file-overwrite-error-enormous-oopses into launchpad:master.

diff --git a/lib/lp/services/timeline/requesttimeline.py b/lib/lp/services/timeline/requesttimeline.py
index 0c1460c..7fb0332 100644
--- a/lib/lp/services/timeline/requesttimeline.py
+++ b/lib/lp/services/timeline/requesttimeline.py
@@ -6,8 +6,11 @@
 __all__ = [
     "get_request_timeline",
     "set_request_timeline",
+    "temporary_request_timeline",
 ]
 
+from contextlib import contextmanager
+
 from timeline import Timeline
 
 # XXX RobertCollins 2010-09-01 bug=623199: Undesirable but pragmatic.
@@ -48,3 +51,20 @@ def set_request_timeline(request, timeline):
     return timeline
     # Disabled code path: bug 623199, ideally we would use this code path.
     request.annotations["timeline"] = timeline
+
+
+@contextmanager
+def temporary_request_timeline(request):
+    """Give `request` a temporary timeline.
+
+    This is useful in contexts where we want to raise an OOPS but we know
+    that the timeline is uninteresting and may be very large.
+
+    :param request: A Zope/Launchpad request object.
+    """
+    old_timeline = get_request_timeline(request)
+    try:
+        set_request_timeline(request, Timeline())
+        yield
+    finally:
+        set_request_timeline(request, old_timeline)
diff --git a/lib/lp/services/timeline/tests/test_requesttimeline.py b/lib/lp/services/timeline/tests/test_requesttimeline.py
index 341ce37..a49ba8a 100644
--- a/lib/lp/services/timeline/tests/test_requesttimeline.py
+++ b/lib/lp/services/timeline/tests/test_requesttimeline.py
@@ -11,6 +11,7 @@ from lp.services import webapp
 from lp.services.timeline.requesttimeline import (
     get_request_timeline,
     set_request_timeline,
+    temporary_request_timeline,
 )
 
 
@@ -55,3 +56,16 @@ class TestRequestTimeline(testtools.TestCase):
         self.assertEqual(timeline, get_request_timeline(req))
 
     # Tests while adapter._local contains the timeline ---end---
+
+    def test_temporary_request_timeline(self):
+        req = TestRequest()
+        timeline = Timeline()
+        set_request_timeline(req, timeline)
+        timeline.start("test", "test").finish()
+        self.assertEqual(timeline, get_request_timeline(req))
+        self.assertEqual(1, len(timeline.actions))
+        with temporary_request_timeline(req):
+            self.assertNotEqual(timeline, get_request_timeline(req))
+            get_request_timeline(req).start("test-2", "test-2").finish()
+        self.assertEqual(timeline, get_request_timeline(req))
+        self.assertEqual(1, len(timeline.actions))
diff --git a/lib/lp/soyuz/model/publishing.py b/lib/lp/soyuz/model/publishing.py
index b2f7a9d..82eea43 100644
--- a/lib/lp/soyuz/model/publishing.py
+++ b/lib/lp/soyuz/model/publishing.py
@@ -50,6 +50,7 @@ from lp.services.database.stormexpr import IsDistinctFrom
 from lp.services.librarian.browser import ProxiedLibraryFileAlias
 from lp.services.librarian.model import LibraryFileAlias, LibraryFileContent
 from lp.services.propertycache import cachedproperty, get_property_cache
+from lp.services.timeline.requesttimeline import temporary_request_timeline
 from lp.services.webapp.errorlog import ErrorReportingUtility, ScriptRequest
 from lp.services.worlddata.model.country import Country
 from lp.soyuz.adapters.proxiedsourcefiles import ProxiedSourceLibraryFileAlias
@@ -190,7 +191,8 @@ class ArchivePublisherBase:
             properties = [("error-explanation", message)]
             request = ScriptRequest(properties)
             error_utility = ErrorReportingUtility()
-            error_utility.raising(sys.exc_info(), request)
+            with temporary_request_timeline(request):
+                error_utility.raising(sys.exc_info(), request)
             log.error("%s (%s)" % (message, request.oopsid))
         else:
             self.setPublished()
diff --git a/lib/lp/soyuz/tests/test_publishing.py b/lib/lp/soyuz/tests/test_publishing.py
index d2e041c..7812578 100644
--- a/lib/lp/soyuz/tests/test_publishing.py
+++ b/lib/lp/soyuz/tests/test_publishing.py
@@ -994,8 +994,11 @@ class TestNativePublishing(TestNativePublishingBase):
         pub_source = self.getPubSource(filecontent=b"Something")
         pub_source.publish(self.disk_pool, self.logger)
 
-        # And an oops should be filed for the error.
+        # An oops should be filed for the error, but we don't include the
+        # SQL timeline; it may be very large and tells us nothing that we
+        # can't get from the error message.
         self.assertEqual("PoolFileOverwriteError", self.oopses[0]["type"])
+        self.assertEqual([], self.oopses[0]["timeline"])
 
         self.layer.commit()
         self.assertEqual(pub_source.status, PackagePublishingStatus.PENDING)