diff --git a/src/instana/instrumentation/asgi.py b/src/instana/instrumentation/asgi.py index 7c420e9d..a15ffb92 100644 --- a/src/instana/instrumentation/asgi.py +++ b/src/instana/instrumentation/asgi.py @@ -5,7 +5,7 @@ Instana ASGI Middleware """ -from typing import TYPE_CHECKING, Any, Awaitable, Callable, Dict +from typing import TYPE_CHECKING, Any, Awaitable, Callable, Dict, Iterable, Iterator from opentelemetry.semconv.trace import SpanAttributes @@ -15,12 +15,45 @@ from instana.util.secrets import strip_secrets_from_query from instana.util.traceutils import extract_custom_headers +try: + from starlette.routing import Match +except ImportError: # pragma: no cover + Match = None + +try: + # FastAPI >= 0.137.2 + from fastapi.routing import iter_route_contexts +except ImportError: + iter_route_contexts = None + if TYPE_CHECKING: from starlette.middleware.exceptions import ExceptionMiddleware from instana.span.span import InstanaSpan +def _iter_routes(routes: Iterable[Any]) -> Iterator[Any]: + """ + Yield the routes of an app in a form that can be matched against a scope + and carries its full path template. + + FastAPI >= 0.137 keeps the routes added with include_router() in a tree + whose nodes have no path of their own, so the tree has to be flattened. + """ + if iter_route_contexts is not None: + # FastAPI >= 0.137.2 + yield from iter_route_contexts(routes) + return + + for route in routes: + if hasattr(route, "effective_route_contexts"): + # FastAPI 0.137.0 and 0.137.1 + yield from route.effective_route_contexts() + else: + # FastAPI < 0.137 and Starlette: the routes are already flat + yield route + + class InstanaASGIMiddleware: """ Instana ASGI Middleware @@ -48,14 +81,15 @@ def _collect_kvs(self, scope: Dict[str, Any], span: "InstanaSpan") -> None: span.set_attribute("http.params", scrubbed_params) app = scope.get("app") - if app and hasattr(app, "routes"): + if app and hasattr(app, "routes") and Match is not None: # Attempt to detect the Starlette routes registered. # If Starlette isn't present, we harmlessly dump out. - from starlette.routing import Match - - for route in scope["app"].routes: + for route in _iter_routes(app.routes): if route.matches(scope)[0] == Match.FULL: - span.set_attribute("http.path_tpl", route.path) + path_tpl = getattr(route, "path", None) + if path_tpl: + span.set_attribute("http.path_tpl", path_tpl) + break except Exception: logger.debug("ASGI collect_kvs: ", exc_info=True) diff --git a/tests/apps/fastapi_app/app.py b/tests/apps/fastapi_app/app.py index eac3662e..6bf9059b 100644 --- a/tests/apps/fastapi_app/app.py +++ b/tests/apps/fastapi_app/app.py @@ -1,7 +1,7 @@ # (c) Copyright IBM Corp. 2021 # (c) Copyright Instana Inc. 2020 -from fastapi import FastAPI, HTTPException, Response +from fastapi import APIRouter, FastAPI, HTTPException, Response from fastapi.concurrency import run_in_threadpool from fastapi.testclient import TestClient from starlette.exceptions import HTTPException as StarletteHTTPException @@ -79,3 +79,21 @@ def non_async_threadpool(): return { "message": "non async functions executed on a thread pool can't be followed through thread boundaries" } + + +included_router = APIRouter(prefix="/included") +nested_router = APIRouter(prefix="/nested") + + +@included_router.get("/users/{user_id}") +async def included_user(user_id): + return {"user": user_id} + + +@nested_router.get("/items/{item_id}") +async def nested_item(item_id): + return {"item": item_id} + + +included_router.include_router(nested_router) +fastapi_server.include_router(included_router) diff --git a/tests/frameworks/test_fastapi.py b/tests/frameworks/test_fastapi.py index 5707716e..ab62245b 100644 --- a/tests/frameworks/test_fastapi.py +++ b/tests/frameworks/test_fastapi.py @@ -350,6 +350,89 @@ def test_path_templates(self) -> None: ) assert http_entry_span.data["sdk"]["custom"]["tags"]["url.path"] == "/users/1" + @pytest.mark.parametrize( + "path, path_tpl", + [ + ("/included/users/1", "/included/users/{user_id}"), + ("/included/nested/items/1", "/included/nested/items/{item_id}"), + ], + ) + def test_path_templates_with_included_routers( + self, path: str, path_tpl: str + ) -> None: + result = self.client.get(path) + + assert result + assert result.status_code == 200 + + spans = self.recorder.queued_spans() + + # FastAPI >= 0.115.0 adds 4 extra OTel spans per request: + # fastapi.dependencies, fastapi.endpoint, fastapi.serialization, GET + assert len(spans) == 5 + + asgi_span = get_first_span_by_filter(spans, lambda span: span.n == "asgi") + assert asgi_span + + assert not asgi_span.ec + assert asgi_span.data["http"]["path"] == path + assert asgi_span.data["http"]["path_tpl"] == path_tpl + assert asgi_span.data["http"]["method"] == "GET" + assert asgi_span.data["http"]["status"] == 200 + + assert_fastapi_otel_spans(spans, path_tpl) + + def test_iter_routes_without_iter_route_contexts( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + # FastAPI 0.137.0 and 0.137.1 have no iter_route_contexts(), but the nodes + # created by include_router() can flatten themselves. Older versions and + # Starlette keep their routes flat. + import instana.instrumentation.asgi as asgi + + class IncludedRouterNode: + def __init__(self, contexts: list) -> None: + self._contexts = contexts + + def effective_route_contexts(self): + return iter(self._contexts) + + flat_route, nested_route = object(), object() + monkeypatch.setattr(asgi, "iter_route_contexts", None) + + routes = [flat_route, IncludedRouterNode([nested_route])] + assert list(asgi._iter_routes(routes)) == [flat_route, nested_route] + + def test_path_template_of_first_full_match_wins(self) -> None: + # Starlette serves the first route that fully matches, so a later route + # that also matches must not overwrite the path template. + from unittest.mock import MagicMock + + from starlette.routing import Match + + from instana.instrumentation.asgi import InstanaASGIMiddleware + + class StubRoute: + def __init__(self, path: str) -> None: + self.path = path + + def matches(self, scope): + return Match.FULL, {} + + app = MagicMock() + app.routes = [StubRoute("/items/{item_id}"), StubRoute("/items/{name}")] + span = MagicMock() + scope = {"path": "/items/1", "method": "GET", "app": app} + + InstanaASGIMiddleware(None)._collect_kvs(scope, span) + + path_tpl_calls = [ + call.args + for call in span.set_attribute.call_args_list + if call.args[0] == "http.path_tpl" + ] + assert path_tpl_calls == [("http.path_tpl", "/items/{item_id}")] + def test_secret_scrubbing(self) -> None: result = None with self.tracer.start_as_current_span("test") as span: