Repository navigation
Add REST API metrics - #64523
Add REST API metrics#64523
Conversation
4233a57 to
75bbd10
Compare
75bbd10 to
d18b47b
Compare
7fa1d0b to
33604c0
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds basic REST API request instrumentation to Airflow’s FastAPI/Starlette stack by emitting standardized metrics via Airflow’s existing Stats abstraction, enabling QPS/latency/error monitoring for /api/v2 and /ui traffic.
Changes:
- Added
api.requests,api.request.duration, andapi.request.errorsmetric definitions to the shared metrics template. - Implemented metric emission in
HttpAccessLogMiddlewarewith bounded tags (api_surface,method,route,status_code,status_family) and guarded failure handling. - Added unit tests validating metric emission behavior for API/UI paths, health checks, non-API paths, failures, and unmatched routes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| shared/observability/src/airflow_shared/observability/metrics/metrics_template.yaml | Registers new API request metrics in the shared metrics catalog. |
| airflow-core/src/airflow/api_fastapi/common/http_access_log.py | Emits API metrics from the access-log middleware after request completion. |
| airflow-core/tests/unit/api_fastapi/common/test_http_access_log.py | Adds unit tests asserting correct metric emission and tagging behavior. |
33604c0 to
af907c0
Compare
pierrejeambrun
left a comment
There was a problem hiding this comment.
Cool that looks promising.
A few comments before we can move forward.
Would love another pair of eyes. (@jedcunningham?)
af907c0 to
719146c
Compare
|
Also cardinality can be a problem and we need a futher understanding of the impact of such metrics. I believe last time I checked it was around 60k time series introduced, which seems too much. |
eb855b2 to
6ca8a2b
Compare
|
ready for review :) |
edb9715 to
20e0118
Compare
There was a problem hiding this comment.
Thanks for the big rework here. The PR description no longer matches the metrics that actually ship — could you update it (and #56034)?
A few suggestions bellow but we're getting close :)
20e0118 to
1e8d17a
Compare
ColtenOuO
left a comment
There was a problem hiding this comment.
I like this idea!
I've reviewed it and left some suggestions in the inline comments for your reference.
There was a problem hiding this comment.
LGTM in regards to the dev mailing discussion, @ashb can you take a final look and reconsider your request for change.
(Needs rebase though)
|
Might (as a follow up) be worth thinking about how to configure FastAPI OTel metrics too |
|
@henry3260 Sorry for the epic delay, just given this a re-review. PTAL |
|
The FastAPI framework has hooks that allow you to specify the provider used for metrics/logs/traces here. You should be able to wrap |
Remove the configurable path-to-surface mapping and emit metrics only for core API and UI routes. Use normalized route templates with method and status family tags to keep metric cardinality bounded.
Stats.timing records milliseconds on every supported backend, so publishing the timer as http_request_duration_seconds would have left every consumer of the metric wrong by a factor of 1000. Route tagging also kept a fallback that reverse-looked-up the endpoint in scope["router"].routes whenever scope["route"] was absent. Only FastAPI's APIRoute populates scope["route"], and all 151 routes under /api/v2 and /ui are APIRoute, so that branch was unreachable. For a mounted sub-app -- the one case it was meant to cover -- scope["router"] stays the root router, so the lookup could not have succeeded there either. The tests built a plain Starlette app, which never sets scope["route"], and so covered only that dead branch instead of the path the API server actually takes.
The router rejects a method mismatch with 405 before any authentication dependency runs, and uvicorn's h11 parser accepts any token as a method. That let an anonymous client create a new metric series per request just by inventing method names, defeating the bounded-cardinality goal of these metrics. Only the five methods registered on API routes now get their own tag value; everything else is recorded as OTHER, so the series count is fixed by the route table rather than by whatever clients send.
`/execution` carries every task heartbeat, state transition and XCom, making it the busiest surface the API server has, and it emitted no metrics at all. Two things kept it out. The middleware was registered from `init_middlewares`, which `create_app` only calls for the core app, so `airflow api-server --apps execution` -- the shape used to run the execution API as its own deployment -- installed nothing, and no configuration could recover the metrics. The path prefix allowlist also covered only `/api/v2` and `/ui`. Install the middleware for every `--apps` selection from a new `init_api_metrics`, the shape apache#72026 settled on after the identical gap left execution-only deployments with no access logs, and add `/execution` to the allowlist. Registering it on the outer app rather than inside the execution sub-app keeps a single instance, so a request cannot be counted twice, and keeps it outside `GZipMiddleware` so execution API durations include compression time the way core API durations already do. A mounted sub-app declares its routes relative to the mount point, so the route tag now carries `root_path`. Without it, `/execution/task-instances/{id}/run` would be tagged `/task-instances/{id}/run` and collide with any core route sharing that template.
Review feedback: every test rebuilt the whole FastAPI app just to issue one request, so the app construction cost was paid once per test case for no benefit. A module-scoped fixture is enough -- the middleware under test holds no state, and each test still gets its own client.
Review feedback: the metric tables promised a breakdown by method, route and status_family without saying that classic StatsD drops tags, so an operator reading the docs would configure statsd_on and then wonder why they only ever see one aggregate series.
|
Added. The prefix is Adding the prefix alone wasn't enough: Kept to a single instance on the outer app: it already sees execution requests, so a second one inside the sub-app would double-count and would sit inside |
|
Agreed it's worth doing, but I'd rather not fold it into this PR — the |
|
@ashb Could I get your final review on this? |
|
I moved it to 3.5.0 so we have time to test it and iterate on it before it's released. (3.4.0 is being cut now and this would feel rushed to me). Merging, feel free to follow up on the above points or if Ash adds more feedback. |
|
Thanks @henry3260 🎉 |
What
Adds REST API metrics using Airflow's existing
Statsabstraction.A new
HttpMetricsMiddleware(airflow/api_fastapi/common/http_metrics.py) emits twometrics for every completed request under
/api/v2and/ui:http_requests_totalhttp_request_duration_millisecondsBoth carry the same low-cardinality tags:
method— the HTTP method; anything other than GET / POST / PUT / PATCH / DELETE is bucketed asOTHERroute— the FastAPI route template (e.g./api/v2/dags/{dag_id}), orunmatchedwhen nothing matched (404s, redirects)
status_family—2xx/4xx/5xxTagging by route template rather than the resolved path is what keeps the series count
bounded, and error rates are derived from
status_familyinstead of a separate errorsmetric for the same reason.
The middleware is only registered when a metrics backend is configured (
statsd_on,statsd_datadog_enabledorotel_on), so deployments without metrics pay nothing forit. It sits inside
HttpAccessLogMiddlewareand outsideGZipMiddlewareso the recordedlatency covers the full request lifecycle, including compression.
closes: #56034
Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.Important
🛠️ Maintainer triage note for @henry3260 · by
@potiuk· 2026-06-22 06:31 UTCYour review threads from
@ashblook addressed — please confirm this PR is ready for maintainer review confirmation:The ball is in your court — you've been assigned to this PR. Reply
yes / ready(and mark the threads resolved) and a maintainer will pick it up from the queue.Automated triage — may be imperfect; a maintainer takes the next look.