From 707b1927ea666f2a1a886a87621c2464d793d24c Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Fri, 15 Nov 2024 09:12:12 -0600 Subject: [PATCH 01/22] fastapi: fix wrapping of middlewares --- .../instrumentation/fastapi/__init__.py | 58 ++++++++++++------- 1 file changed, 38 insertions(+), 20 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py b/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py index 7e4d0aac07..1b509e8ff5 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py @@ -179,10 +179,14 @@ def client_response_hook(span: Span, scope: dict[str, Any], message: dict[str, A from __future__ import annotations import logging +import types from typing import Collection, Literal import fastapi +from starlette.applications import Starlette +from starlette.middleware.error import ServerErrorMiddleware from starlette.routing import Match +from starlette.types import ASGIApp from opentelemetry.instrumentation._semconv import ( _get_schema_url, @@ -280,21 +284,38 @@ def instrument_app( schema_url=_get_schema_url(sem_conv_opt_in_mode), ) - app.add_middleware( - OpenTelemetryMiddleware, - 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, - # Pass in tracer/meter to get __name__and __version__ of fastapi instrumentation - tracer=tracer, - meter=meter, - http_capture_headers_server_request=http_capture_headers_server_request, - http_capture_headers_server_response=http_capture_headers_server_response, - http_capture_headers_sanitize_fields=http_capture_headers_sanitize_fields, - exclude_spans=exclude_spans, - ) + # 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. + + def build_middleware_stack(self: Starlette) -> ASGIApp: + stack = super().build_middleware_stack() + stack = OpenTelemetryMiddleware( + stack, + 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, + # Pass in tracer/meter to get __name__and __version__ of fastapi instrumentation + tracer=tracer, + meter=meter, + http_capture_headers_server_request=http_capture_headers_server_request, + http_capture_headers_server_response=http_capture_headers_server_response, + 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. + stack = ServerErrorMiddleware(stack) + return stack + + app._original_build_middleware_stack = app.build_middleware_stack + app.build_middleware_stack = types.MethodType(build_middleware_stack, app) + app._is_instrumented_by_opentelemetry = True if app not in _InstrumentedFastAPI._instrumented_fastapi_apps: _InstrumentedFastAPI._instrumented_fastapi_apps.add(app) @@ -305,11 +326,8 @@ def instrument_app( @staticmethod def uninstrument_app(app: fastapi.FastAPI): - app.user_middleware = [ - x - for x in app.user_middleware - if x.cls is not OpenTelemetryMiddleware - ] + app.build_middleware_stack = app._original_build_middleware_stack + del app._original_build_middleware_stack app.middleware_stack = app.build_middleware_stack() app._is_instrumented_by_opentelemetry = False From dcfab5684ef9613dbab3e141eba30d8f1b755076 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Fri, 15 Nov 2024 09:30:46 -0600 Subject: [PATCH 02/22] fix import, super --- .../src/opentelemetry/instrumentation/fastapi/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py b/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py index 1b509e8ff5..3bbdf225dc 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py @@ -184,7 +184,7 @@ def client_response_hook(span: Span, scope: dict[str, Any], message: dict[str, A import fastapi from starlette.applications import Starlette -from starlette.middleware.error import ServerErrorMiddleware +from starlette.middleware.errors import ServerErrorMiddleware from starlette.routing import Match from starlette.types import ASGIApp @@ -290,7 +290,7 @@ def instrument_app( # to faithfully record what is returned to the client since it technically cannot know what `ServerErrorMiddleware` is going to do. def build_middleware_stack(self: Starlette) -> ASGIApp: - stack = super().build_middleware_stack() + stack = type(self).build_middleware_stack(self) stack = OpenTelemetryMiddleware( stack, excluded_urls=excluded_urls, From a78b1c610fe1092581bc7ad288cb4688603bf501 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Fri, 15 Nov 2024 09:34:34 -0600 Subject: [PATCH 03/22] add test --- .../tests/test_fastapi_instrumentation.py | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py index fdbad4effb..245b3c6c98 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py @@ -204,12 +204,20 @@ async def _(param: str): @app.get("/healthzz") async def _(): return {"message": "ok"} + + @app.get("/error") + async def _(): + raise UnhandledException("This is an unhandled exception") app.mount("/sub", app=sub_app) return app +class UnhandledException(Exception): + pass + + class TestBaseManualFastAPI(TestBaseFastAPI): @classmethod def setUpClass(cls): @@ -398,6 +406,26 @@ def test_fastapi_excluded_urls(self): spans = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans), 0) + def test_fastapi_unhandled_exception(self): + """If the application has an unhandled error the instrumentation should capture that a 500 response is returned.""" + try: + self._client.get("/error") + except UnhandledException: + pass + else: + self.fail("Expected UnhandledException") + + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 3) + for span in spans: + self.assertIn("GET /error", span.name) + self.assertEqual( + span.attributes[SpanAttributes.HTTP_ROUTE], "/error" + ) + self.assertEqual( + span.attributes[SpanAttributes.HTTP_STATUS_CODE], 500 + ) + def test_fastapi_excluded_urls_not_env(self): """Ensure that given fastapi routes are excluded when passed explicitly (not in the environment)""" app = self._create_app_explicit_excluded_urls() From e25e14310e5f951f43aa742f1221f3858bfd4ee6 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Fri, 15 Nov 2024 09:35:33 -0600 Subject: [PATCH 04/22] changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a1039f83a..2c47e8b53a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#3037](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3037)) - `opentelemetry-instrumentation-sqlalchemy`: Fix a remaining memory leak in EngineTracer ([#3053](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3053)) +- `opentelemetry-instrumentation-fastapi`: instrument unhandled exceptions + ([#3012](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3012)) ### Breaking changes From 54d4482936edce497ff68e31876d5e77b7c7d560 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Fri, 15 Nov 2024 10:05:50 -0600 Subject: [PATCH 05/22] lint --- .../tests/test_fastapi_instrumentation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py index 245b3c6c98..f776ea2b91 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py @@ -204,7 +204,7 @@ async def _(param: str): @app.get("/healthzz") async def _(): return {"message": "ok"} - + @app.get("/error") async def _(): raise UnhandledException("This is an unhandled exception") From 81f1e83fc5eac5ceceec7aa995f39e4544b082b4 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Fri, 15 Nov 2024 10:13:56 -0600 Subject: [PATCH 06/22] lint --- .../src/opentelemetry/instrumentation/fastapi/__init__.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py b/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py index 3bbdf225dc..ce24c6103a 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py @@ -314,7 +314,9 @@ def build_middleware_stack(self: Starlette) -> ASGIApp: return stack app._original_build_middleware_stack = app.build_middleware_stack - app.build_middleware_stack = types.MethodType(build_middleware_stack, app) + app.build_middleware_stack = types.MethodType( + build_middleware_stack, app + ) app._is_instrumented_by_opentelemetry = True if app not in _InstrumentedFastAPI._instrumented_fastapi_apps: From 96e66b99d29c97e5cc363f6db1e4986c60b71fa9 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Fri, 15 Nov 2024 10:33:47 -0600 Subject: [PATCH 07/22] fix --- .../src/opentelemetry/instrumentation/fastapi/__init__.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py b/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py index ce24c6103a..d4fa9d8cd2 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py @@ -328,8 +328,12 @@ def build_middleware_stack(self: Starlette) -> ASGIApp: @staticmethod def uninstrument_app(app: fastapi.FastAPI): - app.build_middleware_stack = app._original_build_middleware_stack - del app._original_build_middleware_stack + original_build_middleware_stack = getattr( + app, "_original_build_middleware_stack", None + ) + if original_build_middleware_stack: + app.build_middleware_stack = original_build_middleware_stack + del app._original_build_middleware_stack app.middleware_stack = app.build_middleware_stack() app._is_instrumented_by_opentelemetry = False From 46e1809cbbcc203d272cdf19d4ef0731ca0db847 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Fri, 15 Nov 2024 11:01:01 -0600 Subject: [PATCH 08/22] ci From daa5a1b29b0bbd5402aecaef1ee6c4d5069f91b8 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Tue, 26 Nov 2024 15:32:09 -0800 Subject: [PATCH 09/22] fix wip --- .../instrumentation/fastapi/__init__.py | 69 ++++++++----------- 1 file changed, 28 insertions(+), 41 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py b/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py index d4fa9d8cd2..dd0dd04a1d 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py @@ -203,9 +203,9 @@ def client_response_hook(span: Span, scope: dict[str, Any], message: dict[str, A from opentelemetry.instrumentation.fastapi.package import _instruments from opentelemetry.instrumentation.fastapi.version import __version__ from opentelemetry.instrumentation.instrumentor import BaseInstrumentor -from opentelemetry.metrics import get_meter +from opentelemetry.metrics import Meter, get_meter from opentelemetry.semconv.trace import SpanAttributes -from opentelemetry.trace import get_tracer +from opentelemetry.trace import Tracer, get_tracer from opentelemetry.util.http import ( get_excluded_urls, parse_excluded_urls, @@ -230,9 +230,9 @@ def instrument_app( server_request_hook: ServerRequestHook = None, client_request_hook: ClientRequestHook = None, client_response_hook: ClientResponseHook = None, - tracer_provider=None, - meter_provider=None, - excluded_urls=None, + tracer_provider: Tracer | None = None, + meter_provider: Meter | None = None, + excluded_urls: str | None = None, http_capture_headers_server_request: list[str] | None = None, http_capture_headers_server_response: list[str] | None = None, http_capture_headers_sanitize_fields: list[str] | None = None, @@ -290,9 +290,9 @@ def instrument_app( # to faithfully record what is returned to the client since it technically cannot know what `ServerErrorMiddleware` is going to do. def build_middleware_stack(self: Starlette) -> ASGIApp: - stack = type(self).build_middleware_stack(self) - stack = OpenTelemetryMiddleware( - stack, + app = type(self).build_middleware_stack(self) + app = OpenTelemetryMiddleware( + app, excluded_urls=excluded_urls, default_span_details=_get_default_span_details, server_request_hook=server_request_hook, @@ -310,8 +310,9 @@ def build_middleware_stack(self: Starlette) -> ASGIApp: # 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. - stack = ServerErrorMiddleware(stack) - return stack + app = ServerErrorMiddleware(app) + return app + app._original_build_middleware_stack = app.build_middleware_stack app.build_middleware_stack = types.MethodType( @@ -385,43 +386,29 @@ class _InstrumentedFastAPI(fastapi.FastAPI): _server_request_hook: ServerRequestHook = None _client_request_hook: ClientRequestHook = None _client_response_hook: ClientResponseHook = None + _http_capture_headers_server_request: list[str] | None = None + _http_capture_headers_server_response: list[str] | None = None + _http_capture_headers_sanitize_fields: list[str] | None = None + _exclude_spans: list[Literal["receive", "send"]] | None = None + _instrumented_fastapi_apps = set() _sem_conv_opt_in_mode = _HTTPStabilityMode.DEFAULT def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - tracer = get_tracer( - __name__, - __version__, - _InstrumentedFastAPI._tracer_provider, - schema_url=_get_schema_url( - _InstrumentedFastAPI._sem_conv_opt_in_mode - ), - ) - meter = get_meter( - __name__, - __version__, - _InstrumentedFastAPI._meter_provider, - schema_url=_get_schema_url( - _InstrumentedFastAPI._sem_conv_opt_in_mode - ), - ) - self.add_middleware( - OpenTelemetryMiddleware, - excluded_urls=_InstrumentedFastAPI._excluded_urls, - default_span_details=_get_default_span_details, - server_request_hook=_InstrumentedFastAPI._server_request_hook, - client_request_hook=_InstrumentedFastAPI._client_request_hook, - client_response_hook=_InstrumentedFastAPI._client_response_hook, - # Pass in tracer/meter to get __name__and __version__ of fastapi instrumentation - tracer=tracer, - meter=meter, - http_capture_headers_server_request=_InstrumentedFastAPI._http_capture_headers_server_request, - http_capture_headers_server_response=_InstrumentedFastAPI._http_capture_headers_server_response, - http_capture_headers_sanitize_fields=_InstrumentedFastAPI._http_capture_headers_sanitize_fields, - exclude_spans=_InstrumentedFastAPI._exclude_spans, + FastAPIInstrumentor.instrument_app( + self, + server_request_hook=self._server_request_hook, + client_request_hook=self._client_request_hook, + client_response_hook=self._client_response_hook, + tracer_provider=self._tracer_provider, + meter_provider=self._meter_provider, + excluded_urls=self._excluded_urls, + http_capture_headers_server_request=self._http_capture_headers_server_request, + http_capture_headers_server_response=self._http_capture_headers_server_response, + http_capture_headers_sanitize_fields=self._http_capture_headers_sanitize_fields, + exclude_spans=self._exclude_spans, ) - self._is_instrumented_by_opentelemetry = True _InstrumentedFastAPI._instrumented_fastapi_apps.add(self) def __del__(self): From de9e795ff279543a3f8a719c9d06c95da062aa82 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Tue, 26 Nov 2024 16:32:55 -0800 Subject: [PATCH 10/22] fix --- .../instrumentation/fastapi/__init__.py | 15 +++---- .../tests/test_fastapi_instrumentation.py | 39 ++++--------------- 2 files changed, 13 insertions(+), 41 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py b/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py index dd0dd04a1d..68c635a526 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py @@ -203,9 +203,9 @@ def client_response_hook(span: Span, scope: dict[str, Any], message: dict[str, A from opentelemetry.instrumentation.fastapi.package import _instruments from opentelemetry.instrumentation.fastapi.version import __version__ from opentelemetry.instrumentation.instrumentor import BaseInstrumentor -from opentelemetry.metrics import Meter, get_meter +from opentelemetry.metrics import MeterProvider, get_meter from opentelemetry.semconv.trace import SpanAttributes -from opentelemetry.trace import Tracer, get_tracer +from opentelemetry.trace import TracerProvider, get_tracer from opentelemetry.util.http import ( get_excluded_urls, parse_excluded_urls, @@ -230,8 +230,8 @@ def instrument_app( server_request_hook: ServerRequestHook = None, client_request_hook: ClientRequestHook = None, client_response_hook: ClientResponseHook = None, - tracer_provider: Tracer | None = None, - meter_provider: Meter | None = None, + tracer_provider: TracerProvider | None = None, + meter_provider: MeterProvider | None = None, excluded_urls: str | None = None, http_capture_headers_server_request: list[str] | None = None, http_capture_headers_server_response: list[str] | None = None, @@ -362,12 +362,7 @@ def _instrument(self, **kwargs): _InstrumentedFastAPI._http_capture_headers_sanitize_fields = ( kwargs.get("http_capture_headers_sanitize_fields") ) - _excluded_urls = kwargs.get("excluded_urls") - _InstrumentedFastAPI._excluded_urls = ( - _excluded_urls_from_env - if _excluded_urls is None - else parse_excluded_urls(_excluded_urls) - ) + _InstrumentedFastAPI._excluded_urls = kwargs.get("excluded_urls") _InstrumentedFastAPI._meter_provider = kwargs.get("meter_provider") _InstrumentedFastAPI._exclude_spans = kwargs.get("exclude_spans") fastapi.FastAPI = _InstrumentedFastAPI diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py index f776ea2b91..32b660c262 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py @@ -170,7 +170,8 @@ def setUp(self): self._instrumentor = otel_fastapi.FastAPIInstrumentor() self._app = self._create_app() self._app.add_middleware(HTTPSRedirectMiddleware) - self._client = TestClient(self._app) + self._client = TestClient(self._app, base_url="https://testserver:443") + self._client.__enter__() # run the lifespan, initialize the middleware stack def tearDown(self): super().tearDown() @@ -205,19 +206,11 @@ async def _(param: str): async def _(): return {"message": "ok"} - @app.get("/error") - async def _(): - raise UnhandledException("This is an unhandled exception") - app.mount("/sub", app=sub_app) return app -class UnhandledException(Exception): - pass - - class TestBaseManualFastAPI(TestBaseFastAPI): @classmethod def setUpClass(cls): @@ -406,26 +399,6 @@ def test_fastapi_excluded_urls(self): spans = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans), 0) - def test_fastapi_unhandled_exception(self): - """If the application has an unhandled error the instrumentation should capture that a 500 response is returned.""" - try: - self._client.get("/error") - except UnhandledException: - pass - else: - self.fail("Expected UnhandledException") - - spans = self.memory_exporter.get_finished_spans() - self.assertEqual(len(spans), 3) - for span in spans: - self.assertIn("GET /error", span.name) - self.assertEqual( - span.attributes[SpanAttributes.HTTP_ROUTE], "/error" - ) - self.assertEqual( - span.attributes[SpanAttributes.HTTP_STATUS_CODE], 500 - ) - def test_fastapi_excluded_urls_not_env(self): """Ensure that given fastapi routes are excluded when passed explicitly (not in the environment)""" app = self._create_app_explicit_excluded_urls() @@ -1152,9 +1125,13 @@ def test_request(self): def test_mulitple_way_instrumentation(self): self._instrumentor.instrument_app(self._app) count = 0 - for middleware in self._app.user_middleware: - if middleware.cls is OpenTelemetryMiddleware: + app = self._app.middleware_stack + while True: + if isinstance(app, OpenTelemetryMiddleware): count += 1 + if app is None: + break + app = getattr(app, "app", None) self.assertEqual(count, 1) def test_uninstrument_after_instrument(self): From f9d348b00fe6174d4288f820161b2f96081cc790 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Tue, 26 Nov 2024 16:33:15 -0800 Subject: [PATCH 11/22] fix --- .../src/opentelemetry/instrumentation/fastapi/__init__.py | 1 - 1 file changed, 1 deletion(-) diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py b/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py index 68c635a526..e7ec2ba104 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py @@ -313,7 +313,6 @@ def build_middleware_stack(self: Starlette) -> ASGIApp: app = ServerErrorMiddleware(app) return app - app._original_build_middleware_stack = app.build_middleware_stack app.build_middleware_stack = types.MethodType( build_middleware_stack, app From 7bc89b373a74340bcc44e1adf6ebf2f35dc3d93b Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Tue, 26 Nov 2024 16:39:28 -0800 Subject: [PATCH 12/22] lint --- .../tests/test_fastapi_instrumentation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py index 32b660c262..b093c0dcc3 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py @@ -171,7 +171,7 @@ def setUp(self): self._app = self._create_app() self._app.add_middleware(HTTPSRedirectMiddleware) self._client = TestClient(self._app, base_url="https://testserver:443") - self._client.__enter__() # run the lifespan, initialize the middleware stack + self._client.__enter__() # noqa: C2801 # run the lifespan, initialize the middleware stack def tearDown(self): super().tearDown() From f62993248bb9761e09fcf554304c2706e248b322 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Tue, 26 Nov 2024 16:48:13 -0800 Subject: [PATCH 13/22] lint --- .../tests/test_fastapi_instrumentation.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py index b093c0dcc3..fda2b32e7a 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py @@ -171,7 +171,9 @@ def setUp(self): self._app = self._create_app() self._app.add_middleware(HTTPSRedirectMiddleware) self._client = TestClient(self._app, base_url="https://testserver:443") - self._client.__enter__() # noqa: C2801 # run the lifespan, initialize the middleware stack + # run the lifespan, initialize the middleware stack + # this is more in-line with what happens in a real application when the server starts up + self._client.__enter__() # pylint: disable=unnecessary-dunder-call def tearDown(self): super().tearDown() From 437891ba680ca85adad6f3856894bb3ca3c43364 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Tue, 26 Nov 2024 22:07:19 -0800 Subject: [PATCH 14/22] Exit? --- .../tests/test_fastapi_instrumentation.py | 1 + 1 file changed, 1 insertion(+) diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py index fda2b32e7a..473ae399d8 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py @@ -176,6 +176,7 @@ def setUp(self): self._client.__enter__() # pylint: disable=unnecessary-dunder-call def tearDown(self): + self._client.__exit__(None, None, None) # pylint: disable=unnecessary-dunder-call super().tearDown() self.env_patch.stop() self.exclude_patch.stop() From 1340ac785b470f12ea304049c990626938b7975f Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Wed, 27 Nov 2024 05:55:58 -0800 Subject: [PATCH 15/22] Update test_fastapi_instrumentation.py Co-authored-by: Riccardo Magliocchetti --- .../tests/test_fastapi_instrumentation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py index 473ae399d8..adff9f0911 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py @@ -1129,7 +1129,7 @@ def test_mulitple_way_instrumentation(self): self._instrumentor.instrument_app(self._app) count = 0 app = self._app.middleware_stack - while True: + while app is not None: if isinstance(app, OpenTelemetryMiddleware): count += 1 if app is None: From 003d7afd78e5b60adeae2db501aa36b0b62f8c74 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Wed, 27 Nov 2024 08:07:33 -0800 Subject: [PATCH 16/22] remove break --- .../tests/test_fastapi_instrumentation.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py index adff9f0911..4aceb78731 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py @@ -1132,8 +1132,6 @@ def test_mulitple_way_instrumentation(self): while app is not None: if isinstance(app, OpenTelemetryMiddleware): count += 1 - if app is None: - break app = getattr(app, "app", None) self.assertEqual(count, 1) From be263930d8dca17665f42448a9195eb38fa5ee90 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Wed, 27 Nov 2024 08:08:53 -0800 Subject: [PATCH 17/22] fix --- .../tests/test_fastapi_instrumentation.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py index 4aceb78731..49f6fd8d3b 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py @@ -173,10 +173,10 @@ def setUp(self): self._client = TestClient(self._app, base_url="https://testserver:443") # run the lifespan, initialize the middleware stack # this is more in-line with what happens in a real application when the server starts up - self._client.__enter__() # pylint: disable=unnecessary-dunder-call + self._client_teardown = self._client.__enter__() # pylint: disable=unnecessary-dunder-call def tearDown(self): - self._client.__exit__(None, None, None) # pylint: disable=unnecessary-dunder-call + self._client_teardown.__exit__(None, None, None) # pylint: disable=unnecessary-dunder-call super().tearDown() self.env_patch.stop() self.exclude_patch.stop() From ec545b7387b270d646900447e5d368e0fa5f8317 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Wed, 27 Nov 2024 08:09:55 -0800 Subject: [PATCH 18/22] remove dunders --- .../tests/test_fastapi_instrumentation.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py index 49f6fd8d3b..b54772a1fb 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py @@ -15,6 +15,7 @@ # pylint: disable=too-many-lines import unittest +from contextlib import ExitStack from timeit import default_timer from unittest.mock import Mock, patch @@ -173,10 +174,11 @@ def setUp(self): self._client = TestClient(self._app, base_url="https://testserver:443") # run the lifespan, initialize the middleware stack # this is more in-line with what happens in a real application when the server starts up - self._client_teardown = self._client.__enter__() # pylint: disable=unnecessary-dunder-call + self._exit_stack = ExitStack() + self._exit_stack.enter_context(self._client) def tearDown(self): - self._client_teardown.__exit__(None, None, None) # pylint: disable=unnecessary-dunder-call + self._exit_stack.close() super().tearDown() self.env_patch.stop() self.exclude_patch.stop() From f825c7697cef1b11fcb0589aae4235b937d4c33a Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Fri, 15 Nov 2024 09:34:34 -0600 Subject: [PATCH 19/22] add test --- .../tests/test_fastapi_instrumentation.py | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py index b54772a1fb..7047482d31 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py @@ -210,12 +210,20 @@ async def _(param: str): @app.get("/healthzz") async def _(): return {"message": "ok"} + + @app.get("/error") + async def _(): + raise UnhandledException("This is an unhandled exception") app.mount("/sub", app=sub_app) return app +class UnhandledException(Exception): + pass + + class TestBaseManualFastAPI(TestBaseFastAPI): @classmethod def setUpClass(cls): @@ -404,6 +412,26 @@ def test_fastapi_excluded_urls(self): spans = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans), 0) + def test_fastapi_unhandled_exception(self): + """If the application has an unhandled error the instrumentation should capture that a 500 response is returned.""" + try: + self._client.get("/error") + except UnhandledException: + pass + else: + self.fail("Expected UnhandledException") + + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 3) + for span in spans: + self.assertIn("GET /error", span.name) + self.assertEqual( + span.attributes[SpanAttributes.HTTP_ROUTE], "/error" + ) + self.assertEqual( + span.attributes[SpanAttributes.HTTP_STATUS_CODE], 500 + ) + def test_fastapi_excluded_urls_not_env(self): """Ensure that given fastapi routes are excluded when passed explicitly (not in the environment)""" app = self._create_app_explicit_excluded_urls() From d90963377d5930087b98de10e393e231165361b5 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Tue, 3 Dec 2024 08:09:13 -0800 Subject: [PATCH 20/22] lint --- .../tests/test_fastapi_instrumentation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py index 7047482d31..40c20500da 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py @@ -210,7 +210,7 @@ async def _(param: str): @app.get("/healthzz") async def _(): return {"message": "ok"} - + @app.get("/error") async def _(): raise UnhandledException("This is an unhandled exception") From 0de3e3776de042f08dc1c0bce9e3884cd6f3fda4 Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Wed, 4 Dec 2024 09:34:46 -0600 Subject: [PATCH 21/22] add endpoint to class --- .../tests/test_fastapi_instrumentation.py | 43 ++++++++++--------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py index 40c20500da..0c2ce97cb6 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py @@ -234,6 +234,25 @@ def setUpClass(cls): super(TestBaseManualFastAPI, cls).setUpClass() + def test_fastapi_unhandled_exception(self): + """If the application has an unhandled error the instrumentation should capture that a 500 response is returned.""" + try: + resp = self._client.get("/error") + assert resp.status_code == 500, resp.content # pragma: no cover, for debugging this test if an exception is _not_ raised + except UnhandledException: + pass + else: + self.fail("Expected UnhandledException") + + spans = self.memory_exporter.get_finished_spans() + self.assertEqual(len(spans), 3) + span = spans[0] + assert span.name == "GET /error http send" + assert span.attributes[SpanAttributes.HTTP_STATUS_CODE] == 500 + span = spans[2] + assert span.name == "GET /error" + assert span.attributes[SpanAttributes.HTTP_TARGET] == "/error" + def test_sub_app_fastapi_call(self): """ This test is to ensure that a span in case of a sub app targeted contains the correct server url @@ -412,26 +431,6 @@ def test_fastapi_excluded_urls(self): spans = self.memory_exporter.get_finished_spans() self.assertEqual(len(spans), 0) - def test_fastapi_unhandled_exception(self): - """If the application has an unhandled error the instrumentation should capture that a 500 response is returned.""" - try: - self._client.get("/error") - except UnhandledException: - pass - else: - self.fail("Expected UnhandledException") - - spans = self.memory_exporter.get_finished_spans() - self.assertEqual(len(spans), 3) - for span in spans: - self.assertIn("GET /error", span.name) - self.assertEqual( - span.attributes[SpanAttributes.HTTP_ROUTE], "/error" - ) - self.assertEqual( - span.attributes[SpanAttributes.HTTP_STATUS_CODE], 500 - ) - def test_fastapi_excluded_urls_not_env(self): """Ensure that given fastapi routes are excluded when passed explicitly (not in the environment)""" app = self._create_app_explicit_excluded_urls() @@ -1010,6 +1009,10 @@ async def _(param: str): async def _(): return {"message": "ok"} + @app.get("/error") + async def _(): + raise UnhandledException("This is an unhandled exception") + app.mount("/sub", app=sub_app) return app From d57b67305a824c0d37a4d4ec7601894100e5faba Mon Sep 17 00:00:00 2001 From: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com> Date: Wed, 4 Dec 2024 09:39:17 -0600 Subject: [PATCH 22/22] fmt --- .../tests/test_fastapi_instrumentation.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py index 0c2ce97cb6..3a84bc067c 100644 --- a/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py +++ b/instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py @@ -238,7 +238,9 @@ def test_fastapi_unhandled_exception(self): """If the application has an unhandled error the instrumentation should capture that a 500 response is returned.""" try: resp = self._client.get("/error") - assert resp.status_code == 500, resp.content # pragma: no cover, for debugging this test if an exception is _not_ raised + assert ( + resp.status_code == 500 + ), resp.content # pragma: no cover, for debugging this test if an exception is _not_ raised except UnhandledException: pass else: