henry3260 commented on code in PR #67592:
URL: https://github.com/apache/airflow/pull/67592#discussion_r3720699919
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py:
##########
@@ -242,6 +249,14 @@ def ti_run(
extra=json.dumps({"host_name": ti_run_payload.hostname}) if
ti_run_payload.hostname else None,
)
)
+ # The scheduler refreshes queued_dttm every time it queues a task, so
utcnow() - queued_dttm
+ # is a meaningful queue wait for first runs and retries alike (a retry
is a new try that
+ # genuinely waited in the queue) -- mirroring the legacy emit in
emit_state_change_metric,
+ # which fired on every transition to RUNNING. Only resumes from
deferral are skipped,
+ # identified by next_method (the trigger sets it on resume), to avoid
re-emitting within the
+ # same try. queued_dttm is None only in rare races and test setups.
+ emit_queued_duration = ti.queued_dttm is not None and ti.next_method
is None
Review Comment:
> This guard came in at @henry3260's request and it does what it says, but
it leaves `task.queued_duration` and its sibling `task.scheduled_duration`
disagreeing about which transitions count, in opposite directions. Taking the
three ways a TI reaches RUNNING:
>
> * First run: both metrics emit.
> * Retry: only `queued_duration`.
[`schedule_tis`](https://github.com/apache/airflow/blob/a424a811da80ed1e84d690adaeebe0804f57a958/airflow-core/src/airflow/models/dagrun.py#L2245-L2249)
sets `SCHEDULED`, `scheduled_dttm` and `try_number` without clearing
`end_date`, so the previous attempt's `end_date` is still on the row when the
scheduler queues the TI, and [`emit_state_change_metric` returns
early](https://github.com/apache/airflow/blob/a424a811da80ed1e84d690adaeebe0804f57a958/airflow-core/src/airflow/models/taskinstance.py#L1497-L1500).
> * Deferral resume: only `scheduled_duration`. The `DEFERRED` update never
touches `end_date` and `ti_run` already set it to `None`, so that guard passes,
and [the trigger refreshes
`scheduled_dttm`](https://github.com/apache/airflow/blob/a424a811da80ed1e84d690adaeebe0804f57a958/airflow-core/src/airflow/models/trigger.py#L544-L545),
so the sample is a real measurement.
>
> The resume's queue wait is equally real: [`queued_dttm` is refreshed on
every
queueing](https://github.com/apache/airflow/blob/a424a811da80ed1e84d690adaeebe0804f57a958/airflow-core/src/airflow/jobs/scheduler_job_runner.py#L1045)
and [the critical section selects `SCHEDULED` with no `next_method`
filter](https://github.com/apache/airflow/blob/a424a811da80ed1e84d690adaeebe0804f57a958/airflow-core/src/airflow/jobs/scheduler_job_runner.py#L712),
so a deferrable task sits in QUEUED again waiting for a worker slot before
`execute_complete` runs. Skipping it means that wait is never measured, and for
sensor-heavy deployments the resume leg is where most of the queue time lives.
>
> I would drop `and ti.next_method is None` and emit per queue wait, which
is also where @ashb's `end_date` change pointed: one sample per real wait
rather than one per try. The per-try reading is defensible too, but then the
retry case should not emit either, and the comment should name the axis so the
asymmetry reads as deliberate.
>
> Either answer works for me, and this is the only thing I would like
settled before merge. Adding these samples after release shifts percentiles for
anyone alerting on the timer, which is cheap to decide now and awkward to
change later.
>
> On why this did not come up in my last pass: I was looking at the tag set
then, and only walked the resume path through the scheduler this time.
Both work for me, I don't have a strong preference. But given the
`start_from_trigger=True` case where the resume is the only queue wait, I'm
happy to drop the guard I originally asked for — per queue wait sounds right.
@kaxil your call on the final wording.
--
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]
