Merge ~cjwatson/launchpad:revert-better-oops-traceback-annotations into launchpad:master

[Colin Watson]

Partially revert timeline traceback handling changes

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.

[Jürgen Gmach (jugmac00)]

review: Approve

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",
-                )