Aaryan123456679 opened a new issue, #74072:
URL: https://github.com/apache/airflow/issues/74072
### Description
_No response_
### Use case/motivation
### Background
`shared/observability/src/airflow_shared/observability/metrics/stats.py` is
symlinked into several distributions (`airflow-core`, `task-sdk`) and imported
under different module names (`airflow._shared...`, `airflow.sdk._shared...`).
Each copy has its own module globals, so each copy needs its own `Stats`
backend configured. Otherwise, code that reaches `Stats` through the task-sdk
path in a long-running process (for example plugin/listener hooks in the
scheduler) silently falls back to `NoStatsLogger` (#69172).
#69270 fixes this by having each copy configure itself lazily on first use.
`_self_configure()` derives its distribution root from `__name__` and calls
`import_module(f"{root}.observability.metrics.stats_utils")` and
`import_module(f"{root}.configuration")`.
### Problem
That makes a shared library depend on core/sdk modules at runtime, which the
shared-library boundary is meant to prevent. The
`check-airflow-imports-in-shared` hook doesn't catch it, for two reasons:
- It is an AST check that only flags `import` / `from ... import`
statements, so string-based `import_module(...)` calls are invisible to it.
- `stats.py` has been in the hook's `exclude:` list in
`.pre-commit-config.yaml` since the hook was introduced in #61350. A few other
shared files are excluded too.
This was already the case before #69270, which only uses the existing
exemption. It is still a boundary violation that CI can't see.
### Proposed follow-up
Make the `shared` Stats module self-contained, so it does not need to reach
into the consuming distribution:
1. Move `get_stats_factory` (and whatever it needs from config) into
`shared`, or have `shared` read the `[metrics]` configuration through an
interface that doesn't import core/sdk.
2. Remove `_self_configure()`'s `import_module` calls.
3. Remove `stats.py` from the `check-airflow-imports-in-shared` exclude list.
Optionally, extend the hook to also flag `import_module(...)` /
`__import__(...)` with `airflow.*` targets, so this can't regress unnoticed.
### Acceptance criteria
- No dynamic or static import of core/sdk modules from
`shared/observability/.../stats.py`.
- `stats.py` is no longer excluded from `check-airflow-imports-in-shared`.
- Core and task-sdk `Stats` copies still both resolve the correct backend
regardless of import order (existing `TestSelfConfigure` /
`TestSdkStatsSelfConfigures` tests keep passing).
### References
- #69172 (original bug), #69270 (fix, introduces the lazy self-configure)
- #61350 (introduced `check-airflow-imports-in-shared` and the exclusion)
### Related issues
_No response_
### Are you willing to submit a PR?
- [ ] Yes I am willing to submit a PR!
### Code of Conduct
- [x] I agree to follow this project's [Code of
Conduct](https://github.com/apache/airflow/blob/main/CODE_OF_CONDUCT.md)
--
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]
