Skip to content

Clarify logging_config_class contract and document REMOTE_TASK_LOG - #67104

Merged
vatsrahul1001 merged 8 commits into
apache:mainfrom
jason810496:refactor/logging/clarify-logging-config-class-core
Jul 28, 2026
Merged

Clarify logging_config_class contract and document REMOTE_TASK_LOG#67104
vatsrahul1001 merged 8 commits into
apache:mainfrom
jason810496:refactor/logging/clarify-logging-config-class-core

Conversation

@jason810496

@jason810496 jason810496 commented May 18, 2026

Copy link
Copy Markdown
Member

Why

[logging] logging_config_class is documented as a "Logging class" but actually resolves to a logging.config.dictConfig dict, and the REMOTE_TASK_LOG / DEFAULT_REMOTE_CONN_ID side channel that powers remote log read-back was undocumented.

So custom configs silently lost UI log read-back, and ElasticsearchTaskHandler / OpensearchTaskHandler papered over it by self-registering from inside __init__. The provider-side deprecation of that self-registration is split into follow-up PRs (one for elasticsearch, one for opensearch); this PR is the core/SDK/shared-library piece they depend on.

How

  • Document the real contract for logging_config_class (dict, not class) and the REMOTE_TASK_LOG / DEFAULT_REMOTE_CONN_ID module-level attributes in the config option help, advanced-logging-configuration.rst, and the discover_remote_log_handler docstring.
  • Add a startup WARNING for the case that "user module is missing REMOTE_TASK_LOG while remote_logging is on". The warning is emitted from configure_logging after dictConfig runs, so the check sees the final state — important because the deprecated ES/OS self-registration path populates _ActiveLoggingConfig.remote_task_log from inside the handler __init__.
  • Drop the remote_logging_enabled parameter from discover_remote_log_handler (no longer needed now that the warning lives in configure_logging).
  • Add unit tests for _warn_if_missing_remote_task_log.

Was generative AI tooling used to co-author this PR?

@amoghrajesh

Copy link
Copy Markdown
Contributor

@jason810496 I wanna take a look at this one but do not have b/w atm, will do soon

@eladkal

eladkal commented May 20, 2026

Copy link
Copy Markdown
Contributor

I think we will need to bump

"elasticsearch": parse_version("6.5.0"),
"opensearch": parse_version("1.9.0"),

as part of this PR but we will need to wait for providers to be released

@eladkal eladkal added this to the Airflow 3.3.0 milestone May 23, 2026
@eladkal

eladkal commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

@jason810496 can you fix the problem above?

Comment thread airflow-core/src/airflow/config_templates/config.yml Outdated
@jason810496
jason810496 marked this pull request as draft June 5, 2026 14:22
@jason810496
jason810496 force-pushed the refactor/logging/clarify-logging-config-class-core branch 3 times, most recently from 584e906 to eef5d24 Compare June 5, 2026 14:43
@jason810496
jason810496 marked this pull request as ready for review June 5, 2026 14:44

@jason810496 jason810496 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review Elad, Phani. I addressed both comments in last rebase.

@jason810496
jason810496 marked this pull request as draft June 9, 2026 02:22
@jason810496
jason810496 force-pushed the refactor/logging/clarify-logging-config-class-core branch from eef5d24 to 654cbec Compare June 9, 2026 02:48

@jason810496 jason810496 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @eladkal,
I some help to resolve the static check CI failure when you have a moment. Thanks.

If I don't update the root pyproject.toml I will encounter:

Update Airflow's meta-package pyproject.toml............................................................Failed
  - hook id: update-pyproject-toml
  - files were modified by this hook

