You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "o-nikolas (via GitHub)" <gi...@apache.org> on 2026/01/26 17:28:51 UTC
[PR] Re-enable missing metric: dag_processing.last_duration. [airflow]
o-nikolas opened a new pull request, #61079:
URL: https://github.com/apache/airflow/pull/61079
This metric was intentionally disabled during AIP-66 development. These changes bring the metric back using relative_fileloc instead of dag_path.file
<!--
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: http://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.
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
-->
---
##### 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)
Cline - Claude Opus 4.5
<!--
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` or `{issue_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments).
--
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] Re-enable missing metric: dag_processing.last_duration. [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #61079:
URL: https://github.com/apache/airflow/pull/61079#discussion_r2733211629
##########
airflow-core/src/airflow/dag_processing/manager.py:
##########
@@ -1195,10 +1195,11 @@ def process_parse_results(
run_count=run_count + 1,
)
- # TODO: AIP-66 emit metrics
- # file_name = Path(dag_file.path).stem
- # Stats.timing(f"dag_processing.last_duration.{file_name}", stat.last_duration)
- # Stats.timing("dag_processing.last_duration", stat.last_duration, tags={"file_name": file_name})
+ # Note: relative_fileloc has a None default. In practice it is always provided but code defensively here in case
+ if relative_fileloc is not None and stat.last_duration is not None:
+ file_name = Path(relative_fileloc).stem
Review Comment:
Chatted offline: Bundle name will go most of the way to ensure uniqueness. If users are duplicating filenames within sub directories of the same bundle that's pretty obscure and they will notice this and can fix that on their end if this metric is very important to 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] Re-enable missing metric: dag_processing.last_duration. [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #61079:
URL: https://github.com/apache/airflow/pull/61079#discussion_r2730959939
##########
airflow-core/src/airflow/dag_processing/manager.py:
##########
@@ -1195,10 +1195,11 @@ def process_parse_results(
run_count=run_count + 1,
)
- # TODO: AIP-66 emit metrics
- # file_name = Path(dag_file.path).stem
- # Stats.timing(f"dag_processing.last_duration.{file_name}", stat.last_duration)
- # Stats.timing("dag_processing.last_duration", stat.last_duration, tags={"file_name": file_name})
+ # Note: relative_fileloc has a None default. In practice it is always provided but code defensively here in case
+ if relative_fileloc is not None and stat.last_duration is not None:
+ file_name = Path(relative_fileloc).stem
+ Stats.timing(f"dag_processing.last_duration.{file_name}", stat.last_duration)
+ Stats.timing("dag_processing.last_duration", stat.last_duration, tags={"file_name": file_name})
Review Comment:
I know this doesn’t really matter in practice, but would it be better to log `{"file_name": None}` instead of not logging at all?
--
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] Re-enable missing metric: dag_processing.last_duration. [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #61079:
URL: https://github.com/apache/airflow/pull/61079#discussion_r2734271590
##########
shared/observability/src/airflow_shared/observability/metrics/stats.py:
##########
@@ -29,6 +30,36 @@
log = logging.getLogger(__name__)
+_VALID_STAT_NAME_CHARS_RE = re.compile(r"^[a-zA-Z0-9_.-]+$")
+_INVALID_STAT_NAME_CHARS_RE = re.compile(r"[^a-zA-Z0-9_.-]")
Review Comment:
I know this is copypasta, but the StatsD and OTel metric names have different sets of allowed characters. The OTel list are in https://github.com/apache/airflow/blob/main/shared/observability/src/airflow_shared/observability/metrics/validators.py#L47
##########
airflow-core/tests/unit/dag_processing/test_manager.py:
##########
@@ -734,11 +733,20 @@ def test_send_file_processing_statsd_timing(
manager = DagFileProcessorManager(max_runs=1)
manager.run()
- last_runtime = manager._file_stats[os.fspath(path_to_parse)].last_duration
+ file_info = DagFileInfo(
+ bundle_name="testing",
+ rel_path=Path("temp_dag.py"),
Review Comment:
Should this maybe be using path_to_parse instead of a magic string that may drift over time?
--
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] Re-enable missing metric: dag_processing.last_duration. [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #61079:
URL: https://github.com/apache/airflow/pull/61079#discussion_r2742709097
##########
airflow-core/src/airflow/dag_processing/manager.py:
##########
@@ -1195,10 +1195,18 @@ def process_parse_results(
run_count=run_count + 1,
)
- # TODO: AIP-66 emit metrics
- # file_name = Path(dag_file.path).stem
- # Stats.timing(f"dag_processing.last_duration.{file_name}", stat.last_duration)
- # Stats.timing("dag_processing.last_duration", stat.last_duration, tags={"file_name": file_name})
+ # Note: relative_fileloc has a None default. In practice it is always provided but code defensively here in case
+ if relative_fileloc is not None and stat.last_duration is not None:
+ # Normalize names to ensure they only contain valid characters for stats (alphanumeric, underscore, dot, dash)
+ file_name = normalize_name_for_stats(Path(relative_fileloc).stem)
+ # bundle_name is included to distinguish files with the same name across different bundles
+ normalized_bundle = normalize_name_for_stats(bundle_name)
+ Stats.timing(f"dag_processing.last_duration.{normalized_bundle}.{file_name}", stat.last_duration)
+ Stats.timing(
+ "dag_processing.last_duration",
+ stat.last_duration,
+ tags={"file_name": file_name, "bundle_name": normalized_bundle},
Review Comment:
Hey Jed, can you say a bit more? I used the same filename for consistency. I do think otel has it's own normlizer, so I suppose it doesn't matter. Curious to hear a bit more
--
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] Re-enable missing metric: dag_processing.last_duration. [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #61079:
URL: https://github.com/apache/airflow/pull/61079#discussion_r2743586861
##########
airflow-core/src/airflow/dag_processing/manager.py:
##########
@@ -1195,10 +1195,18 @@ def process_parse_results(
run_count=run_count + 1,
)
- # TODO: AIP-66 emit metrics
- # file_name = Path(dag_file.path).stem
- # Stats.timing(f"dag_processing.last_duration.{file_name}", stat.last_duration)
- # Stats.timing("dag_processing.last_duration", stat.last_duration, tags={"file_name": file_name})
+ # Note: relative_fileloc has a None default. In practice it is always provided but code defensively here in case
+ if relative_fileloc is not None and stat.last_duration is not None:
+ # Normalize names to ensure they only contain valid characters for stats (alphanumeric, underscore, dot, dash)
+ file_name = normalize_name_for_stats(Path(relative_fileloc).stem)
+ # bundle_name is included to distinguish files with the same name across different bundles
+ normalized_bundle = normalize_name_for_stats(bundle_name)
+ Stats.timing(f"dag_processing.last_duration.{normalized_bundle}.{file_name}", stat.last_duration)
+ Stats.timing(
+ "dag_processing.last_duration",
+ stat.last_duration,
+ tags={"file_name": file_name, "bundle_name": normalized_bundle},
Review Comment:
Its all tradeoffs, happy with the "consistent" answer.
--
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] Re-enable missing metric: dag_processing.last_duration. [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on PR #61079:
URL: https://github.com/apache/airflow/pull/61079#issuecomment-3806579661
> Specifically curious to hear your thoughts on using relative_fileloc here.
That is the right thing to use, yes!
--
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] Re-enable missing metric: dag_processing.last_duration. [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #61079:
URL: https://github.com/apache/airflow/pull/61079#discussion_r2733244353
##########
airflow-core/src/airflow/dag_processing/manager.py:
##########
@@ -1195,10 +1195,11 @@ def process_parse_results(
run_count=run_count + 1,
)
- # TODO: AIP-66 emit metrics
- # file_name = Path(dag_file.path).stem
- # Stats.timing(f"dag_processing.last_duration.{file_name}", stat.last_duration)
- # Stats.timing("dag_processing.last_duration", stat.last_duration, tags={"file_name": file_name})
+ # Note: relative_fileloc has a None default. In practice it is always provided but code defensively here in case
+ if relative_fileloc is not None and stat.last_duration is not None:
+ file_name = Path(relative_fileloc).stem
+ Stats.timing(f"dag_processing.last_duration.{file_name}", stat.last_duration)
Review Comment:
Filenames have more allowed characters than are allowed for metrics is all. Basically a bug that we weren't sanitizing them before.
--
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] Re-enable missing metric: dag_processing.last_duration. [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas merged PR #61079:
URL: https://github.com/apache/airflow/pull/61079
--
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] Re-enable missing metric: dag_processing.last_duration. [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #61079:
URL: https://github.com/apache/airflow/pull/61079#discussion_r2733009418
##########
airflow-core/src/airflow/dag_processing/manager.py:
##########
@@ -1195,10 +1195,11 @@ def process_parse_results(
run_count=run_count + 1,
)
- # TODO: AIP-66 emit metrics
- # file_name = Path(dag_file.path).stem
- # Stats.timing(f"dag_processing.last_duration.{file_name}", stat.last_duration)
- # Stats.timing("dag_processing.last_duration", stat.last_duration, tags={"file_name": file_name})
+ # Note: relative_fileloc has a None default. In practice it is always provided but code defensively here in case
+ if relative_fileloc is not None and stat.last_duration is not None:
+ file_name = Path(relative_fileloc).stem
+ Stats.timing(f"dag_processing.last_duration.{file_name}", stat.last_duration)
+ Stats.timing("dag_processing.last_duration", stat.last_duration, tags={"file_name": file_name})
Review Comment:
I think that would get confusing if we had multiple instances where the fileloc was None. It's generally a very undefined situation that would confuse users if we emitted metrics for it I think.
--
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] Re-enable missing metric: dag_processing.last_duration. [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #61079:
URL: https://github.com/apache/airflow/pull/61079#discussion_r2739620450
##########
airflow-core/src/airflow/dag_processing/manager.py:
##########
@@ -1195,10 +1195,18 @@ def process_parse_results(
run_count=run_count + 1,
)
- # TODO: AIP-66 emit metrics
- # file_name = Path(dag_file.path).stem
- # Stats.timing(f"dag_processing.last_duration.{file_name}", stat.last_duration)
- # Stats.timing("dag_processing.last_duration", stat.last_duration, tags={"file_name": file_name})
+ # Note: relative_fileloc has a None default. In practice it is always provided but code defensively here in case
+ if relative_fileloc is not None and stat.last_duration is not None:
+ # Normalize names to ensure they only contain valid characters for stats (alphanumeric, underscore, dot, dash)
+ file_name = normalize_name_for_stats(Path(relative_fileloc).stem)
+ # bundle_name is included to distinguish files with the same name across different bundles
+ normalized_bundle = normalize_name_for_stats(bundle_name)
+ Stats.timing(f"dag_processing.last_duration.{normalized_bundle}.{file_name}", stat.last_duration)
+ Stats.timing(
+ "dag_processing.last_duration",
+ stat.last_duration,
+ tags={"file_name": file_name, "bundle_name": normalized_bundle},
Review Comment:
Should this one be using the "raw" relative fileloc instead?
--
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] Re-enable missing metric: dag_processing.last_duration. [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on PR #61079:
URL: https://github.com/apache/airflow/pull/61079#issuecomment-3837527630
> Overall LGTM, but I'm not confident in reviewing shared stuff. Tagging @amoghrajesh!
Thanks @jedcunningham, fair enough if you don't feel comfortable approving. @amoghrajesh can you have a look for this one?
--
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] Re-enable missing metric: dag_processing.last_duration. [airflow]
Posted by "ferruzzi (via GitHub)" <gi...@apache.org>.
ferruzzi commented on code in PR #61079:
URL: https://github.com/apache/airflow/pull/61079#discussion_r2734273328
##########
airflow-core/tests/unit/dag_processing/test_manager.py:
##########
@@ -734,11 +733,20 @@ def test_send_file_processing_statsd_timing(
manager = DagFileProcessorManager(max_runs=1)
manager.run()
- last_runtime = manager._file_stats[os.fspath(path_to_parse)].last_duration
+ file_info = DagFileInfo(
+ bundle_name="testing",
+ rel_path=Path("temp_dag.py"),
Review Comment:
I guess it's a unit test, it's not a big deal, feel free to ignore and resolve.
--
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] Re-enable missing metric: dag_processing.last_duration. [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #61079:
URL: https://github.com/apache/airflow/pull/61079#discussion_r2733213200
##########
airflow-core/src/airflow/dag_processing/manager.py:
##########
@@ -1195,10 +1195,11 @@ def process_parse_results(
run_count=run_count + 1,
)
- # TODO: AIP-66 emit metrics
- # file_name = Path(dag_file.path).stem
- # Stats.timing(f"dag_processing.last_duration.{file_name}", stat.last_duration)
- # Stats.timing("dag_processing.last_duration", stat.last_duration, tags={"file_name": file_name})
+ # Note: relative_fileloc has a None default. In practice it is always provided but code defensively here in case
+ if relative_fileloc is not None and stat.last_duration is not None:
+ file_name = Path(relative_fileloc).stem
+ Stats.timing(f"dag_processing.last_duration.{file_name}", stat.last_duration)
Review Comment:
I'll look into this. What are we worried about here in particular? We weren't sanitizing the filename before
--
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] Re-enable missing metric: dag_processing.last_duration. [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on PR #61079:
URL: https://github.com/apache/airflow/pull/61079#issuecomment-3800786689
@jedcunningham As the original author, can you have a look at this one? Specifically curious to hear your thoughts on using `relative_fileloc` here. Thanks!
--
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] Re-enable missing metric: dag_processing.last_duration. [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #61079:
URL: https://github.com/apache/airflow/pull/61079#discussion_r2738499552
##########
airflow-core/tests/unit/dag_processing/test_manager.py:
##########
@@ -734,11 +733,20 @@ def test_send_file_processing_statsd_timing(
manager = DagFileProcessorManager(max_runs=1)
manager.run()
- last_runtime = manager._file_stats[os.fspath(path_to_parse)].last_duration
+ file_info = DagFileInfo(
+ bundle_name="testing",
+ rel_path=Path("temp_dag.py"),
Review Comment:
I'll create a variable with the dag name suppose and use that in both places
--
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] Re-enable missing metric: dag_processing.last_duration. [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #61079:
URL: https://github.com/apache/airflow/pull/61079#discussion_r2738502865
##########
shared/observability/src/airflow_shared/observability/metrics/stats.py:
##########
@@ -29,6 +30,36 @@
log = logging.getLogger(__name__)
+_VALID_STAT_NAME_CHARS_RE = re.compile(r"^[a-zA-Z0-9_.-]+$")
+_INVALID_STAT_NAME_CHARS_RE = re.compile(r"[^a-zA-Z0-9_.-]")
Review Comment:
Yeah, on this one I think I'll keep it as is. I really just want to move over the impl that existed before with no changes. It's very possible to generalize this further but I think that drifts too far from the original intention of this PR
--
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] Re-enable missing metric: dag_processing.last_duration. [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #61079:
URL: https://github.com/apache/airflow/pull/61079#discussion_r2733129320
##########
airflow-core/src/airflow/dag_processing/manager.py:
##########
@@ -1195,10 +1195,11 @@ def process_parse_results(
run_count=run_count + 1,
)
- # TODO: AIP-66 emit metrics
- # file_name = Path(dag_file.path).stem
- # Stats.timing(f"dag_processing.last_duration.{file_name}", stat.last_duration)
- # Stats.timing("dag_processing.last_duration", stat.last_duration, tags={"file_name": file_name})
+ # Note: relative_fileloc has a None default. In practice it is always provided but code defensively here in case
+ if relative_fileloc is not None and stat.last_duration is not None:
+ file_name = Path(relative_fileloc).stem
+ Stats.timing(f"dag_processing.last_duration.{file_name}", stat.last_duration)
Review Comment:
We should likely include the bundle name as well.
##########
airflow-core/src/airflow/dag_processing/manager.py:
##########
@@ -1195,10 +1195,11 @@ def process_parse_results(
run_count=run_count + 1,
)
- # TODO: AIP-66 emit metrics
- # file_name = Path(dag_file.path).stem
- # Stats.timing(f"dag_processing.last_duration.{file_name}", stat.last_duration)
- # Stats.timing("dag_processing.last_duration", stat.last_duration, tags={"file_name": file_name})
+ # Note: relative_fileloc has a None default. In practice it is always provided but code defensively here in case
+ if relative_fileloc is not None and stat.last_duration is not None:
+ file_name = Path(relative_fileloc).stem
Review Comment:
I know it was like this before, but only having the filename feels not unique enough to me...
##########
airflow-core/src/airflow/dag_processing/manager.py:
##########
@@ -1195,10 +1195,11 @@ def process_parse_results(
run_count=run_count + 1,
)
- # TODO: AIP-66 emit metrics
- # file_name = Path(dag_file.path).stem
- # Stats.timing(f"dag_processing.last_duration.{file_name}", stat.last_duration)
- # Stats.timing("dag_processing.last_duration", stat.last_duration, tags={"file_name": file_name})
+ # Note: relative_fileloc has a None default. In practice it is always provided but code defensively here in case
+ if relative_fileloc is not None and stat.last_duration is not None:
+ file_name = Path(relative_fileloc).stem
+ Stats.timing(f"dag_processing.last_duration.{file_name}", stat.last_duration)
Review Comment:
I also wonder if we need to sanitize the filename potentially, similar to `normalize_pool_name_for_stats`?
--
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] Re-enable missing metric: dag_processing.last_duration. [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #61079:
URL: https://github.com/apache/airflow/pull/61079#discussion_r2733208337
##########
airflow-core/src/airflow/dag_processing/manager.py:
##########
@@ -1195,10 +1195,11 @@ def process_parse_results(
run_count=run_count + 1,
)
- # TODO: AIP-66 emit metrics
- # file_name = Path(dag_file.path).stem
- # Stats.timing(f"dag_processing.last_duration.{file_name}", stat.last_duration)
- # Stats.timing("dag_processing.last_duration", stat.last_duration, tags={"file_name": file_name})
+ # Note: relative_fileloc has a None default. In practice it is always provided but code defensively here in case
+ if relative_fileloc is not None and stat.last_duration is not None:
+ file_name = Path(relative_fileloc).stem
+ Stats.timing(f"dag_processing.last_duration.{file_name}", stat.last_duration)
Review Comment:
Fair callout, I'll include bundle name in the metric. It will be different than it was in 2.X, but hey it was a major release so that's okay :)
--
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] Re-enable missing metric: dag_processing.last_duration. [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #61079:
URL: https://github.com/apache/airflow/pull/61079#discussion_r2742928431
##########
airflow-core/src/airflow/dag_processing/manager.py:
##########
@@ -1195,10 +1195,18 @@ def process_parse_results(
run_count=run_count + 1,
)
- # TODO: AIP-66 emit metrics
- # file_name = Path(dag_file.path).stem
- # Stats.timing(f"dag_processing.last_duration.{file_name}", stat.last_duration)
- # Stats.timing("dag_processing.last_duration", stat.last_duration, tags={"file_name": file_name})
+ # Note: relative_fileloc has a None default. In practice it is always provided but code defensively here in case
+ if relative_fileloc is not None and stat.last_duration is not None:
+ # Normalize names to ensure they only contain valid characters for stats (alphanumeric, underscore, dot, dash)
+ file_name = normalize_name_for_stats(Path(relative_fileloc).stem)
+ # bundle_name is included to distinguish files with the same name across different bundles
+ normalized_bundle = normalize_name_for_stats(bundle_name)
+ Stats.timing(f"dag_processing.last_duration.{normalized_bundle}.{file_name}", stat.last_duration)
+ Stats.timing(
+ "dag_processing.last_duration",
+ stat.last_duration,
+ tags={"file_name": file_name, "bundle_name": normalized_bundle},
Review Comment:
Hmm, I'm not sure either. It seemed the most consistent to provide the same sanitized filename to each stat backend. I think statsd is a subset of what otel provides, so it should be safe and then the experience will be the same for users. Is there are downside I'm not seeing that's on your radar 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] Re-enable missing metric: dag_processing.last_duration. [airflow]
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #61079:
URL: https://github.com/apache/airflow/pull/61079#discussion_r2742832447
##########
airflow-core/src/airflow/dag_processing/manager.py:
##########
@@ -1195,10 +1195,18 @@ def process_parse_results(
run_count=run_count + 1,
)
- # TODO: AIP-66 emit metrics
- # file_name = Path(dag_file.path).stem
- # Stats.timing(f"dag_processing.last_duration.{file_name}", stat.last_duration)
- # Stats.timing("dag_processing.last_duration", stat.last_duration, tags={"file_name": file_name})
+ # Note: relative_fileloc has a None default. In practice it is always provided but code defensively here in case
+ if relative_fileloc is not None and stat.last_duration is not None:
+ # Normalize names to ensure they only contain valid characters for stats (alphanumeric, underscore, dot, dash)
+ file_name = normalize_name_for_stats(Path(relative_fileloc).stem)
+ # bundle_name is included to distinguish files with the same name across different bundles
+ normalized_bundle = normalize_name_for_stats(bundle_name)
+ Stats.timing(f"dag_processing.last_duration.{normalized_bundle}.{file_name}", stat.last_duration)
+ Stats.timing(
+ "dag_processing.last_duration",
+ stat.last_duration,
+ tags={"file_name": file_name, "bundle_name": normalized_bundle},
Review Comment:
I genuinely don't know the right call here, but seems like if otel is happy with the non-sanitized filename, that'd be better to pass.
--
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