ashb commented on code in PR #63932:
URL: https://github.com/apache/airflow/pull/63932#discussion_r3154564171
##########
airflow-core/tests/unit/jobs/test_scheduler_job.py:
##########
@@ -436,8 +436,10 @@ def test_only_idle_with_dag_exits_after_n_idle_runs(self,
caplog, dag_maker, ses
assert total_runs_val > idle_runs_val, "Some runs should not be idle"
@mock.patch("airflow.jobs.scheduler_job_runner.TaskCallbackRequest")
- @mock.patch("airflow.jobs.scheduler_job_runner.Stats.incr")
- def test_process_executor_events(self, mock_stats_incr,
mock_task_callback, dag_maker):
+ @mock.patch("airflow._shared.observability.metrics.stats._get_backend")
+ def test_process_executor_events(self, mock_get_backend,
mock_task_callback, dag_maker):
+ mock_stats = mock.MagicMock()
Review Comment:
`mock.MagicMock()` without a `spec` silently accepts any attribute or call —
if production code calls a misspelled method the test still passes.
I think we can use `spec=StatsLogger` (from
`airflow._shared.observability.metrics.base_stats_logger`). Same issue on the
other ~10 sites in this file that follow the same pattern.
##########
shared/observability/src/airflow_shared/observability/metrics/metrics_template.yaml:
##########
@@ -564,8 +564,8 @@ metrics:
- name: "dag_processing.last_duration"
description: "Milliseconds taken to load the given Dag file"
Review Comment:
The legacy name format for `dag_processing.last_duration` changed from
`dag_processing.last_duration.{dag_file}` to
`dag_processing.last_duration.{bundle_name}.{file_name}`. Anyone currently
alerting or dashboarding on the old format (e.g.
`dag_processing.last_duration.my_dag_file`) will silently lose those metrics
after upgrade — the old name simply stops being emitted. Worth a note in the
migration docs or a deprecation warning before the hard switch.
##########
providers/openlineage/src/airflow/providers/openlineage/plugins/listener.py:
##########
@@ -86,7 +86,10 @@ def _executor_initializer():
try:
from airflow.observability.metrics import stats_utils
- Stats.initialize(factory=stats_utils.get_stats_factory(Stats))
+ Stats.initialize(
+ factory=stats_utils.get_stats_factory(),
+ export_legacy_names=conf.getboolean("metrics", "legacy_names_on"),
Review Comment:
```suggestion
export_legacy_names=conf.getboolean("metrics",
"legacy_names_on", fallback=True),
```
(Not sure about the fallback value) but I think we need _a_ fallback in
providers, otherwise if this new provider version is used against an Airflow <
3.2.0 release it will fail with a config not found exc?
##########
airflow-core/src/airflow/plugins_manager.py:
##########
@@ -128,7 +128,8 @@ def __register_plugins(plugin_instances:
list[AirflowPlugin], errors: dict[str,
if not settings.LAZY_LOAD_PROVIDERS:
__register_plugins(*_load_providers_plugins())
- log.debug("Loading %d plugin(s) took %.2f ms", len(plugins),
timer.duration)
+ if hasattr(timer, "duration"):
Review Comment:
This shouldn't be required anymore? I thought the protocol always has it.
##########
providers/openlineage/src/airflow/providers/openlineage/plugins/listener.py:
##########
@@ -230,7 +233,8 @@ def on_running():
if not doc:
doc, doc_type = get_dag_documentation(dag)
- with Stats.timer(f"ol.extract.{event_type}.{operator_name}"):
+ ctx = Stats.timer("ol.extract", tags={"event_type": event_type,
"operator_name": operator_name})
+ with ctx:
Review Comment:
Pointless `ctx` variable? Or just a formatting affordance so it doesn't line
wrap?
##########
providers/edge3/src/airflow/providers/edge3/executors/edge_executor.py:
##########
@@ -225,10 +225,6 @@ def _update_orphaned_jobs(self, session: Session) -> bool:
"queue": job.queue,
"state": str(TaskInstanceState.FAILED),
}
- Stats.incr(
-
f"edge_worker.ti.finish.{job.queue}.{TaskInstanceState.FAILED}.{job.dag_id}.{job.task_id}",
- tags=tags,
- )
Stats.incr("edge_worker.ti.finish", tags=tags)
Review Comment:
Removed on purpose? (I think so, just confirming)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
