Skip to content

Remove TaskContextLogger #43183

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dstandish merged 2 commits into from
Oct 19, 2024
Merged

Remove TaskContextLogger #43183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f73de5f
Remove TaskContextLogger
dstandish Oct 18, 2024
8a5e543
add newsfragment
dstandish Oct 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 0 additions & 12 deletions airflow/config_templates/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1032,18 +1032,6 @@ logging:
type: boolean
example: ~
default: "False"
enable_task_context_logger:
description: |
If enabled, Airflow may ship messages to task logs from outside the task run context, e.g. from
the scheduler, executor, or callback execution context. This can help in circumstances such as
when there's something blocking the execution of the task and ordinarily there may be no task
logs at all.
This is set to ``True`` by default. If you encounter issues with this feature
(e.g. scheduler performance issues) it can be disabled.
version_added: 2.8.0
type: boolean
example: ~
default: "True"
color_log_error_keywords:
description: |
A comma separated list of keywords related to errors whose presence should display the line in red
Expand Down
5 changes: 0 additions & 5 deletions airflow/utils/log/file_task_handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@

from __future__ import annotations

import inspect
import logging
import os
from contextlib import suppress
Expand Down Expand Up @@ -238,10 +237,6 @@ def set_context(self, ti: TaskInstance, *, identifier: str | None = None) -> Non
self.handler.setLevel(self.level)
return SetContextPropagate.MAINTAIN_PROPAGATE if self.maintain_propagate else None

@cached_property
def supports_task_context_logging(self) -> bool:
return "identifier" in inspect.signature(self.set_context).parameters
Copy link
Member

@pankajkoti pankajkoti Oct 19, 2024

Choose a reason for hiding this comment

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

Do we intend to keep accepting the identifier param in the set_context method above that we had introduced in https://github.com/apache/airflow/pull/32646/files. The docstring for that param in the method is currently tightly related to the task context logger.

This param was also extended to be used in the set_context method of the provider log handlers with the below PRs

  1. AWS S3: Extend task context logging support for remote logging using AWS S3 #32950
  2. GCP GCS: Extend task context logging support for remote logging using GCS #32970
  3. Azure WASB: Extend task context logging support for remote logging using WASB (Azure Blob Storage) #32972
  4. ElasticSearch: Extend task context logging support for remote logging using Elasticsearch #32977

It seems the conditionals around AF versions in those provider PRs were removed & cleaned up when those providers started having Airflow min version as 2.8 -- in PR #42764

If we decide to drop the param, perhaps would also be a good idea to add a TODO in those providers saying that this param would no longer be needed/used when min AF version is 3.0 & can be cleaned up.

Copy link
Contributor Author

@dstandish dstandish Oct 19, 2024

Choose a reason for hiding this comment

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

It's a good point. I thought about it and figured that it's ok to leave it even if it is not currently used. Unless there's some backcompat issue with leaving it but i don't think there is.

Do you see any other backcompat concerns with this PR otherwise?

Copy link
Member

@potiuk potiuk Oct 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah. I think we should drop it - and actually - if we are going to add documentation for Airlfow 3, I think we should promote and look more closely into open-telemetry trace integration, which is essentially doing exactly what have been done here (and more).

Open Telemetry span is equivalent to the identifier, but there is already tooling (all tools that support open-telemetry spans) that understand and are able to visualise spans across distributed systems. So I'd say we should make it clearer and explain to people on how they can connect those tools to see relations between things like errors raised in other parts of the system that are related to particular tasks.

This was a very important point raised in the debugging survey - that it is difficult to debug issues in a distributed environment, and IMHO open-telemetry and better integration with it is the answer to that need and maybe we shoudl somehow think about making it more obvious (and learn ourselves how to do it).

Copy link
Member

@pankajkoti pankajkoti Oct 20, 2024

Choose a reason for hiding this comment

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

Do you see any other backcompat concerns with this PR otherwise?

I can't think of any backcompat issues otherwise :), just felt that we would have there an unused param and thought to highlight it once.


@staticmethod
def add_triggerer_suffix(full_path, job_id=None):
"""
Expand Down
188 changes: 0 additions & 188 deletions airflow/utils/log/task_context_logger.py
View file
Open in desktop

This file was deleted.

5 changes: 5 additions & 0 deletions newsfragments/43183.significant.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Remove TaskContextLogger

We introduced this as a way to inject messages into task logs from places
other than the task execution context. We later realized that we were better off
just using the Log table.
2 changes: 1 addition & 1 deletion providers/src/airflow/providers/amazon/CHANGELOG.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -788,7 +788,7 @@ Misc
.. Below changes are excluded from the changelog. Move them to
appropriate section above if needed. Do not delete the lines(!):
* ``Use reproducible builds for provider packages (#35693)``
* ``Update http to s3 system test (#35711)``
* ``Update http to s3 system test (#35711)``

8.11.0
......
Expand Down
139 changes: 0 additions & 139 deletions tests/utils/log/test_task_context_logger.py
View file
Open in desktop

This file was deleted.

4 changes: 1 addition & 3 deletions tests_common/test_utils/mock_executor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
from __future__ import annotations

from collections import defaultdict
from unittest.mock import MagicMock, Mock
from unittest.mock import MagicMock

from airflow.executors.base_executor import BaseExecutor
from airflow.executors.executor_utils import ExecutorName
Expand Down Expand Up @@ -52,8 +52,6 @@ def __init__(self, do_update=True, *args, **kwargs):

super().__init__(*args, **kwargs)

self.task_context_logger = Mock()

def success(self):
return State.SUCCESS

Expand Down