Skip to content

Don't pass bounded server_request_hook when using FastAPIInstrumentor.instrument(server_request_hook=...)#3701

Merged
xrmx merged 4 commits intoopen-telemetry:mainfrom
Kludex:do-not-pass-bound-method-to-InstrumentedFastAPI
Sep 10, 2025
Merged

Don't pass bounded server_request_hook when using FastAPIInstrumentor.instrument(server_request_hook=...)#3701
xrmx merged 4 commits intoopen-telemetry:mainfrom
Kludex:do-not-pass-bound-method-to-InstrumentedFastAPI

Conversation

@Kludex
Copy link
Copy Markdown
Member

@Kludex Kludex commented Aug 20, 2025

You can reproduce the issue with the following:

import logfire

logfire.configure()

from opentelemetry.instrumentation.fastapi import FastAPIInstrumentor

def server_request_hook(span, scope):
    print('server_request_hook', span, scope)

FastAPIInstrumentor().instrument(server_request_hook=server_request_hook)

from fastapi import FastAPI

app = FastAPI()


@app.get('/')
def read_root():
    return {'message': 'Hello, World!'}

if __name__ == '__main__':
    import uvicorn

    uvicorn.run(app, host='0.0.0.0', port=8000)

If you call localhost:8000 after running with python main.py, you'll see an exception. The exception happens because of an issue that was introduced on #3012 (using self._server_request_hook instead of _InstrumentedFastAPI._server_request_hook).

This PR simplifies the logic, and fixes the issue above.

@Kludex
Copy link
Copy Markdown
Member Author

Kludex commented Aug 20, 2025

cc @xrmx @emdneto 🙏

Copy link
Copy Markdown
Member

@emdneto emdneto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"})

@Kludex
Copy link
Copy Markdown
Member Author

Kludex commented Aug 21, 2025

Yeah, that's the case. 👍

@Kludex
Copy link
Copy Markdown
Member Author

Kludex commented Aug 21, 2025

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 .git-blame-ignore-revs file after to ignore that commit.

Most of the project is very lengthy.

@Kludex
Copy link
Copy Markdown
Member Author

Kludex commented Aug 21, 2025

How changing a function in a test below changed the result of a test above?

Copy link
Copy Markdown
Member

@emdneto emdneto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kludex mind if I push some changes to your branch? I think I managed to get it working

@Kludex
Copy link
Copy Markdown
Member Author

Kludex commented Sep 5, 2025

@Kludex mind if I push some changes to your branch? I think I managed to get it working

vlw mano, fica a vtd

Signed-off-by: emdneto <[email protected]>
Signed-off-by: emdneto <[email protected]>
@emdneto emdneto requested a review from a team as a code owner September 9, 2025 23:51
@emdneto
Copy link
Copy Markdown
Member

emdneto commented Sep 9, 2025

@Kludex mind if I push some changes to your branch? I think I managed to get it working

vlw mano, fica a vtd

@Kludex da uma olhada ai dps. vou precisar desse fix

@xrmx xrmx moved this from Ready for review to Approved PRs in Python PR digest Sep 10, 2025
@xrmx xrmx merged commit b2c3c4e into open-telemetry:main Sep 10, 2025
632 checks passed
@github-project-automation github-project-automation Bot moved this from Approved PRs to Done in Python PR digest Sep 10, 2025
@Kludex
Copy link
Copy Markdown
Member Author

Kludex commented Sep 10, 2025

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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants