Don't pass bounded server_request_hook when using FastAPIInstrumentor.instrument(server_request_hook=...)#3701
Merged
xrmx merged 4 commits intoopen-telemetry:mainfrom Sep 10, 2025
Conversation
…tor.instrument(server_request_hook=...)`
Member
Author
emdneto
reviewed
Aug 21, 2025
Member
emdneto
left a comment
There was a problem hiding this comment.
So, the current test_hooks is probably masking the issue since we are passing bounded hooks already. Wdyt of modify the test to something like this or add new one with that:
class TestFastAPIManualInstrumentationHooks(TestBaseManualFastAPI):
def _create_app(self):
def server_request_hook(span, scope):
span.update_name("name from server hook")
def client_request_hook(receive_span, scope, message):
receive_span.update_name("name from client hook")
receive_span.set_attribute("attr-from-request-hook", "set")
def client_response_hook(send_span, scope, message):
send_span.update_name("name from response hook")
send_span.set_attribute("attr-from-response-hook", "value")
self._instrumentor.instrument(
server_request_hook=server_request_hook,
client_request_hook=client_request_hook,
client_response_hook=client_response_hook,
)
app = self._create_fastapi_app()
return app
def test_hooks(self):
self._client.get("/foobar")
spans = self.sorted_spans(self.memory_exporter.get_finished_spans())
self.assertEqual(len(spans), 3)
server_span = spans[2]
self.assertEqual(server_span.name, "name from server hook")
response_spans = spans[:2]
for span in response_spans:
self.assertEqual(span.name, "name from response hook")
self.assertSpanHasAttributes(span, {"attr-from-response-hook": "value"})
Member
Author
|
Yeah, that's the case. 👍 |
Member
Author
|
I can't join the meeting today, but it will be nice if the line-length of this project changes to 120. We could add a Most of the project is very lengthy. |
Member
Author
|
How changing a function in a test below changed the result of a test above? |
6 tasks
Member
Author
vlw mano, fica a vtd |
Signed-off-by: emdneto <[email protected]>
Signed-off-by: emdneto <[email protected]>
Member
emdneto
approved these changes
Sep 9, 2025
xrmx
approved these changes
Sep 10, 2025
Member
Author
|
Thanks |
sightseeker
added a commit
to sightseeker/opentelemetry-python-contrib
that referenced
this pull request
Mar 11, 2026
…tor.instrument(server_request_hook=...)` (open-telemetry#3701) * Don't pass bounded `server_request_hook` when using `FastAPIInstrumentor.instrument(server_request_hook=...)` * try now * changelog Signed-off-by: emdneto <[email protected]> --------- Signed-off-by: emdneto <[email protected]> Co-authored-by: emdneto <[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
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.
You can reproduce the issue with the following:
If you call
localhost:8000after running withpython main.py, you'll see an exception. The exception happens because of an issue that was introduced on #3012 (usingself._server_request_hookinstead of_InstrumentedFastAPI._server_request_hook).This PR simplifies the logic, and fixes the issue above.