launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/job-oops-timeline into lp:launchpad.

Commit message:
Use a per-job OOPS timeline.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/job-oops-timeline/+merge/341554

Job OOPSes currently tend to be rather useless since they don't have a proper timeline set up, so they end up lacking SQL timelines and with other junk in them from previous things the runner has done.  This should improve that situation.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/job-oops-timeline into lp:launchpad.

=== modified file 'lib/lp/services/job/runner.py'
--- lib/lp/services/job/runner.py	2017-09-27 02:12:20 +0000
+++ lib/lp/services/job/runner.py	2018-03-17 01:54:12 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Facilities for running Jobs."""
@@ -81,6 +81,10 @@
     )
 from lp.services.twistedsupport import run_reactor
 from lp.services.webapp import errorlog
+from lp.services.webapp.adapter import (
+    clear_request_started,
+    set_request_started,
+    )
 
 
 class BaseRunnableJobSource:
@@ -311,6 +315,14 @@
         finally:
             set_default_timeout_function(original_timeout_function)
 
+    def runJobHandleError(self, job, fallback=None):
+        set_request_started(enable_timeout=False)
+        try:
+            return super(BaseJobRunner, self).runJobHandleError(
+                job, fallback=fallback)
+        finally:
+            clear_request_started()
+
     def userErrorTypes(self, job):
         return removeSecurityProxy(job).user_error_types
 

=== modified file 'lib/lp/services/job/tests/test_runner.py'
--- lib/lp/services/job/tests/test_runner.py	2017-09-14 03:57:01 +0000
+++ lib/lp/services/job/tests/test_runner.py	2018-03-17 01:54:12 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for job-running facilities."""
@@ -17,6 +17,7 @@
     LeaseHeld,
     SuspendJobException,
     )
+from lazr.restful.utils import get_current_browser_request
 from pytz import UTC
 from testtools.matchers import (
     GreaterThan,
@@ -29,6 +30,7 @@
 from zope.interface import implementer
 
 from lp.services.config import config
+from lp.services.database.sqlbase import flush_database_updates
 from lp.services.features.testing import FeatureFixture
 from lp.services.job.interfaces.job import (
     IRunnableJob,
@@ -42,6 +44,7 @@
     TwistedJobRunner,
     )
 from lp.services.log.logger import BufferLogger
+from lp.services.timeline.requesttimeline import get_request_timeline
 from lp.services.timeout import (
     get_default_timeout_function,
     set_default_timeout_function,
@@ -100,6 +103,15 @@
         raise RaisingJobException(self.message)
 
 
+class RaisingJobTimelineMessage(NullJob):
+    """A job that records a timeline action and then raises when it runs."""
+
+    def run(self):
+        timeline = get_request_timeline(get_current_browser_request())
+        timeline.start('job', self.message).finish()
+        raise RaisingJobException(self.message)
+
+
 class RaisingJobUserError(NullJob):
     """A job that raises a user error when it runs."""
 
@@ -328,6 +340,19 @@
         runner.runJobHandleError(job)
         self.assertEqual(1, len(self.oopses))
 
+    def test_runJobHandleErrors_oops_timeline(self):
+        """The oops timeline only covers the job itself."""
+        timeline = get_request_timeline(get_current_browser_request())
+        timeline.start('test', 'sentinel').finish()
+        job = RaisingJobTimelineMessage('boom')
+        flush_database_updates()
+        runner = JobRunner([job])
+        runner.runJobHandleError(job)
+        self.assertEqual(1, len(self.oopses))
+        actions = [action[2:4] for action in self.oopses[0]['timeline']]
+        self.assertIn(('job', 'boom'), actions)
+        self.assertNotIn(('test', 'sentinel'), actions)
+
     def test_runJobHandleErrors_user_error_no_oops(self):
         """If the job raises a user error, there is no oops."""
         job = RaisingJobUserError('boom')