dstandish commented on PR #74046: URL: https://github.com/apache/airflow/pull/74046#issuecomment-5939383984
Thanks for digging into this. The 1, 1, 1, 1, 1 counter problem is real and worth fixing. I'm hesitant about the approach, though, because it adds a lot of moving parts: - a new Execution API endpoint and version (plus Go/TS SDK model changes) - a new comms message - an aggregating backend - a background flush thread in every task process Before we commit to maintaining all of that, I'd like to check whether a simpler fix covers most of the need. **Most task metrics could be emitted by the API server instead** These are the first-party metrics emitted from `task_runner`: - `ti.start` - `ti.finish` - `ti_successes` and `ti_failures` - `operator_successes` and `operator_failures` - `task.duration` Each of them corresponds to a state transition the API server already handles: `ti_run` for start, and `ti_update_state` for the terminal state and end date. The TI row has `operator`, `start_date` and `end_date`, so every tag and the duration can be computed there. The API server is a long-lived process with its own `MeterProvider`, so the counters would accumulate correctly with no forwarding at all. It would also make the metrics more accurate. A task that gets OOM-killed or SIGKILLed never emits `ti_failures` today. Emitting wherever the failure state is recorded (the state update route, or the scheduler's heartbeat-timeout handling) would count those failures too. **The remaining case is narrow** With lifecycle metrics on the server side, the only thing left broken is OTel combined with custom metrics emitted from inside a task: - StatsD already works, because the StatsD server does the aggregating (the PR description says this too). - As far as I can tell, `Stats` isn't part of the public `airflow.sdk` API. I'm not sure how much in-task custom-metric usage we're committed to supporting. For OTel users who do this, delta temporality (`OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE=delta`) is the standard answer for short-lived processes. It's the same model StatsD uses: each process reports what it added, and the backend sums it up. Backends that accept deltas natively handle this with no extra work. Prometheus via `deltatocumulative` is messier, but that's a general OTel limitation with short-lived processes rather than something Airflow needs to route through its API server. **Suggestion** 1. Emit the lifecycle metrics from the execution API routes and the scheduler, and remove them from `task_runner`. 2. Leave in-task `Stats` as is, and document delta temporality for OTel users who emit custom metrics from tasks. 3. Revisit a forwarding mechanism separately if there's real demand for accumulating custom OTel metrics on Prometheus. The trade-off is that the emitting component changes, so dashboards that filter on worker host or service tags would need updating. That would need a newsfragment. Still, it seems much cheaper than a permanent new Execution API surface. What do you think? Are there use cases I'm missing that need worker-side emission? --- Drafted-by: Claude Code (Opus 5.5); reviewed by @dstandish before posting -- 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]
