launchpad-reviewers team mailing list archive

[Colin Watson <mp+438639@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:revert-better-oops-traceback-annotations into launchpad:master.

Commit message:
Partially revert timeline traceback handling changes

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

Formatting traceback supplements in timelines (commit d00b1f5c789e5fa4a19c2c4b13cbcf00d4ac6c04) results in recursive timeline actions via extra database queries in some cases, causing havoc in some tests.  After exploring various options I regretfully decided to revert this.

I've retained most of the centralization of how `Timeline` objects are created, since that seems like a good idea regardless.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:revert-better-oops-traceback-annotations into launchpad:master.

diff --git a/lib/lp/services/timeline/requesttimeline.py b/lib/lp/services/timeline/requesttimeline.py
index c529ace..d09b098 100644
--- a/lib/lp/services/timeline/requesttimeline.py
+++ b/lib/lp/services/timeline/requesttimeline.py
@@ -71,7 +71,17 @@ def make_timeline(
         factory = partial(FilteredTimeline, detail_filter=detail_filter)
     else:
         factory = Timeline
-    return factory(actions=actions, format_stack=format_stack)
+    # XXX cjwatson 2023-03-09: Ideally we'd pass `format_stack=format_stack`
+    # here so that we pick up traceback supplements.  Unfortunately, the act
+    # of formatting traceback supplements (e.g. TALESTracebackSupplement)
+    # often turns out to involve database access, and the effect of that is
+    # to recursively add a timeline action, which seems bad; it can also be
+    # problematic for the parts of a timeline that immediately follow the
+    # end of a transaction.  Blocking database access here just results in a
+    # traceback from the exception formatter, which isn't much better.
+    # Until we find some solution to this, we'll have to live with plain
+    # tracebacks.
+    return factory(actions=actions)
 
 
 def get_request_timeline(request):
diff --git a/lib/lp/services/webapp/tests/test_statementtracer.py b/lib/lp/services/webapp/tests/test_statementtracer.py
index 92351e0..383865d 100644
--- a/lib/lp/services/webapp/tests/test_statementtracer.py
+++ b/lib/lp/services/webapp/tests/test_statementtracer.py
@@ -10,7 +10,6 @@ from contextlib import contextmanager
 from lazr.restful.utils import get_current_browser_request
 
 from lp.services.osutils import override_environ
-from lp.services.tests.test_stacktrace import Supplement
 from lp.services.timeline.requesttimeline import get_request_timeline
 from lp.services.webapp import adapter as da
 from lp.testing import (
@@ -462,31 +461,3 @@ class TestLoggingWithinRequest(TestCaseWithFactory):
                     self.connection, None, "SELECT * FROM one", (), Exception()
                 )
                 self.assertIsNone(self.connection._lp_statement_action)
-
-    def test_includes_traceback_supplement_and_info(self):
-        # The timeline records information from `__traceback_supplement__`
-        # and `__traceback_info__` in tracebacks.
-        tracer = da.LaunchpadStatementTracer()
-
-        def call():
-            __traceback_supplement__ = (
-                Supplement,
-                {"expression": "something"},
-            )
-            __traceback_info__ = "Extra information"
-            tracer.connection_raw_execute(
-                self.connection, None, "SELECT * FROM one", ()
-            )
-
-        with person_logged_in(self.person):
-            with StormStatementRecorder(tracebacks_if=True):
-                call()
-                timeline = get_request_timeline(get_current_browser_request())
-                self.assertRegex(
-                    timeline.actions[-1].backtrace,
-                    "\n"
-                    "  File .*, in call\n"
-                    "    .*\n"
-                    "   - Expression: something\n"
-                    "   - __traceback_info__: Extra information\n",
-                )