fix(fastapi): support instrumenting middleware-wrapped applications#4729
Open
kshitizj03 wants to merge 3 commits into
Open
fix(fastapi): support instrumenting middleware-wrapped applications#4729kshitizj03 wants to merge 3 commits into
kshitizj03 wants to merge 3 commits into
Conversation
FastAPIInstrumentor.instrument_app/uninstrument_app assumed they were handed a fastapi.FastAPI instance and raised AttributeError when given an application wrapped in ASGI middleware such as CORSMiddleware(app=fastapi_app), because the wrapper has no build_middleware_stack method. Add a _unwrap_middleware helper that follows the conventional `.app` attribute chain down to the underlying Starlette/FastAPI application, and skip gracefully with a warning when none is found. Widen the public app parameter annotation to ASGIApp to reflect that wrapped apps are now accepted. The unwrap stop-condition and guards check Starlette rather than fastapi.FastAPI because the latter is monkeypatched to a subclass while global instrumentation is active, which would make pre-existing app instances fail an isinstance check. Builds on the approach and maintainer review feedback from the previously abandoned PR open-telemetry#4041 by @punitmahes. Fixes open-telemetry#4031 Assisted-by: Claude Opus 4.8
towncrier uses the fragment filename as the changelog reference, so name it after the PR (open-telemetry#4729) rather than the issue. Assisted-by: Claude Opus 4.8
The non-FastAPI uninstrument test referenced no instance state, tripping pylint R6301 (no-self-use) in CI. Assert that uninstrument_app on a non-Starlette app does not attach _original_build_middleware_stack, which both uses self and strengthens the test. Assisted-by: Claude Opus 4.8
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
FastAPIInstrumentor.instrument_app/uninstrument_appassumed they were handed afastapi.FastAPIinstance and raisedAttributeError: 'CORSMiddleware' object has no attribute 'build_middleware_stack'when given an application wrapped in ASGI middleware, e.g.CORSMiddleware(app=fastapi_app).This adds a
_unwrap_middlewarehelper that follows the conventional.appattribute chain down to the underlying application, and skips gracefully with a warning when no app is found. The publicappparameter annotation is widened toASGIAppto reflect that wrapped apps are now accepted.Note: the unwrap stop-condition and guards check
Starletterather thanfastapi.FastAPI.fastapi.FastAPIis monkeypatched to the_InstrumentedFastAPIsubclass while global instrumentation is active, so gating onfastapi.FastAPIwould make pre-existing app instances fail theisinstancecheck during that window.Builds on the approach and review feedback from the now-stale #4041 (@punitmahes), with @xrmx's notes applied (stop unwrapping at the app instance; changelog under Fixed) and the parameter typing he flagged addressed.
Fixes #4031
Type of change
How Has This Been Tested?
CORSMiddleware-wrapped app and asserting theemitted SERVER span name andhttp.routeattribute (name and value type)._unwrap_middleware(bare app, single/nested middleware, FastAPIinstance with an unrelatedappattribute, and the no-app-found case) and for the skip path when no FastAPI application is found.opentelemetry-instrumentation-fastapisuite against both the oldest(fastapi==0.119.1/starlette==0.48.0) and latest (fastapi==0.137.2/starlette==1.3.1) pinned versions, all passing.ruff checkandruff format --checkclean.Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.