Rewrite FastAPI instrumentor middleware stack to be failsafe

CHANGELOG.md

@@ -11,6 +11,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## Unreleased
 
+### Fixed
+
+- `opentelemetry-instrumentation-fastapi`: Implemented failsafe middleware stack.
+  ([#3664](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3664))
+
 ## Version 1.36.0/0.57b0 (2025-07-29)
 
 ### Fixed

instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py

@@ -189,7 +189,6 @@ def client_response_hook(span: Span, scope: dict[str, Any], message: dict[str, A
 
 import fastapi
 from starlette.applications import Starlette
-from starlette.middleware.errors import ServerErrorMiddleware
 from starlette.routing import Match
 from starlette.types import ASGIApp
 
@@ -210,7 +209,7 @@ def client_response_hook(span: Span, scope: dict[str, Any], message: dict[str, A
 from opentelemetry.instrumentation.instrumentor import BaseInstrumentor
 from opentelemetry.metrics import MeterProvider, get_meter
 from opentelemetry.semconv.attributes.http_attributes import HTTP_ROUTE
-from opentelemetry.trace import TracerProvider, get_tracer
+from opentelemetry.trace import Span, TracerProvider, get_tracer
 from opentelemetry.util.http import (
     get_excluded_urls,
     parse_excluded_urls,
@@ -289,22 +288,33 @@ def instrument_app(
                 schema_url=_get_schema_url(sem_conv_opt_in_mode),
             )
 
-            # Instead of using `app.add_middleware` we monkey patch `build_middleware_stack` to insert our middleware
-            # as the outermost middleware.
-            # Otherwise `OpenTelemetryMiddleware` would have unhandled exceptions tearing through it and would not be able
-            # to faithfully record what is returned to the client since it technically cannot know what `ServerErrorMiddleware` is going to do.
-
+            # In order to make traces available at any stage of the request
+            # processing - including exception handling - we wrap ourselves as
+            # the new, outermost middleware. However in order to prevent
+            # exceptions from user-provided hooks of tearing through, we wrap
+            # them to return without failure unconditionally.
             def build_middleware_stack(self: Starlette) -> ASGIApp:
+                def failsafe(func):
+                    @functools.wraps(func)
+                    def wrapper(span: Span, *args, **kwargs):
+                        try:
+                            func(span, *args, **kwargs)
+                        except Exception as exc:  # pylint: disable=W0718
+                            span.record_exception(exc)
+
+                    return wrapper
+
                 inner_server_error_middleware: ASGIApp = (  # type: ignore
                     self._original_build_middleware_stack()  # type: ignore
                 )
-                otel_middleware = OpenTelemetryMiddleware(
+
+                return OpenTelemetryMiddleware(
                     inner_server_error_middleware,
                     excluded_urls=excluded_urls,
                     default_span_details=_get_default_span_details,
-                    server_request_hook=server_request_hook,
-                    client_request_hook=client_request_hook,
-                    client_response_hook=client_response_hook,
+                    server_request_hook=failsafe(server_request_hook),
+                    client_request_hook=failsafe(client_request_hook),
+                    client_response_hook=failsafe(client_response_hook),
                     # Pass in tracer/meter to get __name__and __version__ of fastapi instrumentation
                     tracer=tracer,
                     meter=meter,
@@ -313,23 +323,6 @@ def build_middleware_stack(self: Starlette) -> ASGIApp:
                     http_capture_headers_sanitize_fields=http_capture_headers_sanitize_fields,
                     exclude_spans=exclude_spans,
                 )
-                # Wrap in an outer layer of ServerErrorMiddleware so that any exceptions raised in OpenTelemetryMiddleware
-                # are handled.
-                # This should not happen unless there is a bug in OpenTelemetryMiddleware, but if there is we don't want that
-                # to impact the user's application just because we wrapped the middlewares in this order.
-                if isinstance(
-                    inner_server_error_middleware, ServerErrorMiddleware
-                ):  # usually true
-                    outer_server_error_middleware = ServerErrorMiddleware(
-                        app=otel_middleware,
-                    )
-                else:
-                    # Something else seems to have patched things, or maybe Starlette changed.
-                    # Just create a default ServerErrorMiddleware.
-                    outer_server_error_middleware = ServerErrorMiddleware(
-                        app=otel_middleware
-                    )
-                return outer_server_error_middleware
 
             app._original_build_middleware_stack = app.build_middleware_stack
             app.build_middleware_stack = types.MethodType(

instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py

@@ -59,6 +59,9 @@
 from opentelemetry.semconv._incubating.attributes.net_attributes import (
     NET_HOST_PORT,
 )
+from opentelemetry.semconv.attributes.exception_attributes import (
+    EXCEPTION_TYPE,
+)
 from opentelemetry.semconv.attributes.http_attributes import (
     HTTP_REQUEST_METHOD,
     HTTP_RESPONSE_STATUS_CODE,
@@ -1877,3 +1880,126 @@ def test_custom_header_not_present_in_non_recording_span(self):
         self.assertEqual(200, resp.status_code)
         span_list = self.memory_exporter.get_finished_spans()
         self.assertEqual(len(span_list), 0)
+
+
+class TestTraceableExceptionHandling(TestBase):
+    """Tests to ensure FastAPI exception handlers are only executed once and with a valid context"""
+
+    def setUp(self):
+        super().setUp()
+
+        self.app = fastapi.FastAPI()
+
+        otel_fastapi.FastAPIInstrumentor().instrument_app(self.app)
+        self.client = TestClient(self.app)
+        self.tracer = self.tracer_provider.get_tracer(__name__)
+        self.executed = 0
+        self.request_trace_id = None
+        self.error_trace_id = None
+
+    def tearDown(self) -> None:
+        super().tearDown()
+        with self.disable_logging():
+            otel_fastapi.FastAPIInstrumentor().uninstrument_app(self.app)
+
+    def test_error_handler_context(self):
+        """OTEL tracing contexts must be available during error handler execution"""
+
+        @self.app.exception_handler(Exception)
+        async def _(*_):
+            self.error_trace_id = (
+                trace.get_current_span().get_span_context().trace_id
+            )
+
+        @self.app.get("/foobar")
+        async def _():
+            self.request_trace_id = (
+                trace.get_current_span().get_span_context().trace_id
+            )
+            raise UnhandledException("Test Exception")
+
+        try:
+            self.client.get(
+                "/foobar",
+            )
+        except Exception:  # pylint: disable=W0718
+            pass
+
+        self.assertIsNotNone(self.request_trace_id)
+        self.assertEqual(self.request_trace_id, self.error_trace_id)
+
+    def test_error_handler_side_effects(self):
+        """FastAPI default exception handlers (aka error handlers) must be executed exactly once per exception"""
+
+        @self.app.exception_handler(Exception)
+        async def _(*_):
+            self.executed += 1
+
+        @self.app.get("/foobar")
+        async def _():
+            raise UnhandledException("Test Exception")
+
+        try:
+            self.client.get(
+                "/foobar",
+            )
+        except Exception:  # pylint: disable=W0718
+            pass
+
+        self.assertEqual(self.executed, 1)
+
+
+class TestFailsafeHooks(TestBase):
+    """Tests to ensure FastAPI instrumentation hooks don't tear through"""
+
+    def setUp(self):
+        super().setUp()
+
+        self.app = fastapi.FastAPI()
+
+        @self.app.get("/foobar")
+        async def _():
+            return {"message": "Hello World"}
+
+        def failing_hook(*_):
+            raise UnhandledException("Hook Exception")
+
+        otel_fastapi.FastAPIInstrumentor().instrument_app(
+            self.app,
+            server_request_hook=failing_hook,
+            client_request_hook=failing_hook,
+            client_response_hook=failing_hook,
+        )
+        self.client = TestClient(self.app)
+
+    def tearDown(self) -> None:
+        super().tearDown()
+        with self.disable_logging():
+            otel_fastapi.FastAPIInstrumentor().uninstrument_app(self.app)
+
+    def test_failsafe_hooks(self):
+        """Crashing hooks must not tear through"""
+        resp = self.client.get(
+            "/foobar",
+        )
+
+        self.assertEqual(200, resp.status_code)
+
+    def test_failsafe_error_recording(self):
+        """Failing hooks must record the exception on the span"""
+        self.client.get(
+            "/foobar",
+        )
+
+        spans = self.memory_exporter.get_finished_spans()
+
+        self.assertEqual(len(spans), 3)
+        span = spans[0]
+        self.assertEqual(len(span.events), 1)
+        event = span.events[0]
+        self.assertEqual(event.name, "exception")
+        assert event.attributes is not None
+        self.assertEqual(
+            event.attributes.get(EXCEPTION_TYPE),
+            f"{__name__}.UnhandledException",
+        )