Skip to content

Add REST API metrics - #64523

Merged
pierrejeambrun merged 9 commits into
apache:mainfrom
henry3260:add-metrics-in-rest-api
Oct 13, 2026
Merged

pierrejeambrun merged 9 commits into
apache:mainfrom
henry3260:add-metrics-in-rest-api

Conversation

@henry3260

@henry3260 henry3260 commented Mar 31, 2026 •

Copy link
Copy Markdown
Member

What

Adds REST API metrics using Airflow's existing Stats abstraction.

A new HttpMetricsMiddleware (airflow/api_fastapi/common/http_metrics.py) emits two
metrics for every completed request under /api/v2 and /ui:

Metric Type Description
http_requests_total counter Number of completed REST API requests
http_request_duration_milliseconds timer Milliseconds taken to complete the request

Both carry the same low-cardinality tags:

  • method — the HTTP method; anything other than GET / POST / PUT / PATCH / DELETE is bucketed as OTHER
  • route — the FastAPI route template (e.g. /api/v2/dags/{dag_id}), or unmatched
    when nothing matched (404s, redirects)
  • status_family — 2xx / 4xx / 5xx

Tagging by route template rather than the resolved path is what keeps the series count
bounded, and error rates are derived from status_family instead of a separate errors
metric for the same reason.

The middleware is only registered when a metrics backend is configured (statsd_on,
statsd_datadog_enabled or otel_on), so deployments without metrics pay nothing for
it. It sits inside HttpAccessLogMiddleware and outside GZipMiddleware so the recorded
latency covers the full request lifecycle, including compression.

closes: #56034

Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)

  • Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
  • For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
  • When adding dependency, check compliance with the ASF 3rd Party License Policy.
  • For significant user-facing changes create newsfragment: {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 UTC

Your review threads from @ashb look addressed — please confirm this PR is ready for maintainer review confirmation:

  • 1 thread(s) show your engagement (post-review commits and/or replies).

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.

@boring-cyborg boring-cyborg Bot added the area:API Airflow's REST/HTTP API label Mar 31, 2026
@henry3260
henry3260 force-pushed the add-metrics-in-rest-api branch from 4233a57 to 75bbd10 Compare March 31, 2026 14:30
@henry3260

Copy link
Copy Markdown
Member Author

cc @pierrejeambrun

@henry3260
henry3260 force-pushed the add-metrics-in-rest-api branch from 75bbd10 to d18b47b Compare March 31, 2026 21:12
@henry3260
henry3260 marked this pull request as ready for review April 1, 2026 13:18
@potiuk potiuk added the ready for maintainer review Set by triage when all criteria pass, or by the fork CI reconciler when fork CI is green. label Apr 1, 2026
@henry3260
henry3260 force-pushed the add-metrics-in-rest-api branch from 7fa1d0b to 33604c0 Compare April 1, 2026 17:17
@kaxil
kaxil requested a review from Copilot April 2, 2026 00:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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, and api.request.errors metric definitions to the shared metrics template.
  • Implemented metric emission in HttpAccessLogMiddleware with 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.

@henry3260
henry3260 force-pushed the add-metrics-in-rest-api branch from 33604c0 to af907c0 Compare April 2, 2026 06:28

@pierrejeambrun pierrejeambrun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Cool that looks promising.

A few comments before we can move forward.

Would love another pair of eyes. (@jedcunningham?)

@pierrejeambrun pierrejeambrun added this to the Airflow 3.3.0 milestone Apr 3, 2026
@pierrejeambrun pierrejeambrun removed the ready for maintainer review Set by triage when all criteria pass, or by the fork CI reconciler when fork CI is green. label Apr 3, 2026
@potiuk
potiuk force-pushed the add-metrics-in-rest-api branch from af907c0 to 719146c Compare April 6, 2026 21:54
@pierrejeambrun

Copy link
Copy Markdown
Member

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.

@kaxil
kaxil requested a review from Copilot April 10, 2026 19:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

@henry3260
henry3260 force-pushed the add-metrics-in-rest-api branch 2 times, most recently from eb855b2 to 6ca8a2b Compare April 12, 2026 13:03
@henry3260

Copy link
Copy Markdown
Member Author

ready for review :)

@henry3260
henry3260 force-pushed the add-metrics-in-rest-api branch from edb9715 to 20e0118 Compare August 8, 2026 11:27

@pierrejeambrun pierrejeambrun left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 :)

@ColtenOuO ColtenOuO left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like this idea!

I've reviewed it and left some suggestions in the inline comments for your reference.

@pierrejeambrun pierrejeambrun left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM in regards to the dev mailing discussion, @ashb can you take a final look and reconsider your request for change.

(Needs rebase though)

@ColtenOuO ColtenOuO left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, Great Work!

@jason810496 jason810496 left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

BTW, I think this worths a #protm to share this feature in the Airflow Town Hall to the users when it get merged.
Nice feature, and it took a great effort on the community discussion to settle down the final shape.

@ashb

ashb commented Oct 1, 2026

Copy link
Copy Markdown
Member

Might (as a follow up) be worth thinking about how to configure FastAPI OTel metrics too
for those that want to use OTel https://fastapi.tiangolo.com/advanced/opentelemetry/#choose-what-to-record

@ashb

ashb commented Oct 1, 2026

Copy link
Copy Markdown
Member

@henry3260 Sorry for the epic delay, just given this a re-review. PTAL

@stuart23

stuart23 commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

The FastAPI framework has hooks that allow you to specify the provider used for metrics/logs/traces here.

You should be able to wrap airflow._shared.observability.metrics.stats.Stats into a compatible provider class rather than introducing a custom middleware. This will also produce metrics according to the FastAPI semconv, rather than having to maintain a separate convention.

henry3260 and others added 9 commits October 8, 2026 15:36
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.
@henry3260

Copy link
Copy Markdown
Member Author

Added. The prefix is /execution rather than /api/execution — API_ROOT_PATH defaults to /, so that mount point is the whole path.

Adding the prefix alone wasn't enough: --apps execution never reaches init_middlewares, so that deployment shape had no metrics middleware at all. Registration moved to init_api_metrics(app), called for every --apps
selection — the same shape #72026 used for the access log. The route tag now prepends root_path, since a mounted sub-app's routes are relative to its mount point.

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 GZipMiddleware, excluding compression time from execution durations only.

@henry3260

Copy link
Copy Markdown
Member Author

Agreed it's worth doing, but I'd rather not fold it into this PR — the telemetry= hook needs a newer FastAPI than we currently allow (fastapi<0.137.0, and the parameter doesn't exist in 0.136.3). Configuring FastAPI's OTel metrics was already raised as a follow-up above, which I think is the right place for it.

@henry3260

Copy link
Copy Markdown
Member Author

@ashb Could I get your final review on this?

@pierrejeambrun

Copy link
Copy Markdown
Member

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.

@pierrejeambrun

Copy link
Copy Markdown
Member

Thanks @henry3260 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:API Airflow's REST/HTTP API ready for maintainer review Set by triage when all criteria pass, or by the fork CI reconciler when fork CI is green.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add REST API metrics (latency, QPS, errors)

10 participants