Explicitly initialize Task SDK Stats in API server lifespan (hardening; backport fix for 3.2.x)#68078
Conversation
…e emitted The API server serves the Edge Worker REST API (/edge_worker/v1/...) whose heartbeat handler records edge_worker.* metrics through the Task SDK Stats singleton (resolved by the Edge provider via airflow.providers.common.compat). Unlike the scheduler, triggerer, dag-processor, executors and task runner, the API server never called Stats.initialize(...). After the auto-initializing Stats was removed in apache#63932, that singleton stays a NoStatsLogger in the API server process and every Edge Worker metric is silently dropped. Initialize the Task SDK Stats singleton from the FastAPI lifespan (runs once per worker, post-fork), mirroring the existing init in serde/task_runner. The call is guarded so a metrics misconfiguration can never block API server startup. Closes: apache#68077 Signed-off-by: Diogo Silva <diogo.silva@five9.com>
Signed-off-by: Diogo Silva <diogo.silva@five9.com>
|
Some interesting follow-up findings here: #68077 (comment) |
|
Looks good, but there is another parallel PR in #64523 which adds API server metrics in general - is this PR redundant if the other is merged? Still needed? |
|
Hey @jscheffl Not redundant, I'd say it's actually complementary and something of a prerequisite for #64523. After more investigation the situation turned out to be more nuanced than the original PR description suggested, so I've updated the PR body accordingly. On On |
jscheffl
left a comment
There was a problem hiding this comment.
General makes sense and looks good to me - would propose to make this as a safety net as described. The current "enablement" via the description really looks brittle and some refactoring (actually planned as well) might break it apart.
One safety question... if the reason for the bad init in Edge is the remobal of DualStatsMgr and the usage of the Stats in common-compat - would it also be a fix to add such handling in common-compat such that both core as well as edge can stay "naive" and the common compat bridges the gap? Then also a upgrade of common-compat provider would be a fix to make metrics in Airflow 3.2.x working again knowing that there will not be a next 3.2.x release.
|
Note: Static checks seems to be not related to this PR. |
|
@jscheffl Good point. A common-compat bridge could actually fix 3.2.x retroactively, which this PR can't since there won't be another 3.2.x release. I'd still keep the init here though. Every long-running component (scheduler, triggerer, dag-processor, executors, task-runner) already calls So I see them as complementary. This is the clean fix going forward, and if we want to help existing 3.2.x users I'm happy to open a follow-up for a targeted common-compat change. |
|
Note: If comments adjusted and no objections would propose to merge to get this into 3.3.0 - and as ^^^an improvement in common-compat can be made irrespective in a parallel PR. |
Co-authored-by: Jens Scheffler <95105677+jscheffl@users.noreply.github.com>
Co-authored-by: Jens Scheffler <95105677+jscheffl@users.noreply.github.com>
|
@jscheffl This PR makes no sense to me. It's talking about Task SDK, but it doesn't import anything from in task SDK (nor should it). Task SDK code should not run in the APi Server. Anything relying on that is fundamentally broken and breaking boundaries. If that is the reason this change is introduced then this should be reverted. #68481 |
Sorry, it seems I was not reading through in detail. In earlier commits that was referring to "Edge stats sent via API server not recorded" and I requested to remove Edge because this was a general shortcoming that API server did not properly initialize stats. Somehow I over-looked the "Task SDK" term. No of course Task SDK is not running in API server and this PR does not change this. But workding is still wrong. Let me have a corrective PR. Will do this in the afternoon. Note it is about commit message, exception text and comments, yes should be corrected. But still the general stats initialization was missing. Positively speaking you can read it "if Task SDK is sending something that should be recorded in stats via API server it was missing" (which was the case of Edge in 3.2). |
…g; backport fix for 3.2.x) (apache#68078) * Initialize Task SDK Stats in the API server so Edge Worker metrics are emitted The API server serves the Edge Worker REST API (/edge_worker/v1/...) whose heartbeat handler records edge_worker.* metrics through the Task SDK Stats singleton (resolved by the Edge provider via airflow.providers.common.compat). Unlike the scheduler, triggerer, dag-processor, executors and task runner, the API server never called Stats.initialize(...). After the auto-initializing Stats was removed in apache#63932, that singleton stays a NoStatsLogger in the API server process and every Edge Worker metric is silently dropped. Initialize the Task SDK Stats singleton from the FastAPI lifespan (runs once per worker, post-fork), mirroring the existing init in serde/task_runner. The call is guarded so a metrics misconfiguration can never block API server startup. Closes: apache#68077 Signed-off-by: Diogo Silva <diogo.silva@five9.com> * Add newsfragment for apache#68078 Signed-off-by: Diogo Silva <diogo.silva@five9.com> * fix: fix ruff errors * Remove newsfragment — bug fix does not require a user-facing changelog entry * Make API server Stats init log and docstring metric-agnostic * Update airflow-core/src/airflow/api_fastapi/app.py Co-authored-by: Jens Scheffler <95105677+jscheffl@users.noreply.github.com> * Update airflow-core/tests/unit/api_fastapi/test_app.py Co-authored-by: Jens Scheffler <95105677+jscheffl@users.noreply.github.com> --------- Signed-off-by: Diogo Silva <diogo.silva@five9.com> Co-authored-by: Jens Scheffler <95105677+jscheffl@users.noreply.github.com>
…g; backport fix for 3.2.x) (apache#68078) * Initialize Task SDK Stats in the API server so Edge Worker metrics are emitted The API server serves the Edge Worker REST API (/edge_worker/v1/...) whose heartbeat handler records edge_worker.* metrics through the Task SDK Stats singleton (resolved by the Edge provider via airflow.providers.common.compat). Unlike the scheduler, triggerer, dag-processor, executors and task runner, the API server never called Stats.initialize(...). After the auto-initializing Stats was removed in apache#63932, that singleton stays a NoStatsLogger in the API server process and every Edge Worker metric is silently dropped. Initialize the Task SDK Stats singleton from the FastAPI lifespan (runs once per worker, post-fork), mirroring the existing init in serde/task_runner. The call is guarded so a metrics misconfiguration can never block API server startup. Closes: apache#68077 Signed-off-by: Diogo Silva <diogo.silva@five9.com> * Add newsfragment for apache#68078 Signed-off-by: Diogo Silva <diogo.silva@five9.com> * fix: fix ruff errors * Remove newsfragment — bug fix does not require a user-facing changelog entry * Make API server Stats init log and docstring metric-agnostic * Update airflow-core/src/airflow/api_fastapi/app.py Co-authored-by: Jens Scheffler <95105677+jscheffl@users.noreply.github.com> * Update airflow-core/tests/unit/api_fastapi/test_app.py Co-authored-by: Jens Scheffler <95105677+jscheffl@users.noreply.github.com> --------- Signed-off-by: Diogo Silva <diogo.silva@five9.com> Co-authored-by: Jens Scheffler <95105677+jscheffl@users.noreply.github.com>
closes: #68077
Why
The API server serves the Edge Worker REST API (
/edge_worker/v1/...). A worker heartbeat (PATCH /edge_worker/v1/worker/) runsset_state→set_metrics, which recordsedge_worker.*metrics through the Task SDKStatssingleton (resolved by the Edge provider viaairflow.providers.common.compat).On
main: hardening against a fragile import side effectThe bug does not reproduce on current
main. Onmain,task_runner.pyimportsserdeat module level, which triggersserde._register()→Stats.initialize(...)as a side effect. That fires on anyimport airflow— including in the API server — soStatsgets initialized accidentally.However, this is fragile: it would silently regress if the module-level
serdeimport intask_runnerwere ever made lazy, or if the mask-secrets import chain changed. The API server doesn't actually depend onserde, so relying on this side effect is not intentional.Adding an explicit
Stats.initialize(...)in the FastAPIlifespanmakes the dependency intentional and self-contained. It's a no-op onmain(already initialized) and removes the accidental coupling.On
3.2.x: real bug fixOn 3.2.x,
task_runnerimportsserdelazily (inside functions), so the accidentalStats.initializeside effect never fires in the API server process. As a result, the Task SDKStatssingleton stays aNoStatsLoggerand every Edge Worker metric is silently dropped.This explains why
api_server.*metrics still work on 3.2.x (they use the separately-initialized core stats path) while alledge_worker.*metrics vanish.A backport of this change to the 3.2.x stable branch is the actual user-visible fix.
What
Initialize the Task SDK
Statssingleton from the FastAPIlifespan(runs once per worker, post-fork), mirroring the existing explicit init in other long-running components (scheduler_job_runner.py,triggerer_job_runner.py,dag_processing/manager.py,executors/base_executor.py,task_runner.py). The call is guarded so a metrics misconfiguration can never block API server startup.How verified
airflow-core/tests/unit/api_fastapi/test_app.py: assertslifespaninitializes Task SDKStatswith the configured factory, and that an init failure is swallowed (startup not blocked).edge33.7.0 deployment: before the fixStats.instanceisNoStatsLoggerand noedge_worker.*series export; after the fix 200+edge_worker.*series export, correctly tagged withworker_name.mainwithbreeze start-airflow --executor EdgeExecutor --integration statsd:edge_worker.*series present with and without the lifespan init (confirmingmainis not broken; the explicit init becomes a no-op).Relationship to #67328
Not a duplicate. #67328 reintroduces a
DualStatsManagercode path in edge3'sset_metrics, guarded bytry: from airflow.sdk.observability.stats import DualStatsManager.DualStatsManagerwas removed in #63932, so on any Airflow that includes #63932 (i.e. 3.2+) that import fails and edge3 falls back to the existingStats.gauge(..., tags={"worker_name": ...})path. Neither branch of #67328 callsStats.initialize(), so Edge Worker metrics on 3.2.x still depend on this PR.Relationship to #64523
Not redundant. #64523 adds
HttpMetricsMiddlewarethat emits REST API request metrics (api.requests,api.request.duration,api.request.errors) using theStatssingleton. If this PR is merged first, those metrics flow through onmainregardless (due to the accidental init). But on 3.2.x or if the accidental import side effect ever regresses onmain, #64523's metrics would also silently drop — making this an implicit prerequisite for #64523 as well.Was generative AI tooling used to co-author this PR?
Generated-by: GitHub Copilot (Claude Sonnet 4.6) following the guidelines