-
Notifications
You must be signed in to change notification settings - Fork 766
Rewrite FastAPI instrumentor middleware stack to be failsafe #3664
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Rewrite FastAPI instrumentor middleware stack to be failsafe #3664
Conversation
.../opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py
Outdated
Show resolved
Hide resolved
.../opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py
Show resolved
Hide resolved
.../opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py
Outdated
Show resolved
Hide resolved
...tation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py
Outdated
Show resolved
Hide resolved
.../opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py
Outdated
Show resolved
Hide resolved
.../opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py
Outdated
Show resolved
Hide resolved
instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py
Show resolved
Hide resolved
instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py
Show resolved
Hide resolved
instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py
Outdated
Show resolved
Hide resolved
.../opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py
Outdated
Show resolved
Hide resolved
…H-3642-fastapi-exceptions
Also improve code documentation and add another test.
.../opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py
Outdated
Show resolved
Hide resolved
instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py
Outdated
Show resolved
Hide resolved
instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py
Outdated
Show resolved
Hide resolved
instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py
Outdated
Show resolved
Hide resolved
instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py
Outdated
Show resolved
Hide resolved
instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_instrumentation.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Alex Hall <alex.mojaki@gmail.com>
Co-authored-by: Alex Hall <alex.mojaki@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks solid, thank you so much!
Thank you for the thorough review and the persistence @alexmojaki! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.../opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py
Show resolved
Hide resolved
.../opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py
Show resolved
Hide resolved
span.set_status( | ||
Status( | ||
status_code=StatusCode.ERROR, | ||
description=f"{type(exc).__name__}: {exc}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to your PR I guess, but I can't see the description in the exported span. #3713
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you suggest I do?
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)