Update metrics names to allow multiple executors to report metrics

[syedahsn]

Currently, metrics for executors are emitted using the following format:

airflow_executor_open_slots
airflow_executor_queued_tasks
airflow_executor_running_tasks

When multiple executors are configured, this format will not work because each executor will emit its data under the same name. In order to resolve this, we can include the executor to differentiate between each executor:

airflow_executor_open_slots_LocalExecutor
airflow_executor_queued_tasks_LocalExecutor
airflow_executor_running_tasks_LocalExecutor


airflow_executor_open_slots_AwsEcsExecutor
airflow_executor_queued_tasks_AwsEcsExecutor
airflow_executor_running_tasks_AwsEcsExecutor

---

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

[romsharon98]

Why not adding this suffix for each executor no matter number of executors?

[syedahsn]

Why not adding this suffix for each executor no matter number of executors?

We could do that, but that would be a breaking change. Any workflows that require these metrics to be emitted will break if we change the naming format.

[o-nikolas]

Why not adding this suffix for each executor no matter number of executors?

We could do that, but that would be a breaking change. Any workflows that require these metrics to be emitted will break if we change the naming format.

Yeah, doing it this way allows the default case (one executor only) which most folks will continue to use for some time, will work as it always has without a change in the metric names (since a name change may interfere with people's dashboards and metrics processing). For Airflow 3 (when we can easily make breaking changes) we'll update the metrics to include the executor name in all cases always.

[eladkal]

For Airflow 3 (when we can easily make breaking changes) we'll update the metrics to include the executor name in all cases always.

What about introducing a setting that will control over which behavior is used and for Airflow 3 we will eliminate the setting and make the new way (with executor name) the default one? This way we can implement everything now and not have a tech debt later.

WDYT?

[o-nikolas]

For Airflow 3 (when we can easily make breaking changes) we'll update the metrics to include the executor name in all cases always.

What about introducing a setting that will control over which behavior is used and for Airflow 3 we will eliminate the setting and make the new way (with executor name) the default one? This way we can implement everything now and not have a tech debt later.

WDYT?

I think Airflow has too many configurations as it is 😅 Given the short period of time between now and Airflow 3 I don't think it's worth the complexity. Either way we'll have to circle back and make a code change (either remove that configuration logic or make the small code change to modify the behaviour) so I don't think it buys us too much, personally.

[shubham22]

I am with @o-nikolas on this one. Introducing a config, just to remove it again in next version (assuming there is no v2.11) does not make much sense. We anyways need a lot of clean up, and this tech debt is a trivial change that we can keep track and ensure that it is there in v3.

[o-nikolas]

Code changes look fine, but should this have a unit test?

[eladkal]

I am with @o-nikolas on this one. Introducing a config, just to remove it again in next version (assuming there is no v2.11) does not make much sense. We anyways need a lot of clean up, and this tech debt is a trivial change that we can keep track and ensure that it is there in v3.

It can make sense if you want to allow users to be prepared earlier and have 1 less thing to worry about. You give them the chance to be Aifflow 3 complient. This may be something we want to think about more generally... cc @kaxil

This is not something I feel strongly about so either way we can proceed.

[syedahsn]

Code changes look fine, but should this have a unit test?

There wasn't a unit test initially that tested original behavior, so I didn't write one to test the updated behavior. As far as I can tell, there are no tests related to metrics being emitted.

[syedahsn]

What about introducing a setting that will control over which behavior is used and for Airflow 3 we will eliminate the setting and make the new way (with executor name) the default one? This way we can implement everything now and not have a tech debt later.

Agree with the others on this. There isn't necessarily a tech debt here, and if so, it wouldn't take much more effort to update the metric than it would to remove a configuration which allows the newer metric to be emitted.

You give them the chance to be Aifflow 3 complient.

I do agree with this point, but ultimately, its not a big enough change to warrant to much prep work.

[kaxil]

The other option is to emit both metrics, so users can move to Airflow 3 style without worrying about Airflow configs but can still stay ahead of the curve.

Could you also update the following 2 places:

• Helm Chart (this can be a separate PR as long as we do it):

  airflow/chart/files/statsd-mappings.yml

  Line 21 in cd68840

  # instead utilize `statsd.extraMappings` or `statsd.overrideMappings` in your environment,

• Metrics Docs: https://github.com/apache/airflow/blob/main/docs/apache-airflow/administration-and-deployment/logging-monitoring/metrics.rst