Make app field optional in Flask, Starlette and FastAPI instrumentations #1228
Conversation
Deploying logfire-docs with
|
| Latest commit: |
8e22479
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://566fc505.logfire-docs.pages.dev |
| Branch Preview URL: | https://kludex-feat-no-need-for-app.logfire-docs.pages.dev |
| @@ -48,7 +48,7 @@ def find_mounted_apps(app: FastAPI) -> list[FastAPI]: | |||
|
|
|||
| def instrument_fastapi( | |||
| logfire_instance: Logfire, | |||
| app: FastAPI, | |||
| app: FastAPI | None = None, | |||
There was a problem hiding this comment.
Comment about how this differs from public signature
There was a problem hiding this comment.
Note that
Logfire.instrument_fastapirequires this argument. It's only omitted when called via thelogfire runCLI. This is becauseFastApiInstrumentor.instrument()has to be called beforefrom fastapi import FastAPIwhich is easy to get wrong.
| **opentelemetry_kwargs, | ||
| ) | ||
| if app is None: | ||
| FastAPIInstrumentor().instrument( |
There was a problem hiding this comment.
If it seems too hard to also apply the other fastapi instrumentation, at least comment about it
There was a problem hiding this comment.
I don't have the app instance, how would I go about it to make the custom logic work?
There was a problem hiding this comment.
if request.app not in registry then check for a 'global' FastAPIInstrumentation next.
@alexmojaki As I told you, this only works if instrumentation happens before import.