Rewrite FastAPI instrumentor middleware stack to be failsafe #3664
Conversation
outergod
changed the title
| try: | ||
| func(*args, **kwargs) | ||
| except Exception: # pylint: disable=W0718 | ||
| pass |
There was a problem hiding this comment.
rather record this exception on the span
There was a problem hiding this comment.
@alexmojaki good point! Addressed in new commit.
| if isinstance( | ||
| inner_server_error_middleware, ServerErrorMiddleware | ||
| ): # usually true | ||
| outer_server_error_middleware = ServerErrorMiddleware( | ||
| app=otel_middleware, | ||
| ) |
There was a problem hiding this comment.
It's not clear to me why this is being removed or how this relates to the added tests. If you keep this, do the tests fail?
I'm guessing this is meant to fix #3637 but I don't see how this PR does or how that's being tested.
There was a problem hiding this comment.
It's not related to #3637 but you are correct, this should probably stay as-is. No effect on the tests either way. Addressed in commit.
| server_request_hook=failsafe(server_request_hook), | ||
| client_request_hook=failsafe(client_request_hook), | ||
| client_response_hook=failsafe(client_response_hook), |
There was a problem hiding this comment.
This wrapping should be in OpenTelemetryMiddleware itself.
There was a problem hiding this comment.
Addressed in commit.
| def failsafe(func): | ||
| @wraps(func) | ||
| def wrapper(span: Span, *args, **kwargs): | ||
| if func is not None: |
There was a problem hiding this comment.
| if func is not None: | |
| if func is not None: |
this should be checked outside wrapper
| # In order to make traces available at any stage of the request | ||
| # processing - including exception handling - we wrap ourselves as | ||
| # the new, outermost middleware. However in order to prevent | ||
| # exceptions from user-provided hooks of tearing through, we wrap |
There was a problem hiding this comment.
The wrapping is no longer here. I also don't think this comment is an improvement. It's now less clear than before that this approach ensures the correct http status code.
| ) | ||
| raise | ||
|
|
||
| app.add_middleware(ExceptionHandlerMiddleware) |
There was a problem hiding this comment.
I don't actually know where this middleware is going to go, it'd be clearer if this was inserted in build_middleware_stack.
| def test_error_handler_context(self): | ||
| """OTEL tracing contexts must be available during error handler execution""" | ||
|
|
||
| @self.app.exception_handler(Exception) |
There was a problem hiding this comment.
Please test if the exception and http status code are recorded on the span in this case.
|
|
||
| self.assertEqual(len(spans), 3) | ||
| span = spans[2] | ||
| self.assertEqual(span.status.status_code, StatusCode.ERROR) |
There was a problem hiding this comment.
please also check the http status code on the span.
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 #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: