Skip to content

Override base log folder by using task handler's base_log_folder#32781

Merged
eladkal merged 6 commits into
apache:mainfrom
Owen-CH-Leung:Use-task-handler's-base_log_folder-to-serve-logs
Jul 26, 2023
Merged

Override base log folder by using task handler's base_log_folder#32781
eladkal merged 6 commits into
apache:mainfrom
Owen-CH-Leung:Use-task-handler's-base_log_folder-to-serve-logs

Conversation

@Owen-CH-Leung

@Owen-CH-Leung Owen-CH-Leung commented Jul 23, 2023

Copy link
Copy Markdown
Contributor

Fixes #23967

Original code use log_directory defined in airflow.cfg even though another base_log_folder is defined in logging config.

This PR adds logic to attempt to override log_directory if logging_config_class and base_log_folder are both defined, and fall back to original default log directory if they don't

How to test if it works:

  1. execute breeze start-airflow --executor CeleryExecutor --backend postgres --load-example-dags --db-reset
  2. go to $AIRFLOW_HOME, create a directory called config, and create an empty __init__.py and log_config.py file inside
  3. Inside airflow/config_templates/airflow_local_settings.py, modify DEFAULT_LOGGING_CONFIG[handlers][task][base_log_folder] to some other different locations
  4. copy all the contents inside airflow/config_templates/airflow_local_settings.py into log_config.py
  5. In /root/airflow/airflow.cfg , under logging section, modify logging_config_class to log_config.DEFAULT_LOGGING_CONFIG
  6. Restart everything
  7. Run a DAG in webUI, check the log, and you should be about to see the logs are served.
  8. Go to the location that you defined in step 3, and you should see the dag log

Adaverse
Adaverse reviewed Jul 23, 2023
View reviewed changes
Comment thread airflow/utils/serve_logs.py Outdated
Comment on lines +53 to +55
module_name, dict_name = log_config_class.split(".")
module = importlib.import_module(module_name)
logging_config = getattr(module, dict_name)

@Adaverse Adaverse Jul 23, 2023

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.

Below can be used instead?

from airflow.utils.module_loading import import_string
logging_config = import_string(log_config_class)

@Adaverse

Adaverse commented Jul 23, 2023

Copy link
Copy Markdown
Contributor

Can we make the above change in the configure_logging() from airflow/logging_config.py and call it here. Since it is loading the custom logging config there.

@Owen-CH-Leung

Owen-CH-Leung commented Jul 23, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@Adaverse Thanks for your feedback.

Yes I agree that we can re-use import_string and configure_logging as suggested, but I'm a bit hesitant to modify the configure_logging() to return the overriden base_log_dir since I saw it's being used in a few other places also. (e.g. airflow/www/app.py, airflow/settings.py , airflow/cli/commands/internal_api_command.py and so on)

How about I just reuse these 2 functions and continue my override logic here in serve_logs.py ? WDYT ?

@Owen-CH-Leung
Owen-CH-Leung requested a review from Adaverse July 23, 2023 15:56
uranusjr
uranusjr requested changes Jul 24, 2023
View reviewed changes
Comment thread airflow/utils/serve_logs.py Outdated
Comment thread airflow/utils/serve_logs.py Outdated
@Adaverse

Adaverse commented Jul 24, 2023

Copy link
Copy Markdown
Contributor

@Adaverse Thanks for your feedback.

Yes I agree that we can re-use import_string and configure_logging as suggested, but I'm a bit hesitant to modify the configure_logging() to return the overriden base_log_dir since I saw it's being used in a few other places also. (e.g. airflow/www/app.py, airflow/settings.py , airflow/cli/commands/internal_api_command.py and so on)

How about I just reuse these 2 functions and continue my override logic here in serve_logs.py ? WDYT ?

Sure looks good!
Also it would be good to have these changes covered by tests.

uranusjr
uranusjr reviewed Jul 24, 2023
View reviewed changes
Comment thread airflow/utils/serve_logs.py Outdated
@eladkal eladkal added this to the Airflow 2.6.4 milestone Jul 24, 2023
@eladkal eladkal added the type:bug-fix Changelog: Bug Fixes label Jul 24, 2023
@Owen-CH-Leung

Owen-CH-Leung commented Jul 24, 2023

Copy link
Copy Markdown
Contributor Author

@Adaverse Thanks for your feedback.
Yes I agree that we can re-use import_string and configure_logging as suggested, but I'm a bit hesitant to modify the configure_logging() to return the overriden base_log_dir since I saw it's being used in a few other places also. (e.g. airflow/www/app.py, airflow/settings.py , airflow/cli/commands/internal_api_command.py and so on)
How about I just reuse these 2 functions and continue my override logic here in serve_logs.py ? WDYT ?

Sure looks good! Also it would be good to have these changes covered by tests.

Sorry after inspecting the code a bit I'll have to remove the use of configure_logging. The reason being this method will by default load the DEFAULT_LOGGING_CONFIG defined in airflow_local_settings.py , and when it's being called here, it will always override the setting at base_log_dir under logging section. (And hence some of the tests are broken). I've fall back to use conf.get("logging", "logging_config_class") but keep import_string() for now.

Meanwhile, I'm adding some tests to cover the changes I made.

@Owen-CH-Leung
Owen-CH-Leung requested a review from uranusjr July 24, 2023 08:35
uranusjr
uranusjr reviewed Jul 24, 2023
View reviewed changes
Comment thread tests/utils/test_serve_logs.py Outdated
@Owen-CH-Leung
Owen-CH-Leung requested a review from uranusjr July 24, 2023 12:31
@eladkal
eladkal merged commit 579ce06 into apache:main Jul 26, 2023
@notacoat

notacoat commented Jul 26, 2023

Copy link
Copy Markdown

Sort of confused by this, as I created the original issue based on issues I was having at work. When we upgraded to airflow 2, we didn’t see this problem anymore. AFAIK, we’re still using the same logging config from airflow 1.

@eladkal eladkal modified the milestones: Airflow 2.6.4, Airflow 2.7.0 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@uranusjr uranusjr uranusjr approved these changes
@eladkal eladkal eladkal approved these changes
@Adaverse Adaverse Awaiting requested review from Adaverse

Assignees

No one assigned

Labels

type:bug-fix Changelog: Bug Fixes

Projects

None yet

Milestone

Airflow 2.7.0

Development

Successfully merging this pull request may close these issues.

serve_logs.py should respect the logging config's task handler's base_log_folder value

5 participants

@Owen-CH-Leung @Adaverse @notacoat @uranusjr @eladkal