Skip to content

Rework remote task log handling for the structlog era. - #48491

Merged
ashb merged 1 commit into
apache:mainfrom
astronomer:overhaul-task-logging
Apr 2, 2025
Merged

Rework remote task log handling for the structlog era.#48491
ashb merged 1 commit into
apache:mainfrom
astronomer:overhaul-task-logging

Conversation

@ashb

@ashbashb commented Mar 28, 2025•
edited
Loading

Copy link
Copy Markdown
Member

Previously this feature was built on top of the stdlib logging.Handler
interface, and it worked but had a few issues (even before we switched to
structlog for Task SDK):

  • we had to use what is in many ways a hack with the "set_context" to get
    information down in to the task handler.
  • Discovering of the configured task handler was somewhat baroque
  • The whole thing is just complex due to the features of stdlib logging
    (loggers, propagate, handler levels etc etc.)
  • The upload was triggered somewhat "automatically" inside close, which from
    an abstraction point of view is messy.

This changes things to have a more explicit interface purpose made for
uploading task log files and for reading them, and perhaps more crucially for
things like CloudWatch Logs, it (re)adds the ability to install a structlog
processor that will recieve every log message as it happens.

The return types for the read et al functions were confusing the living
daylights out of me, so I've created type alias to give the return types
explicit names to reduce (my) confusion.

@boring-cyborgboring-cyborgBot added area:logging area:providers area:task-sdk provider:amazon AWS/Amazon - related issues provider:google Google (including GCP) related issues provider:microsoft-azure Azure-related issues labels Mar 28, 2025
@ashbashb changed the title Rework remote task log handleing for the structlog era.🚧 Rework remote task log handleing for the structlog era.Mar 28, 2025
@ashbashb changed the title 🚧 Rework remote task log handleing for the structlog era.🚧 Rework remote task log handling for the structlog era.Mar 28, 2025
@ashb

This comment was marked as outdated.

Sign in to view
@ashb
ashbforce-pushed the overhaul-task-logging branch 2 times, most recently from e25bd96 to e43c7bbCompareMarch 31, 2025 15:48
@ashbashb changed the title 🚧 Rework remote task log handling for the structlog era.Rework remote task log handling for the structlog era.Mar 31, 2025
@ashb
ashb marked this pull request as ready for review March 31, 2025 15:49
kaxil
kaxil approved these changes Mar 31, 2025
View reviewed changes
Comment threadairflow-core/src/airflow/logging/remote.py Outdated
Comment threadairflow-core/src/airflow/logging_config.py Outdated
Comment threadairflow-core/src/airflow/logging_config.py Outdated
ashb
ashb commented Mar 31, 2025
View reviewed changes
Comment threadairflow-core/src/airflow/logging_config.py Outdated
ashb
ashb commented Mar 31, 2025
View reviewed changes
Comment threadairflow-core/src/airflow/logging_config.py Outdated
ashb
ashb commented Mar 31, 2025
View reviewed changes
Comment threadairflow-core/src/airflow/logging_config.py Outdated
amoghrajesh
amoghrajesh approved these changes Apr 1, 2025
View reviewed changes

@amoghrajeshamoghrajesh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM +1
Some nits

Comment threadairflow-core/src/airflow/logging_config.py Outdated
Comment threadairflow-core/src/airflow/utils/log/file_task_handler.py Outdated
o-nikolas
o-nikolas reviewed Apr 1, 2025
View reviewed changes
Comment threadairflow-core/src/airflow/utils/log/file_task_handler.py
Comment threadproviders/amazon/src/airflow/providers/amazon/aws/log/cloudwatch_task_handler.py
Comment threadproviders/amazon/tests/unit/amazon/aws/log/test_cloudwatch_task_handler.py
Comment threadtask-sdk/src/airflow/sdk/log.py
Comment threadtask-sdk/src/airflow/sdk/log.py Outdated
@eladkal

eladkal commented Apr 1, 2025

Copy link
Copy Markdown
Contributor

Does this affect elasticsearch / opensearch logging? asking because I don't see changes in this PR for these providers

@ashb

ashb commented Apr 1, 2025

Copy link
Copy Markdown
MemberAuthor

Does this affect elasticsearch / opensearch logging? asking because I don't see changes in this PR for these providers

Oh yes, I forgot to deal with this. My plan for now is to make it fall back to the existing airflow.task handler.