All changes made by hooks:
diff --git a/pyproject.toml b/pyproject.toml
index bae4c06..5a6f250 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -219,7 +219,7 @@ apache-airflow = "airflow.__main__:main"
     "apache-airflow-providers-edge3>=1.0.0"
 ]
 "elasticsearch" = [
-    "apache-airflow-providers-elasticsearch>=6.5.0" # Set from MIN_VERSION_OVERRIDE in update_airflow_pyproject_toml.py
+    "apache-airflow-providers-elasticsearch>=6.6.0" # Set from MIN_VERSION_OVERRIDE in update_airflow_pyproject_toml.py
 ]
 "exasol" = [
     "apache-airflow-providers-exasol>=4.6.1"
@@ -306,7 +306,7 @@ apache-airflow = "airflow.__main__:main"
     "apache-airflow-providers-openlineage>=2.3.0" # Set from MIN_VERSION_OVERRIDE in update_airflow_pyproject_toml.py
 ]
 "opensearch" = [
-    "apache-airflow-providers-opensearch>=1.9.0" # Set from MIN_VERSION_OVERRIDE in update_airflow_pyproject_toml.py
+    "apache-airflow-providers-opensearch>=1.9.3" # Set from MIN_VERSION_OVERRIDE in update_airflow_pyproject_toml.py
 ]
 "opsgenie" = [
     "apache-airflow-providers-opsgenie>=5.8.0"
@@ -447,7 +447,7 @@ apache-airflow = "airflow.__main__:main"
     "apache-airflow-providers-discord>=3.9.0",
     "apache-airflow-providers-docker>=3.14.1",
     "apache-airflow-providers-edge3>=1.0.0",
-    "apache-airflow-providers-elasticsearch>=6.5.0", # Set from MIN_VERSION_OVERRIDE in update_airflow_pyproject_toml.py
+    "apache-airflow-providers-elasticsearch>=6.6.0", # Set from MIN_VERSION_OVERRIDE in update_airflow_pyproject_toml.py
     "apache-airflow-providers-exasol>=4.6.1",
     "apache-airflow-providers-fab>=3.6.0", # Set from MIN_VERSION_OVERRIDE in update_airflow_pyproject_toml.py
     "apache-airflow-providers-facebook>=3.7.0",
@@ -476,7 +476,7 @@ apache-airflow = "airflow.__main__:main"
     "apache-airflow-providers-openai>=1.5.0",
     "apache-airflow-providers-openfaas>=3.7.0",
     "apache-airflow-providers-openlineage>=2.3.0", # Set from MIN_VERSION_OVERRIDE in update_airflow_pyproject_toml.py
-    "apache-airflow-providers-opensearch>=1.9.0", # Set from MIN_VERSION_OVERRIDE in update_airflow_pyproject_toml.py
+    "apache-airflow-providers-opensearch>=1.9.3", # Set from MIN_VERSION_OVERRIDE in update_airflow_pyproject_toml.py
     "apache-airflow-providers-opsgenie>=5.8.0",
     "apache-airflow-providers-oracle>=3.12.0",
     "apache-airflow-providers-pagerduty>=3.8.1",

Fail run: https://github.com/apache/airflow/actions/runs/27183757151/job/80249517727

However, if I update the pyproject.toml. I will encounter the following error from breeze ci selective-check for the CI Image check:

Provider dependency version bumps detected that should only be performed by Release Managers!

  - pyproject.toml : apache-airflow-providers-elasticsearch >= version changed from 6.5.0 to 6.6.0
  - pyproject.toml : apache-airflow-providers-opensearch >= version changed from 1.9.0 to 1.9.3
  - pyproject.toml : apache-airflow-providers-elasticsearch >= version changed from 6.5.0 to 6.6.0
  - pyproject.toml : apache-airflow-providers-opensearch >= version changed from 1.9.0 to 1.9.3

Fail run: https://github.com/apache/airflow/actions/runs/27185728163/job/80254150717

@eladkal

eladkal commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

I think you just need to add allow provider dependency bump label and rebase the PR. That will make breeze ci selective-check pass.
I assume release manager @vatsrahul1001 is OK with this

@vatsrahul1001 vatsrahul1001 added this to the Airflow 3.3.1 milestone Jun 25, 2026
@eladkal
eladkal force-pushed the refactor/logging/clarify-logging-config-class-core branch from b5b681b to 2f5ae76 Compare July 2, 2026 04:29
@eladkal eladkal added the backport-to-v3-3-test Backport to v3-3-test label Jul 2, 2026
@jason810496
jason810496 force-pushed the refactor/logging/clarify-logging-config-class-core branch 2 times, most recently from 9079e0a to ba8c945 Compare July 6, 2026 06:28

@Lee-W Lee-W left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few nits. nothing major

Comment thread airflow-core/src/airflow/logging_config.py Outdated
Comment thread airflow-core/tests/unit/logging/test_logging_config.py Outdated
Comment thread airflow-core/tests/unit/logging/test_logging_config.py Outdated
jason810496 added a commit to jason810496/airflow that referenced this pull request Jul 7, 2026
An empty ``logging_config_class`` falls back to the default, so treating
it as user-defined under the old ``user_defined`` name was ambiguous per
review feedback. Rename it to state what it actually checks and document
why the empty-path case is excluded. Also collapse the near-duplicate
``TestWarnIfMissingRemoteTaskLog`` tests into one parametrized test using
the project's ``conf_vars`` helper for the config override, per review
suggestions on apache#67104.
Comment thread airflow-core/src/airflow/logging_config.py
Comment thread airflow-core/src/airflow/logging_config.py Outdated
@jason810496
jason810496 requested a review from Lee-W July 8, 2026 02:12
``[logging] logging_config_class`` is documented as a "Logging class" but
actually resolves to a ``logging.config.dictConfig`` dict, and the
``REMOTE_TASK_LOG`` / ``DEFAULT_REMOTE_CONN_ID`` side channel that powers
remote log read-back was undocumented. Custom configs silently lost UI log
read-back as a result.

- Document the real contract for ``logging_config_class`` (dict, not class)
  and the ``REMOTE_TASK_LOG`` / ``DEFAULT_REMOTE_CONN_ID`` module-level
  attributes in the config option help, ``advanced-logging-configuration.rst``,
  and the ``discover_remote_log_handler`` docstring.
- Add a startup ``WARNING`` when ``remote_logging`` is on but the user's
  logging module is missing ``REMOTE_TASK_LOG``, emitted from
  ``configure_logging`` after ``dictConfig`` runs so it sees the final state.
Document only REMOTE_TASK_LOG / DEFAULT_REMOTE_CONN_ID in the new remote logging section; do not show users a LOGGING_CONFIG dict to build.
An empty ``logging_config_class`` falls back to the default, so treating
it as user-defined under the old ``user_defined`` name was ambiguous per
review feedback. Rename it to state what it actually checks and document
why the empty-path case is excluded. Also collapse the near-duplicate
``TestWarnIfMissingRemoteTaskLog`` tests into one parametrized test using
the project's ``conf_vars`` helper for the config override, per review
suggestions on apache#67104.
…s handling

_ActiveLoggingConfig.remote_task_log had no default, so
_warn_if_missing_remote_task_log() raised AttributeError if it ran
before _load_logging_config() ever populated the class. A short
comment also clarifies that the `or DEFAULT_LOGGING_CONFIG_PATH`
fallback intentionally covers an explicitly empty
`logging_config_class = ""`.
@jason810496
jason810496 force-pushed the refactor/logging/clarify-logging-config-class-core branch from 66b52a6 to 9282def Compare July 13, 2026 05:54

@SZL741023 SZL741023 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems necessary to actually obtain the remote task log.

Comment thread airflow-core/src/airflow/logging_config.py Outdated
The check read _ActiveLoggingConfig.remote_task_log directly, which is
only populated once something has triggered resolution (previously the
deprecated Elasticsearch/OpenSearch handler self-registration during
dictConfig). A user with a custom logging_config_class whose remote
logging actually resolves through ProvidersManager dispatch -- with no
ES/OS handler in the mix -- got a false-positive warning because the
cache was still cold at check time. Going through get_remote_task_log()
triggers the real resolution lazily, so the check reflects whether
remote logging is actually available.
@vatsrahul1001
vatsrahul1001 requested a review from phanikumv July 28, 2026 10:32
@vatsrahul1001
vatsrahul1001 merged commit d8b8620 into apache:main Jul 28, 2026
303 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

Backport successfully created: v3-3-test

Note: As of Merging PRs targeted for Airflow 3.X
the committer who merges the PR is responsible for backporting the PRs that are bug fixes (generally speaking) to the maintenance branches.

In matter of doubt please ask in #release-management Slack channel.

Status Branch Result
v3-3-test PR Link

github-actions Bot pushed a commit to aws-mwaa/upstream-to-airflow that referenced this pull request Jul 28, 2026
…REMOTE_TASK_LOG`` (apache#67104)

* Clarify ``logging_config_class`` contract and document REMOTE_TASK_LOG

``[logging] logging_config_class`` is documented as a "Logging class" but
actually resolves to a ``logging.config.dictConfig`` dict, and the
``REMOTE_TASK_LOG`` / ``DEFAULT_REMOTE_CONN_ID`` side channel that powers
remote log read-back was undocumented. Custom configs silently lost UI log
read-back as a result.

- Document the real contract for ``logging_config_class`` (dict, not class)
  and the ``REMOTE_TASK_LOG`` / ``DEFAULT_REMOTE_CONN_ID`` module-level
  attributes in the config option help, ``advanced-logging-configuration.rst``,
  and the ``discover_remote_log_handler`` docstring.
- Add a startup ``WARNING`` when ``remote_logging`` is on but the user's
  logging module is missing ``REMOTE_TASK_LOG``, emitted from
  ``configure_logging`` after ``dictConfig`` runs so it sees the final state.

* Fix CI error

* CI: Fix pyproject.toml

* Fix pyproject.toml

* Drop LOGGING_CONFIG dict from remote logging docs section

Document only REMOTE_TASK_LOG / DEFAULT_REMOTE_CONN_ID in the new remote logging section; do not show users a LOGGING_CONFIG dict to build.

* Clarify user-defined logging config detection and simplify warning tests

An empty ``logging_config_class`` falls back to the default, so treating
it as user-defined under the old ``user_defined`` name was ambiguous per
review feedback. Rename it to state what it actually checks and document
why the empty-path case is excluded. Also collapse the near-duplicate
``TestWarnIfMissingRemoteTaskLog`` tests into one parametrized test using
the project's ``conf_vars`` helper for the config override, per review
suggestions on apache#67104.

* Default remote_task_log to None and clarify empty logging_config_class handling

_ActiveLoggingConfig.remote_task_log had no default, so
_warn_if_missing_remote_task_log() raised AttributeError if it ran
before _load_logging_config() ever populated the class. A short
comment also clarifies that the `or DEFAULT_LOGGING_CONFIG_PATH`
fallback intentionally covers an explicitly empty
`logging_config_class = ""`.

* Resolve missing-REMOTE_TASK_LOG warning via get_remote_task_log()

The check read _ActiveLoggingConfig.remote_task_log directly, which is
only populated once something has triggered resolution (previously the
deprecated Elasticsearch/OpenSearch handler self-registration during
dictConfig). A user with a custom logging_config_class whose remote
logging actually resolves through ProvidersManager dispatch -- with no
ES/OS handler in the mix -- got a false-positive warning because the
cache was still cold at check time. Going through get_remote_task_log()
triggers the real resolution lazily, so the check reflects whether
remote logging is actually available.
(cherry picked from commit d8b8620)

Co-authored-by: Jason(Zhe-You) Liu <68415893+jason810496@users.noreply.github.com>
aws-airflow-bot pushed a commit to aws-mwaa/upstream-to-airflow that referenced this pull request Jul 28, 2026
…REMOTE_TASK_LOG`` (apache#67104)

* Clarify ``logging_config_class`` contract and document REMOTE_TASK_LOG

``[logging] logging_config_class`` is documented as a "Logging class" but
actually resolves to a ``logging.config.dictConfig`` dict, and the
``REMOTE_TASK_LOG`` / ``DEFAULT_REMOTE_CONN_ID`` side channel that powers
remote log read-back was undocumented. Custom configs silently lost UI log
read-back as a result.

- Document the real contract for ``logging_config_class`` (dict, not class)
  and the ``REMOTE_TASK_LOG`` / ``DEFAULT_REMOTE_CONN_ID`` module-level
  attributes in the config option help, ``advanced-logging-configuration.rst``,
  and the ``discover_remote_log_handler`` docstring.
- Add a startup ``WARNING`` when ``remote_logging`` is on but the user's
  logging module is missing ``REMOTE_TASK_LOG``, emitted from
  ``configure_logging`` after ``dictConfig`` runs so it sees the final state.

* Fix CI error

* CI: Fix pyproject.toml

* Fix pyproject.toml

* Drop LOGGING_CONFIG dict from remote logging docs section

Document only REMOTE_TASK_LOG / DEFAULT_REMOTE_CONN_ID in the new remote logging section; do not show users a LOGGING_CONFIG dict to build.

* Clarify user-defined logging config detection and simplify warning tests

An empty ``logging_config_class`` falls back to the default, so treating
it as user-defined under the old ``user_defined`` name was ambiguous per
review feedback. Rename it to state what it actually checks and document
why the empty-path case is excluded. Also collapse the near-duplicate
``TestWarnIfMissingRemoteTaskLog`` tests into one parametrized test using
the project's ``conf_vars`` helper for the config override, per review
suggestions on apache#67104.

* Default remote_task_log to None and clarify empty logging_config_class handling

_ActiveLoggingConfig.remote_task_log had no default, so
_warn_if_missing_remote_task_log() raised AttributeError if it ran
before _load_logging_config() ever populated the class. A short
comment also clarifies that the `or DEFAULT_LOGGING_CONFIG_PATH`
fallback intentionally covers an explicitly empty
`logging_config_class = ""`.

* Resolve missing-REMOTE_TASK_LOG warning via get_remote_task_log()

The check read _ActiveLoggingConfig.remote_task_log directly, which is
only populated once something has triggered resolution (previously the
deprecated Elasticsearch/OpenSearch handler self-registration during
dictConfig). A user with a custom logging_config_class whose remote
logging actually resolves through ProvidersManager dispatch -- with no
ES/OS handler in the mix -- got a false-positive warning because the
cache was still cold at check time. Going through get_remote_task_log()
triggers the real resolution lazily, so the check reflects whether
remote logging is actually available.
(cherry picked from commit d8b8620)

Co-authored-by: Jason(Zhe-You) Liu <68415893+jason810496@users.noreply.github.com>
jason810496 pushed a commit that referenced this pull request Jul 29, 2026
vatsrahul1001 pushed a commit that referenced this pull request Aug 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

8 participants