Repository navigation
Use FastAPI's native OpenTelemetry for HTTP instrumentation - #249
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
FastAPI 0.142 added native OpenTelemetry. This repo already passed a telemetry config, but logfire.instrument_fastapi added the contrib ASGI middleware, so FastAPI kept its own telemetry dormant. Drop the Logfire FastAPI instrumentation and let the native integration run on the providers that logfire.configure installs. - Request spans now use current semantic conventions and get fastapi.* child spans for dependencies, endpoint and serialization, plus http.server.request.duration and active-request metrics, and log events for unhandled exceptions and validation failures. - ServerSpanMiddleware sets client.address on the request span and keeps that span reachable, so record_auth_attributes tags the request rather than the fastapi.dependencies span. - Require fastapi>=0.143.0: 0.142.4 isolates excluded requests, and 0.143.0 makes exporter auto-configuration opt-in.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0f46daa8e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| + "|".join(re.escape(path) for path in HEALTH_ENDPOINT_PATHS) | ||
| + r")(?:\?.*)?$" | ||
| ) | ||
| _SERVER_SPAN_KEY = context.create_key("app.server_span") |
There was a problem hiding this comment.
Creating _SERVER_SPAN_KEY here generates and retains a process-global identity as soon as app.telemetry is imported, putting telemetry state outside the application/lifespan lifecycle and directly violating the repository's prohibition on module-level singletons. Move ownership of this key into an application-scoped component rather than initializing it during import.
AGENTS.md reference: AGENTS.md:L69-L69
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Partly applied in bd62cf1. The key is now the string literal "app.telemetry.server_span", so nothing runs at import time.
I kept it at module scope. It is an immutable constant name, not mutable state or a resource, so the AGENTS.md rule against module-level singletons does not apply, in the same way it does not apply to HEALTH_ENDPOINT_PATHS. FastAPI's own _REQUEST_TELEMETRY_KEY follows the same pattern. Moving the key into an application-scoped object would require passing that object into record_auth_attributes, which runs inside a dependency, and that would add plumbing without changing behavior.
claude-opus-5-5
| span = trace.get_current_span() | ||
| if scope["type"] not in ("http", "websocket") or not span.is_recording(): | ||
| await self.app(scope, receive, send) |
There was a problem hiding this comment.
When /health or /health/ready is dispatched in-process while another recording span is current, FastAPI excludes the probe and therefore creates no request span, but this condition still accepts the inherited span as the probe's server span. The middleware then overwrites that unrelated span's client.address and exposes it through _SERVER_SPAN_KEY, so excluded probes can mutate their caller's telemetry; check the same health-probe exclusion before reading or tagging the current span.
AGENTS.md reference: AGENTS.md:L52-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bd62cf1. ServerSpanMiddleware now applies the same health-probe check as native_telemetry_config() before it reads or tags the current span. A new unit test runs /health in-process under a caller span and asserts the caller span gets no client.address. It fails without the fix.
claude-opus-5-5
e0f46da to
b7543cb
Compare
FastAPI creates no request span for excluded probes, so an in-process probe would tag whatever span its caller had open. Apply the same health-probe check the native telemetry config uses. Name the OpenTelemetry context key with a string literal instead of calling create_key at import time.
|
) A failed aiosqlite connect queues its worker-thread shutdown without waiting for it (omnilib/aiosqlite#394). When the test's event loop closed first, the thread raised "Event loop is closed", and pytest reported it as an unhandled thread exception in the next test. This flaked test (3.13) on main after #249. Join the worker threads in the unreachable_settings fixture teardown, off the event loop, so the loop is still alive when the thread finishes.
1 participant
Summary
FastAPI 0.142.0 added native OpenTelemetry support (fastapi#16403). This repo already passed
telemetry=native_telemetry_config(), but the native layer never ran.logfire.instrument_fastapiadds the contribOpenTelemetryMiddleware, and FastAPI keeps its own telemetry dormant whenever that middleware is in the stack.This PR removes the Logfire FastAPI instrumentation so the native integration takes over. Logfire still owns the providers and exporters (
logfire.configure) and the SQLAlchemy instrumentation.What changes in the telemetry
GET /entities/{entity_id}withhttp.method,http.status_codehttp.request.method,http.response.status_code,url.path,server.*fastapi.dependencies,fastapi.endpoint,fastapi.serialization,fastapi.background_taskhttp.server.request.duration,http.server.active_requestshttp.server.request.exception(with stack trace),fastapi.validation.failed(error count and route)client.ipviarequest_attributes_mapperclient.address, set byServerSpanMiddlewareerrors(pydantic errors, which include input values)excludeon the ASGI pathCode
app/telemetry.pyinstrument_fastapi, the request-attributes mapper and the URL regex.ServerSpanMiddleware, a pure ASGI middleware. FastAPI runs dependencies inside afastapi.dependencieschild span, so the middleware stores the request span in the OpenTelemetry context.record_auth_attributesthen tags the request span withoidc.*, not the child span. It also setsclient.address.native_telemetry_config()is unchanged:auto_configurestays off so platformOTEL_*variables cannot add a second exporter.app/main.py: registers the middleware in place ofinstrument_fastapi.pyproject.toml:fastapi>=0.143.0. 0.142.4 isolates telemetry for excluded requests, and 0.143.0 makes exporter auto-configuration opt-in.AGENTS.mdandREADME.mddescribe the new ownership split.Tests
capfire. They check the request span (route, status,client.address,oidc.*), that auth attributes are not on the dependencies span, and that health probes produce no spans.tests/integration/test_otel_api.pynow uses the new attribute names. It still checks that no request body or bearer token reaches any exported attribute.make checkpasses, and integration tests pass on Postgres (74 passed).Opened by claude-opus-5-5