-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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.
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
Fix broken task_log_prefix_template by adding it to FileTaskHandler itself. #38709
Conversation
CC @pankajkoti |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. @abhishekbhakat are we able to test this with well including remote loggers like S3, GCP, Azure, Elasticsearch?
I've verified this with S3 yet. Will check on others today. |
Thanks @abhishekbhakat . GCP and Azure would be similar to test like S3. For Elasticsearch, I have detailed one of the ways to test it here #32438 (comment) |
I'm done verifying it on AWS, Azure and GCP. Will confirm for ES in some time. |
So the code changes I made does not accommodate Elasticsearch's write_stdout. It was not working earlier, and so the behavior remains the same. |
To make it working with |
Also, I'm not sure if prefix is even needed on Elasticsearch. If we use ElasticsearchJSONFormatter it already includes fields for dag_id, task_id, etc. Shall we ignore ES for the scope of this PR then ? |
Moving discussions to Issue #39019. |
Instead of adding prefix support to all our of logging contexts, I think we should just keep it scoped to |
I would say to strip it out completely. That |
Sorry, didn't mean to split the conversation, but left some more thoughts on #39019. I've removed this from 2.9.1 since we don't know how we will proceed yet. |
@abhishekbhakat I think you would be able to use |
I tried, but couldn't get it working without just using an entirely different Handler. |
Closing this PR in favor of deprecating this and more discussions at #39019. |
As of now the config
task_log_prefix_template
does not work. Although the below code is specifically written for this purpose, the class is extending a StreamHandler seems obsolete:airflow/airflow/utils/log/task_handler_with_custom_formatter.py
Line 36 in 649d643
I recommend removing this StreamHandler. This PR implements the prefix to FileTaskHandler itself. Have also written tests for the same.
The only use-case to discuss will be elasticsearch's
write_stdout
.Please recommend if we are good to drop the
TaskHandlerWithCustomFormatter
.^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rst
or{issue_number}.significant.rst
, in newsfragments.