Skip to content
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

Jump to bottom

Avoid non-recommended usage of logging #37792

Merged
merged 1 commit into from Feb 29, 2024
Merged

Avoid non-recommended usage of logging #37792

merged 1 commit into from Feb 29, 2024

Conversation

Taragolis
Copy link
Contributor

Enable two ruff checks for the logging and usages:

Example of rule violations before manual + auto fixes

Run 'ruff' for extremely fast Python linting.............................Failed
- hook id: ruff
- exit code: 1
- files were modified by this hook

airflow/jobs/backfill_job_runner.py:684:42: G201 Logging `.exception(...)` should be used instead of `.error(..., exc_info=True)`
airflow/jobs/backfill_job_runner.py:989:22: G201 Logging `.exception(...)` should be used instead of `.error(..., exc_info=True)`
airflow/models/variable.py:246:29: G001 Logging statement uses `str.format`
airflow/providers/amazon/aws/operators/sagemaker.py:352:64: G202 Logging statement has redundant `exc_info`
airflow/providers/amazon/aws/operators/sagemaker.py:358:66: G202 Logging statement has redundant `exc_info`
airflow/providers/amazon/aws/operators/sagemaker.py:780:22: G201 Logging `.exception(...)` should be used instead of `.error(..., exc_info=True)`
airflow/providers/amazon/aws/operators/sagemaker.py:788:22: G201 Logging `.exception(...)` should be used instead of `.error(..., exc_info=True)`
airflow/providers/amazon/aws/operators/sagemaker.py:816:66: G202 Logging statement has redundant `exc_info`
airflow/providers/cncf/kubernetes/executors/kubernetes_executor.py:434:30: G201 Logging `.exception(...)` should be used instead of `.error(..., exc_info=True)`
airflow/providers/google/cloud/operators/dataproc.py:838:26: G201 Logging `.exception(...)` should be used instead of `.error(..., exc_info=True)`
airflow/utils/db_cleanup.py:52:28: LOG002 Use `__name__` with `logging.getLogger()`
Found 13 errors (2 fixed, 11 remaining).
No fixes available (1 hidden fix can be enabled with the `--unsafe-fixes` option).

^ 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.

@boring-cyborg boring-cyborg bot added area:logging area:providers area:Scheduler Scheduler or dag parsing Issues provider:amazon-aws AWS/Amazon - related issues provider:cncf-kubernetes Kubernetes provider related issues provider:google Google (including GCP) related issues labels Feb 29, 2024
@Taragolis
Copy link
Contributor Author

I've also thought about ban implicit usage of root logger because I thought it made by a mistake in most case if not at all.

E.g. by add this rules into the banned api

# Logging root logger
"logging.debug".msg = "Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger"
"logging.info".msg = "Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger"
"logging.warning".msg = "Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger"
"logging.error".msg = "Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger"
"logging.exception".msg = "Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger"
"logging.fatal".msg = "Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger"
"logging.critical".msg = "Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger"
"logging.log".msg = "Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger"

❯ pre-commit run ruff --all-files
Run 'ruff' for extremely fast Python linting.............................Failed
- hook id: ruff
- exit code: 1

airflow/example_dags/example_python_decorator.py:60:9: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/example_dags/example_python_operator.py:63:9: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/metrics/otel_logger.py:308:9: TID251 `logging.debug` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/metrics/otel_logger.py:403:5: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/providers/amazon/aws/auth_manager/views/auth.py:84:13: TID251 `logging.error` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/providers/amazon/aws/auth_manager/views/auth.py:85:13: TID251 `logging.error` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/providers/amazon/aws/auth_manager/views/auth.py:86:13: TID251 `logging.error` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/sensors/base.py:260:21: TID251 `logging.error` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/settings.py:469:5: TID251 `logging.debug` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/utils/cli_action_loggers.py:41:5: TID251 `logging.debug` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/utils/cli_action_loggers.py:55:5: TID251 `logging.debug` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/utils/cli_action_loggers.py:67:5: TID251 `logging.debug` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/utils/cli_action_loggers.py:72:13: TID251 `logging.exception` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/utils/cli_action_loggers.py:85:5: TID251 `logging.debug` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/utils/cli_action_loggers.py:90:13: TID251 `logging.exception` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/utils/cli_action_loggers.py:134:13: TID251 `logging.warning` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/utils/cli_action_loggers.py:136:9: TID251 `logging.warning` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/utils/log/file_processor_handler.py:127:21: TID251 `logging.warning` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/utils/log/file_processor_handler.py:133:17: TID251 `logging.warning` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/utils/log/file_task_handler.py:541:17: TID251 `logging.warning` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/utils/log/task_handler_with_custom_formatter.py:61:9: TID251 `logging.warning` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/www/extensions/init_jinja_globals.py:54:9: TID251 `logging.error` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/www/views.py:1361:9: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/www/views.py:1480:13: TID251 `logging.warning` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/www/views.py:1487:9: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/www/views.py:4009:13: TID251 `logging.warning` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/www/views.py:4510:13: TID251 `logging.warning` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/www/views.py:4897:17: TID251 `logging.error` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/www/views.py:4904:21: TID251 `logging.warning` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/www/views.py:4910:21: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/www/views.py:5844:13: TID251 `logging.error` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/dag_processing/test_job_runner.py:905:17: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/dag_processing/test_job_runner.py:914:17: TID251 `logging.debug` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/dag_processing/test_job_runner.py:946:13: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/dag_processing/test_job_runner.py:949:13: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/dag_processing/test_job_runner.py:950:13: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/dag_processing/test_job_runner.py:952:13: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/dags/test_task_view_type_check.py:50:1: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
Found 38 errors.
tests/providers/apache/hive/transfers/test_s3_to_hive.py:210:13: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/system/providers/amazon/aws/example_sagemaker.py:165:9: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/system/providers/amazon/aws/example_sagemaker.py:457:9: TID251 `logging.error` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/system/providers/github/example_github.py:84:39: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/system/providers/github/example_github.py:95:39: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/system/utils/__init__.py:36:13: TID251 `logging.warning` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/system/utils/__init__.py:51:9: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/task/task_runner/test_standard_task_runner.py:381:9: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/task/task_runner/test_standard_task_runner.py:385:9: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/task/task_runner/test_standard_task_runner.py:387:9: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/task/task_runner/test_standard_task_runner.py:390:9: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/task/task_runner/test_standard_task_runner.py:394:9: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
Found 12 errors.

Any thoughts? I do not want to include this change into this PR, just want to implement and fix it as follow up

@vincbeck
Copy link
Contributor

I've also thought about ban implicit usage of root logger because I thought it made by a mistake in most case if not at all.

E.g. by add this rules into the banned api

# Logging root logger
"logging.debug".msg = "Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger"
"logging.info".msg = "Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger"
"logging.warning".msg = "Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger"
"logging.error".msg = "Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger"
"logging.exception".msg = "Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger"
"logging.fatal".msg = "Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger"
"logging.critical".msg = "Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger"
"logging.log".msg = "Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger"

❯ pre-commit run ruff --all-files
Run 'ruff' for extremely fast Python linting.............................Failed
- hook id: ruff
- exit code: 1

airflow/example_dags/example_python_decorator.py:60:9: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/example_dags/example_python_operator.py:63:9: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/metrics/otel_logger.py:308:9: TID251 `logging.debug` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/metrics/otel_logger.py:403:5: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/providers/amazon/aws/auth_manager/views/auth.py:84:13: TID251 `logging.error` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/providers/amazon/aws/auth_manager/views/auth.py:85:13: TID251 `logging.error` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/providers/amazon/aws/auth_manager/views/auth.py:86:13: TID251 `logging.error` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/sensors/base.py:260:21: TID251 `logging.error` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/settings.py:469:5: TID251 `logging.debug` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/utils/cli_action_loggers.py:41:5: TID251 `logging.debug` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/utils/cli_action_loggers.py:55:5: TID251 `logging.debug` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/utils/cli_action_loggers.py:67:5: TID251 `logging.debug` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/utils/cli_action_loggers.py:72:13: TID251 `logging.exception` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/utils/cli_action_loggers.py:85:5: TID251 `logging.debug` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/utils/cli_action_loggers.py:90:13: TID251 `logging.exception` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/utils/cli_action_loggers.py:134:13: TID251 `logging.warning` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/utils/cli_action_loggers.py:136:9: TID251 `logging.warning` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/utils/log/file_processor_handler.py:127:21: TID251 `logging.warning` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/utils/log/file_processor_handler.py:133:17: TID251 `logging.warning` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/utils/log/file_task_handler.py:541:17: TID251 `logging.warning` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/utils/log/task_handler_with_custom_formatter.py:61:9: TID251 `logging.warning` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/www/extensions/init_jinja_globals.py:54:9: TID251 `logging.error` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/www/views.py:1361:9: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/www/views.py:1480:13: TID251 `logging.warning` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/www/views.py:1487:9: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/www/views.py:4009:13: TID251 `logging.warning` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/www/views.py:4510:13: TID251 `logging.warning` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/www/views.py:4897:17: TID251 `logging.error` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/www/views.py:4904:21: TID251 `logging.warning` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/www/views.py:4910:21: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
airflow/www/views.py:5844:13: TID251 `logging.error` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/dag_processing/test_job_runner.py:905:17: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/dag_processing/test_job_runner.py:914:17: TID251 `logging.debug` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/dag_processing/test_job_runner.py:946:13: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/dag_processing/test_job_runner.py:949:13: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/dag_processing/test_job_runner.py:950:13: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/dag_processing/test_job_runner.py:952:13: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/dags/test_task_view_type_check.py:50:1: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
Found 38 errors.
tests/providers/apache/hive/transfers/test_s3_to_hive.py:210:13: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/system/providers/amazon/aws/example_sagemaker.py:165:9: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/system/providers/amazon/aws/example_sagemaker.py:457:9: TID251 `logging.error` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/system/providers/github/example_github.py:84:39: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/system/providers/github/example_github.py:95:39: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/system/utils/__init__.py:36:13: TID251 `logging.warning` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/system/utils/__init__.py:51:9: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/task/task_runner/test_standard_task_runner.py:381:9: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/task/task_runner/test_standard_task_runner.py:385:9: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/task/task_runner/test_standard_task_runner.py:387:9: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/task/task_runner/test_standard_task_runner.py:390:9: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
tests/task/task_runner/test_standard_task_runner.py:394:9: TID251 `logging.info` is banned: Instantiate new `logger = logging.getLogger(__name__)` and use it instead of root logger
Found 12 errors.

Any thoughts? I do not want to include this change into this PR, just want to implement and fix it as follow up

I'd be inclined to add this rule too. I really think it is used by mistake

@hussein-awala hussein-awala merged commit 77341ef into apache:main Feb 29, 2024
64 checks passed
@Taragolis Taragolis deleted the ruff-logging-rules branch February 29, 2024 20:22
@eladkal eladkal mentioned this pull request Mar 4, 2024
Closed
79 tasks
abhishekbhakat pushed a commit to abhishekbhakat/my_airflow that referenced this pull request Mar 5, 2024
@Taragolis @abhishekbhakat
Avoid non-recommended usage of logging (apache#37792)
dfddef9
@utkarsharma2 utkarsharma2 added the type:improvement Changelog: Improvements label Mar 6, 2024
utkarsharma2 pushed a commit to astronomer/airflow that referenced this pull request Apr 22, 2024
@Taragolis @utkarsharma2
Avoid non-recommended usage of logging (apache#37792)
23b6749
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hussein-awala hussein-awala hussein-awala approved these changes

@eladkal eladkal eladkal approved these changes

@vincbeck vincbeck vincbeck approved these changes

@o-nikolas o-nikolas Awaiting requested review from o-nikolas o-nikolas is a code owner

@jedcunningham jedcunningham Awaiting requested review from jedcunningham jedcunningham is a code owner

@ephraimbuddy ephraimbuddy Awaiting requested review from ephraimbuddy ephraimbuddy is a code owner

@kaxil kaxil Awaiting requested review from kaxil kaxil is a code owner

@XD-DENG XD-DENG Awaiting requested review from XD-DENG XD-DENG is a code owner

@ashb ashb Awaiting requested review from ashb ashb is a code owner

Assignees
No one assigned
Labels
area:logging area:providers area:Scheduler Scheduler or dag parsing Issues provider:amazon-aws AWS/Amazon - related issues provider:cncf-kubernetes Kubernetes provider related issues provider:google Google (including GCP) related issues type:improvement Changelog: Improvements
Projects
None yet
Milestone
Airflow 2.9.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Taragolis @vincbeck @hussein-awala @eladkal @utkarsharma2