(I also really don't like we have two almost identical providers for ES and OS. Thanks a lot Elastic.

@ashb
ashbforce-pushed the overhaul-task-logging branch from b6cea26 to 1d0c3ebCompareApril 1, 2025 09:26
@ashb
Rework remote task log handling for the structlog era.
6191c13
Previously this feature was built on top of the stdlib logging.Handler
interface, and it worked but had a few issues (even before we switched to
structlog for Task SDK):
- we had to use what is in many ways a hack with the "set_context" to get
information down in to the task handler.
- Discovering of the configured task handler was somewhat baroque
- The whole thing is just complex due to the features of stdlib logging
(loggers, propagate, handler levels etc etc.)
- The upload was triggered somewhat "automatically" inside close, which from
an abstraction point of view is messy.
This changes things to have a more explicit interface purpose made for
uploading task log files and for reading them, and perhaps more crucially for
things like CloudWatch Logs, it (re)adds the ability to install a structlog
processor that will receive every log message as it happens.
The return types for the read et al functions were confusing the living
daylights out of me, so I've created type alias to give the return types
explicit names to reduce (my) confusion.
@ashb
ashbforce-pushed the overhaul-task-logging branch from 062fb4b to 6191c13CompareApril 2, 2025 12:44
@eladkaleladkal mentioned this pull request Apr 2, 2025
Closed
1 task
@eladkal

eladkal commented Apr 2, 2025

Copy link
Copy Markdown
Contributor

We are missing handling for HdfsTaskHandler but I believe this is a niche one and we can do it later. created #48685 to followup

@ashb

ashb commented Apr 2, 2025

Copy link
Copy Markdown
MemberAuthor

mypy providers failure is fixed in #48686

@ashb

ashb commented Apr 2, 2025

Copy link
Copy Markdown
MemberAuthor

I haven't changed deps, everything but lowest provider deps is passing, so I'm merging this now.

@ashb
ashb merged commit c1088b6 into apache:mainApr 2, 2025
@ashb
ashb deleted the overhaul-task-logging branch April 2, 2025 14:10
@ashbashb linked an issue Apr 2, 2025 that may be closed by this pull request
Task SDK cannot upload remote logs due to missing handlers #47634
Closed
2 tasks
@jason810496

jason810496 commented Apr 3, 2025

Copy link
Copy Markdown
Member

We are missing handling for HdfsTaskHandler but I believe this is a niche one and we can do it later. created #48685 to followup

I think ElasticsearchTaskHandler and RedisTaskHandler are missing in this PR. I will create separate issues to track them.

This was referenced Apr 3, 2025
Closed
Closed
@ashb

ashb commented Apr 3, 2025

Copy link
Copy Markdown
MemberAuthor

@jason810496 ES and OS are a bit of a mess tbh. They are entirely too specialized, and 95% of that isn't needed anymore now that Task SDK writes out JSON logs natively.

@eladkal

eladkal commented Apr 3, 2025

Copy link
Copy Markdown
Contributor

We can decide that these two providers (Elastic, open search) would be Airflow 3+ compatible from next release.
they get code changes very rarely anyway. If it simplify stuff we can do that.

aaron-y-chen pushed a commit to aaron-y-chen/airflow that referenced this pull request Apr 4, 2025
@ashb@aaron-y-chen
Rework remote task log handling for the structlog era. (apache#48491)
58afae5
Previously this feature was built on top of the stdlib logging.Handler
interface, and it worked but had a few issues (even before we switched to
structlog for Task SDK):
- we had to use what is in many ways a hack with the "set_context" to get
information down in to the task handler.
- Discovering of the configured task handler was somewhat baroque
- The whole thing is just complex due to the features of stdlib logging
(loggers, propagate, handler levels etc etc.)
- The upload was triggered somewhat "automatically" inside close, which from
an abstraction point of view is messy.
This changes things to have a more explicit interface purpose made for
uploading task log files and for reading them, and perhaps more crucially for
things like CloudWatch Logs, it (re)adds the ability to install a structlog
processor that will receive every log message as it happens.
The return types for the read et al functions were confusing the living
daylights out of me, so I've created type alias to give the return types
explicit names to reduce (my) confusion.
@eladkaleladkal mentioned this pull request Apr 6, 2025
Closed
diogotrodrigues pushed a commit to diogotrodrigues/airflow that referenced this pull request Apr 6, 2025
@ashb@diogotrodrigues
Rework remote task log handling for the structlog era. (apache#48491)
3415c40
Previously this feature was built on top of the stdlib logging.Handler
interface, and it worked but had a few issues (even before we switched to
structlog for Task SDK):
- we had to use what is in many ways a hack with the "set_context" to get
information down in to the task handler.
- Discovering of the configured task handler was somewhat baroque
- The whole thing is just complex due to the features of stdlib logging
(loggers, propagate, handler levels etc etc.)
- The upload was triggered somewhat "automatically" inside close, which from
an abstraction point of view is messy.
This changes things to have a more explicit interface purpose made for
uploading task log files and for reading them, and perhaps more crucially for
things like CloudWatch Logs, it (re)adds the ability to install a structlog
processor that will receive every log message as it happens.
The return types for the read et al functions were confusing the living
daylights out of me, so I've created type alias to give the return types
explicit names to reduce (my) confusion.
simonprydden pushed a commit to simonprydden/airflow that referenced this pull request Apr 8, 2025
@ashbSimon Prydden
Rework remote task log handling for the structlog era. (apache#48491)
8cdc638
Previously this feature was built on top of the stdlib logging.Handler
interface, and it worked but had a few issues (even before we switched to
structlog for Task SDK):
- we had to use what is in many ways a hack with the "set_context" to get
information down in to the task handler.
- Discovering of the configured task handler was somewhat baroque
- The whole thing is just complex due to the features of stdlib logging
(loggers, propagate, handler levels etc etc.)
- The upload was triggered somewhat "automatically" inside close, which from
an abstraction point of view is messy.
This changes things to have a more explicit interface purpose made for
uploading task log files and for reading them, and perhaps more crucially for
things like CloudWatch Logs, it (re)adds the ability to install a structlog
processor that will receive every log message as it happens.
The return types for the read et al functions were confusing the living
daylights out of me, so I've created type alias to give the return types
explicit names to reduce (my) confusion.
@jason810496jason810496 mentioned this pull request Apr 20, 2025
Merged
@jason810496jason810496 mentioned this pull request Apr 29, 2025
Closed
18 tasks
@eladkaleladkal mentioned this pull request Apr 29, 2025
Closed
2 tasks
@AutomationDev85AutomationDev85 mentioned this pull request Nov 27, 2025
Closed
2 tasks
YoannAbriel added a commit to YoannAbriel/airflow that referenced this pull request Mar 5, 2026
@YoannAbriel
fix: separate handler kwargs from RemoteLogIO kwargs in remote loggin…
36cc1c2
…g config
When remote_task_handler_kwargs contains FileTaskHandler parameters like
max_bytes, backup_count, or delay, these were incorrectly passed to
RemoteLogIO constructors (e.g., WasbRemoteLogIO, S3RemoteLogIO) which
don't accept them, causing TypeError on startup.
This was a regression introduced when RemoteLogIO classes were split out
from the handlers in PR apache#48491. The remote_task_handler_kwargs dict was
passed entirely to RemoteLogIO and then cleared, so handler-level params
were never applied to the FileTaskHandler.
The fix separates the kwargs: handler-specific parameters (max_bytes,
backup_count, delay) are preserved and applied to the handler config,
while the remaining kwargs are passed to the RemoteLogIO constructor.
Closes: apache#58770
@YoannAbrielYoannAbriel mentioned this pull request Mar 5, 2026
Closed
1 task
YoannAbriel added a commit to YoannAbriel/airflow that referenced this pull request Mar 5, 2026
@YoannAbriel
fix: separate handler kwargs from RemoteLogIO kwargs in remote loggin…
f3e077d
…g config
When remote_task_handler_kwargs contains FileTaskHandler parameters like
max_bytes, backup_count, or delay, these were incorrectly passed to
RemoteLogIO constructors (e.g., WasbRemoteLogIO, S3RemoteLogIO) which
don't accept them, causing TypeError on startup.
This was a regression introduced when RemoteLogIO classes were split out
from the handlers in PR apache#48491. The remote_task_handler_kwargs dict was
passed entirely to RemoteLogIO and then cleared, so handler-level params
were never applied to the FileTaskHandler.
The fix separates the kwargs: handler-specific parameters (max_bytes,
backup_count, delay) are preserved and applied to the handler config,
while the remaining kwargs are passed to the RemoteLogIO constructor.
Closes: apache#58770
@amoghrajeshamoghrajesh mentioned this pull request Apr 14, 2026
Closed
1 task
@haseebmalik18haseebmalik18 mentioned this pull request Apr 14, 2026
Merged
@potiukpotiuk mentioned this pull request Jun 3, 2026
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@o-nikolaso-nikolaso-nikolas left review comments
@kaxilkaxilkaxil approved these changes
@amoghrajeshamoghrajeshamoghrajesh approved these changes
@eladkaleladkaleladkal approved these changes
@jedcunninghamjedcunninghamjedcunningham approved these changes
@potiukpotiukAwaiting requested review from potiukpotiuk is a code owner
@gopidesupavangopidesupavanAwaiting requested review from gopidesupavangopidesupavan is a code owner
+1 more reviewer
@AdaverseAdaverseAdaverse left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area:loggingarea:providersarea:task-sdkprovider:amazonAWS/Amazon - related issuesprovider:googleGoogle (including GCP) related issuesprovider:microsoft-azureAzure-related issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Task SDK cannot upload remote logs due to missing handlers

8 participants

@ashb@eladkal@jason810496@kaxil@Adaverse@amoghrajesh@o-nikolas@jedcunningham