dsuhinin commented on PR #68393:
URL: https://github.com/apache/airflow/pull/68393#issuecomment-5093745951
> The consolidation is a genuine improvement and mirroring the traces shape
is the right direction — thanks for pushing this through so many rounds.
>
> Caveat up front: I have **not** run this locally, so treat all of this as
reading-the-diff impressions rather than verified behaviour. Most of it is
small stuff left inline. One thing does worry me though.
>
> **Removing the `lifespan` stats init.** `settings.initialize()` runs
exactly once, from `airflow/__init__.py:79`, at first `import airflow` — it is
_not_ re-run in a forked child. Every call site this PR removes was a post-fork
re-init, and the API server one looks like the exposed case: it runs gunicorn
with `"preload_app": True`
(`airflow-core/src/airflow/api_fastapi/gunicorn_app.py:276`), so `import
airflow` happens in the master and workers are forked from it. `lifespan` was
the only per-worker hook.
>
> `stats.initialize()` sets `_backend = None`, and that is what forces the
OTel `MeterProvider` — and its `PeriodicExportingMetricReader` background
thread — to be rebuilt in the child. That thread doesn't survive `fork()`. If
anything touches `Stats` during gunicorn preload, `_backend` gets built in the
master and every worker inherits a provider whose exporter thread is dead, with
nothing left to reset it. That's the same failure mode #64703 fixed, and
`_initialize_api_server_stats()` was added deliberately in #68514 for it.
>
> Centralising the _configuration_ in `settings.initialize()` seems right to
me — it's the per-process reset I think needs to stay at each fork boundary,
either as the explicit calls or an `os.register_at_fork` hook. Could you
sanity-check it against a multi-worker api-server before this goes in? It may
also be related to what @xBis7 reported today about task-SDK metrics reaching
Prometheus but querying empty, which is still open.
>
> The rest is inline — the traces exporter default change is the one I'd
most like a newsfragment for.
Thanks for the thorough read.
On the fork boundary reset: it does stay — it just doesn't live at the
call sites anymore. stats.py registers an
os.register_at_fork(after_in_child=...) hook at module level (added in #63932,
before this PR) that resets _backend in every forked child, so the first metric
emission in a worker rebuilds the backend
through the factory. The dead-exporter-thread case is handled too:
get_otel_logger() resets the OTel SDK's Once() guard before
set_meter_provider() (the #64690 fix), so the child installs a fresh
MeterProvider with a live PeriodicExportingMetricReader thread instead of
silently keeping the parent's stale one. The
calls this PR removes were redundant with that hook — the lifespan init
wasn't the only per-worker reset.
Sanity check, as requested: I ran the API server with gunicorn
(preload_app, 2 workers) and a test plugin that touches Stats at import time —
i.e. the backend and its exporter thread get built in the master before any
fork, which is the worst case you describe. The master exported its metric, and
after forking
both workers exported request-time metrics tagged with their own PIDs via
the console exporter. So the per-worker rebuild works end to end.
Test coverage: the at-fork reset had no test, so I added one — it forks a
real child and asserts the backend is reset and rebuilt fresh
(shared/observability/tests/.../test_stats.py::TestBackendResetAfterFork).
Newsfragment: added for the traces default transport change (otlp now
defaults to http/protobuf per the OTel spec instead of gRPC), including how to
restore the old behaviour via OTEL_EXPORTER_OTLP_PROTOCOL=grpc.
On the task-SDK metrics report — I don't think it's related to this reset
path (that's the supervisor/task runner side, not a gunicorn fork), but happy
to look at it separately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
