You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "xBis7 (via GitHub)" <gi...@apache.org> on 2026/10/01 15:21:39 UTC
[PR] Export task-sdk metrics from the API server [airflow]
xBis7 opened a new pull request, #74046:
URL: https://github.com/apache/airflow/pull/74046
<!-- SPDX-License-Identifier: Apache-2.0
https://www.apache.org/licenses/LICENSE-2.0 -->
<!--
Thank you for contributing!
Please provide above a brief description of the changes made in this pull request.
Write a good git commit message following this guide: https://chris.beams.io/posts/git-commit/
Please make sure that your code changes are covered with tests.
And in case of new features or big changes remember to adjust the documentation.
For user-facing UI changes, please attach before/after screenshots (or a short
screen recording) so reviewers can assess the visual impact.
Feel free to ping (in general) for the review if you do not see reaction for a few days
(72 Hours is the minimum reaction time you can expect from volunteers) - we sometimes miss notifications.
In case of an existing issue, reference it using one of the following:
* closes: #ISSUE
* related: #ISSUE
-->
## Issue
When using OpenTelemetry, every metric exported from the task-sdk while a task is running (including custom user-defined metrics) is broken. Note here that this issue doesn't exist for StatsD.
For every task, a new task runner is created along with a new `MeterProvider`. As a result, each series of metrics is treated as new and unique and values don't accumulate.
For example, if we had 5 successful tasks, we would expect to see a value 5 for the counter `airflow_ti_successes_total`. Instead we end up with a series of 1, 1, 1, 1, 1.
## Solution
The solution is to maintain all the call sites exactly as they are but use a new type of logger which keeps everything in memory instead of sending them to a collector. All the accumulated metrics are forwarded to the supervisor and that in turn sends them as a batch to the API server over an endpoint. The API server is a long-lived process with its own `MeterProvider` which exports all the individual task metrics. This solves the accumulation issue while also allowing for the usage of user-defined metrics.
## Testing
I've added an integration test that reads the metrics directly from the otel-collector and checks the value of task metrics to make sure that a counter is increased.
I've also added unit tests for the in-memory accumulating logic as well as the forwarding.
---
##### Was generative AI tooling used to co-author this PR?
<!--
If generative AI tooling has been used in the process of authoring this PR, please
change below checkbox to `[X]` followed by the name of the tool, uncomment the "Generated-by".
-->
- [X] Yes (please specify the tool below)
Claude-code Fable 5.1 xhigh
<!--
Generated-by: [Tool Name] following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
-->
---
* Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#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](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
* When adding dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
* For significant user-facing changes create newsfragment: `{pr_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments). You can add this file in a follow-up commit after the PR is created so you know the PR number.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #74046:
URL: https://github.com/apache/airflow/pull/74046#discussion_r4336612742
##########
task-sdk/src/airflow/sdk/observability/metrics/otel_logger.py:
##########
@@ -16,16 +16,38 @@
# under the License.
from __future__ import annotations
+import os
from typing import TYPE_CHECKING
+from opentelemetry.sdk.resources import SERVICE_INSTANCE_ID
+
from airflow.sdk._shared.observability.metrics import otel_logger
+from airflow.sdk._shared.observability.otel_env_config import load_metrics_env_config
from airflow.sdk.configuration import conf
if TYPE_CHECKING:
from airflow.sdk._shared.observability.metrics.otel_logger import SafeOtelLogger
-def get_otel_logger() -> SafeOtelLogger:
+def get_otel_logger(short_lived: bool = False) -> SafeOtelLogger:
+ service_instance_id = None
+ temporality_preference_env = "OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE"
+
+ if short_lived:
+ # The temporality env variable defaults to `CUMULATIVE` if unset.
+ # If the process is `short_lived`, then set the default fallback to `DELTA`.
+ os.environ.setdefault(temporality_preference_env, "DELTA")
Review Comment:
This is just setting the default. If a user has provided a value, then the user value wins
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #74046:
URL: https://github.com/apache/airflow/pull/74046#discussion_r4298224480
##########
airflow-core/docs/administration-and-deployment/logging-monitoring/metrics.rst:
##########
@@ -184,6 +185,37 @@ How the attributes surface depends on the backend. Those implementing the OpenTe
`Prometheus compatibility <https://opentelemetry.io/docs/specs/otel/compatibility/prometheus_and_openmetrics/>`_
spec expose them as the ``job`` and ``instance`` labels.
+.. _short-lived-processes:
+
+Short-lived processes
+^^^^^^^^^^^^^^^^^^^^^
+
+A task runs in a process that exits when the attempt ends, so a running total exported by that
+process never grows if the values are cumulative. Task SDK processes therefore export counters
Review Comment:
this should be "must therefore" (or "should therefore"?) -- remember that the customer has control over temporality configuration
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on PR #74046:
URL: https://github.com/apache/airflow/pull/74046#issuecomment-6199326699
@dstandish I've updated the PR and changed the approach.
The most important metrics
* `ti.start`
* `ti.finish`
* `ti_successes` and `ti_failures`
* `operator_successes` and `operator_failures`
* `task.duration`
have been moved under the API server. I've also added 2 metrics missing from the scheduler for consistency.
I've made some other small changes to the `task-sdk` so that we don't have to remove the rest of the metrics.
The temporality environment variable defaults to `cumulative` if unset. For the `task-sdk`, now it defaults to `delta` if unset.
Also, the OTel SDK assigns a random and unique `service.instance.id` per process. Just for the `task-sdk`, this is set to a fixed value so that all task runners are treated as the same process.
For backends that support delta values natively, no config changes are needed. But for backends that don't, such as prometheus, the user will need to extend the `otel-collector.yml` file so that it converts the delta values back to cumulative. The required config changes have been added to the documentation. They are simple and they could be just copy-pasted and they will also allow custom metrics to continue working.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #74046:
URL: https://github.com/apache/airflow/pull/74046#discussion_r4339818741
##########
airflow-core/docs/administration-and-deployment/logging-monitoring/metrics.rst:
##########
@@ -184,6 +185,37 @@ How the attributes surface depends on the backend. Those implementing the OpenTe
`Prometheus compatibility <https://opentelemetry.io/docs/specs/otel/compatibility/prometheus_and_openmetrics/>`_
spec expose them as the ``job`` and ``instance`` labels.
+.. _short-lived-processes:
+
+Short-lived processes
+^^^^^^^^^^^^^^^^^^^^^
+
+A task runs in a process that exits when the attempt ends, so a running total exported by that
+process never grows if the values are cumulative. Task SDK processes therefore export counters
Review Comment:
I've updated it and changed the tone
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #74046:
URL: https://github.com/apache/airflow/pull/74046#discussion_r4298286309
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py:
##########
@@ -422,6 +422,12 @@ def ti_run(
if token.claims.scope == "workload":
issue_execution_token(services, response, sub=str(task_instance_id))
+ # Emitted last for the same reason as task.queued_duration below. Every state other than QUEUED
+ # either raised above or was a duplicate request from a running task.
Review Comment:
this comment is confusing. it says "emitted last for same reason as below" -- well that reveals in fact that it _isn't_ emitted last -- something else comes after it. so let's say what we really mean. meanwhile, i also think it's unhelpful to say "same reason as below" even if it is true -- better to just be duplicative because we aren't always going to be able to see the "below" thing in a review, and the "below" thing might at some point get removed. as it is, it's unclear to me what this comment is trying to tell me.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #74046:
URL: https://github.com/apache/airflow/pull/74046#discussion_r4298315743
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py:
##########
@@ -422,6 +422,12 @@ def ti_run(
if token.claims.scope == "workload":
issue_execution_token(services, response, sub=str(task_instance_id))
+ # Emitted last for the same reason as task.queued_duration below. Every state other than QUEUED
+ # either raised above or was a duplicate request from a running task.
+ if previous_state == TaskInstanceState.QUEUED:
+ dr._team_name = dr.team_name
+ stats.incr("ti.start", tags={**dr.stats_tags, "task_id": ti.task_id})
Review Comment:
personally i am of the belief that we should remove dag_id and task_id as attrs. too high cardinality if api server configured with delta to cumulative -- you're likely to get non-increasing series anyway. you should also see what else is in stats_tags.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #74046:
URL: https://github.com/apache/airflow/pull/74046#discussion_r4298472146
##########
task-sdk/src/airflow/sdk/observability/metrics/otel_logger.py:
##########
@@ -16,16 +16,38 @@
# under the License.
from __future__ import annotations
+import os
from typing import TYPE_CHECKING
+from opentelemetry.sdk.resources import SERVICE_INSTANCE_ID
+
from airflow.sdk._shared.observability.metrics import otel_logger
+from airflow.sdk._shared.observability.otel_env_config import load_metrics_env_config
from airflow.sdk.configuration import conf
if TYPE_CHECKING:
from airflow.sdk._shared.observability.metrics.otel_logger import SafeOtelLogger
-def get_otel_logger() -> SafeOtelLogger:
+def get_otel_logger(short_lived: bool = False) -> SafeOtelLogger:
+ service_instance_id = None
+ temporality_preference_env = "OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE"
+
+ if short_lived:
+ # The temporality env variable defaults to `CUMULATIVE` if unset.
+ # If the process is `short_lived`, then set the default fallback to `DELTA`.
+ os.environ.setdefault(temporality_preference_env, "DELTA")
Review Comment:
i feel we should still be leaving this to the user. the user has env vars they can use for this.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #74046:
URL: https://github.com/apache/airflow/pull/74046#discussion_r4339810743
##########
airflow-core/docs/administration-and-deployment/logging-monitoring/metrics.rst:
##########
@@ -329,6 +361,12 @@ On earlier versions, use the ``Stats`` class instead:
These metrics are silently dropped unless a backend is enabled (see `Setup - StatsD`_
or `Setup - OpenTelemetry`_).
+.. note::
+
+ With OpenTelemetry, a metric emitted from inside a task leaves the task's process as a delta
+ value under ``service.instance.id=task-sdk``. A backend that only accepts cumulative values,
+ such as Prometheus, needs the Collector to convert it back, see `Short-lived processes`_.
Review Comment:
I've changed the tone and updated it.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #74046:
URL: https://github.com/apache/airflow/pull/74046#discussion_r4298244583
##########
airflow-core/docs/administration-and-deployment/logging-monitoring/metrics.rst:
##########
@@ -184,6 +185,37 @@ How the attributes surface depends on the backend. Those implementing the OpenTe
`Prometheus compatibility <https://opentelemetry.io/docs/specs/otel/compatibility/prometheus_and_openmetrics/>`_
spec expose them as the ``job`` and ``instance`` labels.
+.. _short-lived-processes:
+
+Short-lived processes
+^^^^^^^^^^^^^^^^^^^^^
+
+A task runs in a process that exits when the attempt ends, so a running total exported by that
+process never grows if the values are cumulative. Task SDK processes therefore export counters
+and histograms as **delta** values and share ``service.instance.id=task-sdk``, so that every
+attempt adds to the same series. ``OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE`` and
+``OTEL_RESOURCE_ATTRIBUTES`` override either, if set.
+
+For backends that support delta values natively, no change is needed. Backends that only
Review Comment:
"no change" is needed is unclear. because it begs the question, "change from what"
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #74046:
URL: https://github.com/apache/airflow/pull/74046#discussion_r4298429710
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py:
##########
@@ -787,6 +803,26 @@ def _build_rejected_gate_payload(
return TITerminalStatePayload(state=TerminalStateNonSuccess.FAILED, retry_reason=reason, **carried)
+def _emit_task_finish_metrics(
Review Comment:
i'm not sure we need all of these attributes. and they can cause problems.
even on api server, you are relying on the same worker getting hit with the same event type for the same team / operator / task / dag before the series gets expired (if delta to cumulative) or before the worker gets cycled (if cumulative), aren't you? otherwise you'll get non-incrementing series.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #74046:
URL: https://github.com/apache/airflow/pull/74046#discussion_r4326624557
##########
airflow-core/docs/administration-and-deployment/logging-monitoring/metrics.rst:
##########
@@ -184,6 +185,37 @@ How the attributes surface depends on the backend. Those implementing the OpenTe
`Prometheus compatibility <https://opentelemetry.io/docs/specs/otel/compatibility/prometheus_and_openmetrics/>`_
spec expose them as the ``job`` and ``instance`` labels.
+.. _short-lived-processes:
+
+Short-lived processes
+^^^^^^^^^^^^^^^^^^^^^
+
+A task runs in a process that exits when the attempt ends, so a running total exported by that
+process never grows if the values are cumulative. Task SDK processes therefore export counters
+and histograms as **delta** values and share ``service.instance.id=task-sdk``, so that every
+attempt adds to the same series. ``OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE`` and
+``OTEL_RESOURCE_ATTRIBUTES`` override either, if set.
+
+For backends that support delta values natively, no change is needed. Backends that only
+accept cumulative values, such as Prometheus, need the Collector to convert the
+values back and to keep idle series long enough for the next run of a Dag to add to them:
+
+.. code-block:: yaml
+
+ processors:
+ deltatocumulative:
+ max_stale: 168h # 1 week
Review Comment:
I'm looking into this. I'm running manual tests in breeze and taking some measurements.
This part below is unnecessary and actually leads to unwanted side effects. I'll remove it.
```
exporters:
prometheus:
endpoint: 0.0.0.0:8889
metric_expiration: 168h # 1 week
```
Every stream is approximately 1kB.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #74046:
URL: https://github.com/apache/airflow/pull/74046#discussion_r4336630031
##########
task-sdk/src/airflow/sdk/observability/metrics/otel_logger.py:
##########
@@ -16,16 +16,38 @@
# under the License.
from __future__ import annotations
+import os
from typing import TYPE_CHECKING
+from opentelemetry.sdk.resources import SERVICE_INSTANCE_ID
+
from airflow.sdk._shared.observability.metrics import otel_logger
+from airflow.sdk._shared.observability.otel_env_config import load_metrics_env_config
from airflow.sdk.configuration import conf
if TYPE_CHECKING:
from airflow.sdk._shared.observability.metrics.otel_logger import SafeOtelLogger
-def get_otel_logger() -> SafeOtelLogger:
+def get_otel_logger(short_lived: bool = False) -> SafeOtelLogger:
+ service_instance_id = None
+ temporality_preference_env = "OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE"
+
+ if short_lived:
+ # The temporality env variable defaults to `CUMULATIVE` if unset.
+ # If the process is `short_lived`, then set the default fallback to `DELTA`.
+ os.environ.setdefault(temporality_preference_env, "DELTA")
+
+ # Check the value of the env variable because the user could have defined it as `CUMULATIVE`.
+ # LOWMEMORY also exports counters and histograms as deltas.
+ if (
+ os.environ[temporality_preference_env].strip().upper() in ("DELTA", "LOWMEMORY")
+ and SERVICE_INSTANCE_ID not in load_metrics_env_config().resource_attributes
Review Comment:
@dstandish See here, if the service instance id isn't provided by the user, then we set a fixed value.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #74046:
URL: https://github.com/apache/airflow/pull/74046#discussion_r4298268941
##########
airflow-core/docs/administration-and-deployment/logging-monitoring/metrics.rst:
##########
@@ -329,6 +361,12 @@ On earlier versions, use the ``Stats`` class instead:
These metrics are silently dropped unless a backend is enabled (see `Setup - StatsD`_
or `Setup - OpenTelemetry`_).
+.. note::
+
+ With OpenTelemetry, a metric emitted from inside a task leaves the task's process as a delta
+ value under ``service.instance.id=task-sdk``. A backend that only accepts cumulative values,
+ such as Prometheus, needs the Collector to convert it back, see `Short-lived processes`_.
Review Comment:
similar to above comments. maybe we should talk about "shoulds" because user has control over delta temporality. and, we should check whether you can set service.instance.id to a constant via env var and if so, we should probably just recommend that the user do that rather than doing it for them.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #74046:
URL: https://github.com/apache/airflow/pull/74046#discussion_r4336638432
##########
shared/observability/src/airflow_shared/observability/metrics/otel_logger.py:
##########
@@ -481,7 +485,11 @@ def get_otel_logger(
otel_env_config = load_metrics_env_config()
effective_service_name: str = otel_env_config.service_name or service_name or "airflow"
- resource = Resource.create(attributes={SERVICE_NAME: effective_service_name})
+ resource_attributes = {SERVICE_NAME: effective_service_name}
+ if service_instance_id:
Review Comment:
It is, but it's decided at the caller. The caller checks the environment and if the user has provided a value, then this is `None`. If there isn't a value, this gets a fixed one which is then set here.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #74046:
URL: https://github.com/apache/airflow/pull/74046#discussion_r4336685262
##########
airflow-core/docs/administration-and-deployment/logging-monitoring/metrics.rst:
##########
@@ -184,6 +185,37 @@ How the attributes surface depends on the backend. Those implementing the OpenTe
`Prometheus compatibility <https://opentelemetry.io/docs/specs/otel/compatibility/prometheus_and_openmetrics/>`_
spec expose them as the ``job`` and ``instance`` labels.
+.. _short-lived-processes:
+
+Short-lived processes
+^^^^^^^^^^^^^^^^^^^^^
+
+A task runs in a process that exits when the attempt ends, so a running total exported by that
+process never grows if the values are cumulative. Task SDK processes therefore export counters
+and histograms as **delta** values and share ``service.instance.id=task-sdk``, so that every
Review Comment:
I renamed it to `task-worker` but I also removed all the references from the docs because the name isn't fixed and the user can provide his own value. I also changed the tone to suggest that we advice using a shared value if this is set.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #74046:
URL: https://github.com/apache/airflow/pull/74046#discussion_r4336727058
##########
airflow-core/docs/administration-and-deployment/logging-monitoring/metrics.rst:
##########
@@ -184,6 +185,37 @@ How the attributes surface depends on the backend. Those implementing the OpenTe
`Prometheus compatibility <https://opentelemetry.io/docs/specs/otel/compatibility/prometheus_and_openmetrics/>`_
spec expose them as the ``job`` and ``instance`` labels.
+.. _short-lived-processes:
+
+Short-lived processes
+^^^^^^^^^^^^^^^^^^^^^
+
+A task runs in a process that exits when the attempt ends, so a running total exported by that
+process never grows if the values are cumulative. Task SDK processes therefore export counters
+and histograms as **delta** values and share ``service.instance.id=task-sdk``, so that every
+attempt adds to the same series. ``OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE`` and
+``OTEL_RESOURCE_ATTRIBUTES`` override either, if set.
+
+For backends that support delta values natively, no change is needed. Backends that only
+accept cumulative values, such as Prometheus, need the Collector to convert the
+values back and to keep idle series long enough for the next run of a Dag to add to them:
+
+.. code-block:: yaml
+
+ processors:
+ deltatocumulative:
+ max_stale: 168h # 1 week
Review Comment:
I updated the docs with more information about this and some scenarios where we might end up with an OOM error.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on PR #74046:
URL: https://github.com/apache/airflow/pull/74046#issuecomment-5934669350
@stuart23 I know you've been looking at similar things.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on PR #74046:
URL: https://github.com/apache/airflow/pull/74046#issuecomment-6322183567
@dstandish Thank you for the review! I've addressed your comments and made the changes. Please take a look and let me know how it looks. I've rewritten the documentation changes.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #74046:
URL: https://github.com/apache/airflow/pull/74046#discussion_r4298440141
##########
shared/observability/src/airflow_shared/observability/metrics/otel_logger.py:
##########
@@ -481,7 +485,11 @@ def get_otel_logger(
otel_env_config = load_metrics_env_config()
effective_service_name: str = otel_env_config.service_name or service_name or "airflow"
- resource = Resource.create(attributes={SERVICE_NAME: effective_service_name})
+ resource_attributes = {SERVICE_NAME: effective_service_name}
+ if service_instance_id:
Review Comment:
why not leave it to optional env var configuration to preserve the flexibility ordinarily provided to users?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #74046:
URL: https://github.com/apache/airflow/pull/74046#discussion_r4298254131
##########
airflow-core/docs/administration-and-deployment/logging-monitoring/metrics.rst:
##########
@@ -184,6 +185,37 @@ How the attributes surface depends on the backend. Those implementing the OpenTe
`Prometheus compatibility <https://opentelemetry.io/docs/specs/otel/compatibility/prometheus_and_openmetrics/>`_
spec expose them as the ``job`` and ``instance`` labels.
+.. _short-lived-processes:
+
+Short-lived processes
+^^^^^^^^^^^^^^^^^^^^^
+
+A task runs in a process that exits when the attempt ends, so a running total exported by that
+process never grows if the values are cumulative. Task SDK processes therefore export counters
+and histograms as **delta** values and share ``service.instance.id=task-sdk``, so that every
+attempt adds to the same series. ``OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE`` and
+``OTEL_RESOURCE_ATTRIBUTES`` override either, if set.
+
+For backends that support delta values natively, no change is needed. Backends that only
+accept cumulative values, such as Prometheus, need the Collector to convert the
+values back and to keep idle series long enough for the next run of a Dag to add to them:
+
+.. code-block:: yaml
+
+ processors:
+ deltatocumulative:
+ max_stale: 168h # 1 week
Review Comment:
this has the potential to result in OOM issues doesn't it?
maybe worth a mention of the tradeoff
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on PR #74046:
URL: https://github.com/apache/airflow/pull/74046#issuecomment-5939821088
The current approach is too complicated and so I converted this to a draft while I'm exploring some alternatives.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #74046:
URL: https://github.com/apache/airflow/pull/74046#discussion_r4328564236
##########
airflow-core/docs/administration-and-deployment/logging-monitoring/metrics.rst:
##########
@@ -154,9 +154,10 @@ decide how much of a deployment can be told apart:
a metric to the kind of process that produced it.
``service.instance.id``
- Which process of that component reported the metric. It is unset by default, so processes
- running the same component (e.g. 2+ schedulers) are indistinguishable. Set it per process
- to attribute a metric to one of them.
+ Which process of that component reported the metric. The SDK generates a random one for every
+ process, so two schedulers produce separate series, but nothing in the id says which scheduler
+ is which. Set it per process to attribute a metric to one of them. Short-lived Task SDK
Review Comment:
Yes, I agree. That's what I tried to say. Let me try to make it more clear.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #74046:
URL: https://github.com/apache/airflow/pull/74046#discussion_r4339689581
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py:
##########
@@ -787,6 +803,26 @@ def _build_rejected_gate_payload(
return TITerminalStatePayload(state=TerminalStateNonSuccess.FAILED, retry_reason=reason, **carried)
+def _emit_task_finish_metrics(
Review Comment:
These aren't new. These are the old ones that have been moved.
Statsd has an option to disable tags
https://github.com/apache/airflow/blob/main/airflow-core/src/airflow/config_templates/config.yml#L1328-L1336
```
statsd_disabled_tags:
description: |
If you want to avoid sending all the available metrics tags to StatsD,
you can configure a block list of prefixes (comma separated) to filter out metric tags
that start with the elements of the list (e.g: ``job_id,run_id``)
version_added: 2.6.0
type: string
example: job_id,run_id,dag_id,task_id
default: job_id,run_id
```
what if we do the same for OTel? That way it's up to the users whether they want the cardinality or not.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #74046:
URL: https://github.com/apache/airflow/pull/74046#discussion_r4298224480
##########
airflow-core/docs/administration-and-deployment/logging-monitoring/metrics.rst:
##########
@@ -184,6 +185,37 @@ How the attributes surface depends on the backend. Those implementing the OpenTe
`Prometheus compatibility <https://opentelemetry.io/docs/specs/otel/compatibility/prometheus_and_openmetrics/>`_
spec expose them as the ``job`` and ``instance`` labels.
+.. _short-lived-processes:
+
+Short-lived processes
+^^^^^^^^^^^^^^^^^^^^^
+
+A task runs in a process that exits when the attempt ends, so a running total exported by that
+process never grows if the values are cumulative. Task SDK processes therefore export counters
Review Comment:
this should be "must therefore" (or "should therefore"?) -- remember that the sure cas control over temporality configuration
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #74046:
URL: https://github.com/apache/airflow/pull/74046#discussion_r4298224480
##########
airflow-core/docs/administration-and-deployment/logging-monitoring/metrics.rst:
##########
@@ -184,6 +185,37 @@ How the attributes surface depends on the backend. Those implementing the OpenTe
`Prometheus compatibility <https://opentelemetry.io/docs/specs/otel/compatibility/prometheus_and_openmetrics/>`_
spec expose them as the ``job`` and ``instance`` labels.
+.. _short-lived-processes:
+
+Short-lived processes
+^^^^^^^^^^^^^^^^^^^^^
+
+A task runs in a process that exits when the attempt ends, so a running total exported by that
+process never grows if the values are cumulative. Task SDK processes therefore export counters
Review Comment:
this should be "must therefore" -- remember that the sure cas control over temporality configuration
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #74046:
URL: https://github.com/apache/airflow/pull/74046#discussion_r4298217249
##########
airflow-core/docs/administration-and-deployment/logging-monitoring/metrics.rst:
##########
@@ -154,9 +154,10 @@ decide how much of a deployment can be told apart:
a metric to the kind of process that produced it.
``service.instance.id``
- Which process of that component reported the metric. It is unset by default, so processes
- running the same component (e.g. 2+ schedulers) are indistinguishable. Set it per process
- to attribute a metric to one of them.
+ Which process of that component reported the metric. The SDK generates a random one for every
+ process, so two schedulers produce separate series, but nothing in the id says which scheduler
+ is which. Set it per process to attribute a metric to one of them. Short-lived Task SDK
Review Comment:
it is automatically set by default to a uuid as of otel sdk 1.43 i believe. which is desirable particularly e.g. to distinguish api server workers which are long lived -- though undesirable for short-lived processes such as task processes where the only sensible treatment is delta temporality
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #74046:
URL: https://github.com/apache/airflow/pull/74046#discussion_r4298286309
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py:
##########
@@ -422,6 +422,12 @@ def ti_run(
if token.claims.scope == "workload":
issue_execution_token(services, response, sub=str(task_instance_id))
+ # Emitted last for the same reason as task.queued_duration below. Every state other than QUEUED
+ # either raised above or was a duplicate request from a running task.
Review Comment:
this comment is confusing. it says "emitted last for same reason as below" -- well that reveals in fact that it _isn't_ emitted last -- something else comes after it. so let's say what we really mean. meanwhile, i also think it's unhelpful to say "same reason as below" even if it is true -- better to just be duplicative because we aren't always going to be able to see the "below" thing in a review, and the "below" thing might at some point get removed.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #74046:
URL: https://github.com/apache/airflow/pull/74046#discussion_r4339814923
##########
airflow-core/docs/administration-and-deployment/logging-monitoring/metrics.rst:
##########
@@ -184,6 +185,37 @@ How the attributes surface depends on the backend. Those implementing the OpenTe
`Prometheus compatibility <https://opentelemetry.io/docs/specs/otel/compatibility/prometheus_and_openmetrics/>`_
spec expose them as the ``job`` and ``instance`` labels.
+.. _short-lived-processes:
+
+Short-lived processes
+^^^^^^^^^^^^^^^^^^^^^
+
+A task runs in a process that exits when the attempt ends, so a running total exported by that
+process never grows if the values are cumulative. Task SDK processes therefore export counters
+and histograms as **delta** values and share ``service.instance.id=task-sdk``, so that every
+attempt adds to the same series. ``OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE`` and
+``OTEL_RESOURCE_ATTRIBUTES`` override either, if set.
+
+For backends that support delta values natively, no change is needed. Backends that only
Review Comment:
I've updated it.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on PR #74046:
URL: https://github.com/apache/airflow/pull/74046#issuecomment-5936489534
Also @dstandish, worth you looking at this.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Export task-sdk metrics from the API server [airflow]
Posted by "xBis7 (via GitHub)" <gi...@apache.org>.
xBis7 commented on code in PR #74046:
URL: https://github.com/apache/airflow/pull/74046#discussion_r4337591975
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py:
##########
@@ -422,6 +422,12 @@ def ti_run(
if token.claims.scope == "workload":
issue_execution_token(services, response, sub=str(task_instance_id))
+ # Emitted last for the same reason as task.queued_duration below. Every state other than QUEUED
+ # either raised above or was a duplicate request from a running task.
Review Comment:
I updated the comment.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org