Rewrite FastAPI instrumentor middleware stack to be failsafe#3664
Merged
xrmx merged 39 commits intoopen-telemetry:mainfrom Aug 28, 2025
Merged
Rewrite FastAPI instrumentor middleware stack to be failsafe#3664xrmx merged 39 commits intoopen-telemetry:mainfrom
xrmx merged 39 commits intoopen-telemetry:mainfrom
Conversation
outergod
changed the title
This was referenced Aug 5, 2025
alexmojaki
reviewed
Aug 8, 2025
alexmojaki
suggested changes
Aug 15, 2025
alexmojaki
reviewed
Aug 18, 2025
Also improve code documentation and add another test.
alexmojaki
reviewed
Aug 18, 2025
Co-authored-by: Alex Hall <[email protected]>
Co-authored-by: Alex Hall <[email protected]>
alexmojaki
approved these changes
Aug 19, 2025
Contributor
alexmojaki
left a comment
alexmojaki
left a comment
There was a problem hiding this comment.
Looks solid, thank you so much!
Contributor
Author
|
Thank you for the thorough review and the persistence @alexmojaki! |
Contributor
emdneto
reviewed
Aug 25, 2025
Member
There was a problem hiding this comment.
Right. I gave it a try to run the repro and now I can see the recorded exception. Overall, it sounds good.
If it helps to others to review, before we had:
ServerErrorMiddleware (outermost) -> OpenTelemetryMiddleware -> ServerErrorMiddleware (innermost)
now we have:
ServerErrorMiddleware (outer -- same as before) -> OpenTelemetryMiddleware -> ServerErrorMiddleware (with original handler/debug) -> ExceptionHandlerMiddleware (with access to the span context)
But I'm afraid we are not seeing some issues in the current structure of tests for FastAPI. Noticed that while reviewing #3701
emdneto
reviewed
Aug 26, 2025
emdneto
reviewed
Aug 28, 2025
emdneto
approved these changes
Aug 28, 2025
github-project-automation
Bot
moved this from Ready for review
to Done
in Python PR digest
zhirafovod
pushed a commit
to zhirafovod/opentelemetry-python-contrib
that referenced
this pull request
Sep 15, 2025
…lemetry#3664) * rewrite FastAPIInstrumentor:build_middleware_stack to become failsafe * add test cases for FastAPI failsafe handling * add CHANGELOG entry * remove unused import * [lint] don't return from failsafe wrapper * [lint] allow broad exceptions * [lint] more allowing * record FastAPI hook exceptions in active span * remove comment * properly deal with hooks not being set * add custom FastAPI exception recording * move failsafe hook handling down to OpenTelemetryMiddleware * shut up pylint * optimize failsafe to check for `None` only once * remove confusing comment and simplify wrapper logic * add clarifying comment * test proper exception / status code recording * add HTTP status code check * test HTTP status on the exception recording span * improve test by removing TypeError * rectify comment/explanation on inner middleware for exception handling * minor typo * move ExceptionHandlingMiddleware as the outermost inner middleware Also improve code documentation and add another test. * use distinct status code in test * improve comemnt Co-authored-by: Alex Hall <[email protected]> * narrow down exception handling Co-authored-by: Alex Hall <[email protected]> * narrow down FastAPI exception tests to relevant spans * collapse tests, more narrow exceptions * move failsafe hook tests to ASGI test suite * update CHANGELOG * satisfy linter * don't record exception if span is not recording * add test for unhappy instrumentation codepath * make inject fixtures private * give up and shut up pylint * improve instrumentation failure error message and add test --------- Co-authored-by: Alex Hall <[email protected]>
8 tasks
sightseeker
added a commit
to sightseeker/opentelemetry-python-contrib
that referenced
this pull request
Mar 11, 2026
…lemetry#3664) * rewrite FastAPIInstrumentor:build_middleware_stack to become failsafe * add test cases for FastAPI failsafe handling * add CHANGELOG entry * remove unused import * [lint] don't return from failsafe wrapper * [lint] allow broad exceptions * [lint] more allowing * record FastAPI hook exceptions in active span * remove comment * properly deal with hooks not being set * add custom FastAPI exception recording * move failsafe hook handling down to OpenTelemetryMiddleware * shut up pylint * optimize failsafe to check for `None` only once * remove confusing comment and simplify wrapper logic * add clarifying comment * test proper exception / status code recording * add HTTP status code check * test HTTP status on the exception recording span * improve test by removing TypeError * rectify comment/explanation on inner middleware for exception handling * minor typo * move ExceptionHandlingMiddleware as the outermost inner middleware Also improve code documentation and add another test. * use distinct status code in test * improve comemnt Co-authored-by: Alex Hall <[email protected]> * narrow down exception handling Co-authored-by: Alex Hall <[email protected]> * narrow down FastAPI exception tests to relevant spans * collapse tests, more narrow exceptions * move failsafe hook tests to ASGI test suite * update CHANGELOG * satisfy linter * don't record exception if span is not recording * add test for unhappy instrumentation codepath * make inject fixtures private * give up and shut up pylint * improve instrumentation failure error message and add test --------- Co-authored-by: Alex Hall <[email protected]>
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
4 participants
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
Change the way the FastAPI instrumentor deals with the FastAPI middleware stack so that exception handling code doesn't get executed twice, but still has a valid OTEL context available. At the same time, make sure instrumentor hooks failures cannot crash the service itself.
Fixes #3642
Fixes #3637
Type of change
How Has This Been Tested?
Using the MRE in the linked issue, and added unit tests.
Does This PR Require a Core Repo Change?
Checklist:
Documentation has been updated(not needed)