[v3-3-test] Clarify logging_config_class contract and document REMOTE_TASK_LOG (#67104) - #70592
Merged
Merged
Conversation
…REMOTE_TASK_LOG`` (#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 #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>
1 task
Lee-W
marked this pull request as ready for review
July 28, 2026 11:17
Lee-W
requested review from
amoghrajesh,
ashb,
bugraoz93,
gopidesupavan,
jason810496,
jscheffl,
kaxil and
potiuk
as code owners
July 28, 2026 11:17
jason810496
approved these changes
Jul 28, 2026
Lee-W
approved these changes
Jul 28, 2026
jason810496
approved these changes
Jul 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
logging_config_classcontract and document REMOTE_TASK_LOG[logging] logging_config_classis documented as a "Logging class" butactually resolves to a
logging.config.dictConfigdict, and theREMOTE_TASK_LOG/DEFAULT_REMOTE_CONN_IDside channel that powersremote log read-back was undocumented. Custom configs silently lost UI log
read-back as a result.
logging_config_class(dict, not class)and the
REMOTE_TASK_LOG/DEFAULT_REMOTE_CONN_IDmodule-levelattributes in the config option help,
advanced-logging-configuration.rst,and the
discover_remote_log_handlerdocstring.WARNINGwhenremote_loggingis on but the user'slogging module is missing
REMOTE_TASK_LOG, emitted fromconfigure_loggingafterdictConfigruns 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.
An empty
logging_config_classfalls back to the default, so treatingit as user-defined under the old
user_definedname was ambiguous perreview feedback. Rename it to state what it actually checks and document
why the empty-path case is excluded. Also collapse the near-duplicate
TestWarnIfMissingRemoteTaskLogtests into one parametrized test usingthe project's
conf_varshelper for the config override, per reviewsuggestions on #67104.
_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_PATHfallback intentionally covers an explicitly empty
logging_config_class = "".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