Skip to content

Conversation

@kaxil
Copy link
Member

@kaxil kaxil commented Nov 13, 2024

Follow-up of #39908
Changes:

  • Replaces the metrics_consistency_on config with timer_unit_consistency for better clarity!
  • Improves the newsfragment entry & deprecation warning
  • Changes the default to be False so folks aren't caught by surprise.

We should backport this to 2.11 and remove this setting from Airflow main


^ 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.

@kaxil kaxil requested a review from ferruzzi November 13, 2024 14:53
@kaxil kaxil requested review from XD-DENG and ashb as code owners November 13, 2024 14:53
@kaxil kaxil added this to the Airflow 2.11.0 milestone Nov 13, 2024
@kaxil kaxil changed the title Refactor & rename metrics_consistency_on conf to `timer_unit_consis… Refactor & rename metrics_consistency_on conf to timer_unit_consistency Nov 13, 2024
…tency`

Changes:
- Replaces the `metrics_consistency_on` config with `timer_unit_consistency` for better clarity!
- Improves the newsfragment entry & deprecation warning
- Changes the default to be `False` so folks aren't caught by surprise.

We should backport this to 2.11 and remove this setting from Airflow main
@kaxil
Copy link
Member Author

kaxil commented Nov 13, 2024

The failed checks are unrelated and due to "docker timeouts"

@kaxil kaxil merged commit 1bd061d into apache:main Nov 13, 2024
@kaxil kaxil deleted the refactor-metrics branch November 13, 2024 16:28
@ferruzzi
Copy link
Contributor

I like the name change, but the default being "on" was discussed in the PR where it was added](#39908), why are we changing it now? This is a "breaking change" in that it requires adjusting StatsD dashboards, but it is being applied in 3.0 which is the time to do such changes, right? The last I heard in discussions was that StatsD was being moved to second-class and OTel Metrics were supposed to be treated as the standard starting in 3.0

Please read through the conversation on the other PR and reconsider this change.

@kaxil
Copy link
Member Author

kaxil commented Nov 13, 2024

I like the name change, but the default being "on" was discussed in the PR where it was added](#39908), why are we changing it now? This is a "breaking change" in that it requires adjusting StatsD dashboards, but it is being applied in 3.0 which is the time to do such changes, right? The last I heard in discussions was that StatsD was being moved to second-class and OTel Metrics were supposed to be treated as the standard starting in 3.0

Please read through the conversation on the other PR and reconsider this change.

A few things:

  • The version-added in this one was still version_added: 2.10.0
  • Airflow 3 will already remove this, check PR description where it says: "We should backport this to 2.11 and remove this setting from Airflow main" including the newsfragment. The idea is to keep it "off" for 2.11 so users get advanced warning and they can migrate early if they want without breaking and of their current tools. The main behaviour still says the same once we remove it, there is also a TODO I had added in this PR, please let me know if that isn't what you wanted

@kaxil
Copy link
Member Author

kaxil commented Nov 13, 2024

The only thing different is instead of getting it in 3.0 with default "on" and breaking compact and removing it in 3.1 -- here we make it consistent that 3.0 will break it -- and we warn users in advance about this in 2.11 -- our bridge release.

It will make it consistent with how we are breaking other things.

@ferruzzi
Copy link
Contributor

Thank you.

kaxil added a commit to astronomer/airflow that referenced this pull request Nov 13, 2024
- Removes the `timer_unit_consistency` configuration option, standardizing all timer and timing metrics to milliseconds by default.
- Updates metric loggers (e.g., Datadog, OpenTelemetry) to ensure uniform milliseconds-based reporting.
- Cleans up related warnings!

This is a follow-up of apache#43966 for main & Airflow 3
kaxil added a commit to astronomer/airflow that referenced this pull request Nov 13, 2024
- Removes the `timer_unit_consistency` configuration option, standardizing all timer and timing metrics to milliseconds by default.
- Updates metric loggers (e.g., Datadog, OpenTelemetry) to ensure uniform milliseconds-based reporting.
- Cleans up related warnings!

This is a follow-up of apache#43966 for main & Airflow 3
amoghrajesh pushed a commit to astronomer/airflow that referenced this pull request Nov 14, 2024
…tency` (apache#43966)

Changes:
- Replaces the `metrics_consistency_on` config with `timer_unit_consistency` for better clarity!
- Improves the newsfragment entry & deprecation warning
- Changes the default to be `False` so folks aren't caught by surprise.

We should backport this to 2.11 and remove this setting from Airflow main
kaxil added a commit that referenced this pull request Nov 14, 2024
- Removes the `timer_unit_consistency` configuration option, standardizing all timer and timing metrics to milliseconds by default.
- Updates metric loggers (e.g., Datadog, OpenTelemetry) to ensure uniform milliseconds-based reporting.
- Cleans up related warnings!

This is a follow-up of #43966 for main & Airflow 3
kaxil added a commit that referenced this pull request Apr 29, 2025
…tency` (#43966)

Changes:
- Replaces the `metrics_consistency_on` config with `timer_unit_consistency` for better clarity!
- Improves the newsfragment entry & deprecation warning
- Changes the default to be `False` so folks aren't caught by surprise.

We should backport this to 2.11 and remove this setting from Airflow main

(cherry picked from commit 1bd061d)
kaxil added a commit that referenced this pull request Apr 29, 2025
…tency` (#43966)

Changes:
- Replaces the `metrics_consistency_on` config with `timer_unit_consistency` for better clarity!
- Improves the newsfragment entry & deprecation warning
- Changes the default to be `False` so folks aren't caught by surprise.

We should backport this to 2.11 and remove this setting from Airflow main

(cherry picked from commit 1bd061d)
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request May 27, 2025
- Removes the `timer_unit_consistency` configuration option, standardizing all timer and timing metrics to milliseconds by default.
- Updates metric loggers (e.g., Datadog, OpenTelemetry) to ensure uniform milliseconds-based reporting.
- Cleans up related warnings!

This is a follow-up of apache/airflow#43966 for main & Airflow 3

GitOrigin-RevId: 339bc7748156a4d099121092addb5af1da4e81e8
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Sep 23, 2025
- Removes the `timer_unit_consistency` configuration option, standardizing all timer and timing metrics to milliseconds by default.
- Updates metric loggers (e.g., Datadog, OpenTelemetry) to ensure uniform milliseconds-based reporting.
- Cleans up related warnings!

This is a follow-up of apache/airflow#43966 for main & Airflow 3

GitOrigin-RevId: 339bc7748156a4d099121092addb5af1da4e81e8
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Oct 20, 2025
- Removes the `timer_unit_consistency` configuration option, standardizing all timer and timing metrics to milliseconds by default.
- Updates metric loggers (e.g., Datadog, OpenTelemetry) to ensure uniform milliseconds-based reporting.
- Cleans up related warnings!

This is a follow-up of apache/airflow#43966 for main & Airflow 3

GitOrigin-RevId: 339bc7748156a4d099121092addb5af1da4e81e8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants