You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/07/08 01:42:35 UTC
[GitHub] [airflow] potiuk opened a new pull request, #24910: Automatically detect if non-lazy logging interpolation is used
potiuk opened a new pull request, #24910:
URL: https://github.com/apache/airflow/pull/24910
We used to have pylint check that was preventing it but since
we have no pylint, we need to do some intelligent guessing
based on AST of the python code.
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
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](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a diff in pull request #24910: Automatically detect if non-lazy logging interpolation is used
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24910:
URL: https://github.com/apache/airflow/pull/24910#discussion_r916592844
##########
scripts/ci/pre_commit/pre_commit_check_lazy_logging.py:
##########
@@ -0,0 +1,79 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+import ast
+import pathlib
+import re
+import sys
+
+import astor as astor
+from rich.console import Console
+
+console = Console(color_system="standard", width=200)
+
+LOGGIN_MATCHER = re.compile(r'^log.?[a-z]*\.[a-z]*\(f.*["\']')
+SELF_LOG_MATCHER = re.compile(r'^self\.log\.[a-z]*\(f.*["\']')
+
+
+class LogFinder(astor.TreeWalk):
+ module_printed: bool = False
+ name: str = ''
+ error_count = 0
+
+ def pre_Call(self):
+ if isinstance(self.cur_node.func, ast.Attribute) and (
+ isinstance(self.cur_node.func.value, ast.Name)
+ and (
+ self.cur_node.func.value.id == "logger"
+ or self.cur_node.func.value.id == "logging"
+ or self.cur_node.func.value.id == "log"
+ )
+ or (self.cur_node.func.attr in ['debug', 'warning', 'info', "error", "critical"])
Review Comment:
Right.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #24910: Automatically detect if non-lazy logging interpolation is used
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24910:
URL: https://github.com/apache/airflow/pull/24910#issuecomment-1178445395
Maybe not perfect, but I think it should catch vast majority of cases when f-strings are used in logging.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] josh-fell commented on pull request #24910: Automatically detect if non-lazy logging interpolation is used
Posted by GitBox <gi...@apache.org>.
josh-fell commented on PR #24910:
URL: https://github.com/apache/airflow/pull/24910#issuecomment-1178948296
Ah great minds! I was actually working on implementing the [flake8-logging-format](https://github.com/globality-corp/flake8-logging-format) extension based on [Kaxil's comment in #23597](https://github.com/apache/airflow/pull/23597#discussion_r872213321). It's definitely a more intense check beyond f-strings, but I'm happy to stop working on this if the check here satisfies needs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #24910: Automatically detect if non-lazy logging interpolation is used
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24910:
URL: https://github.com/apache/airflow/pull/24910#issuecomment-1179019035
Ahh... :). Maybe the flake8 might be better. My solution is really a quick&dirty hack which detects likely vast majority of the uses of logging but not all of them (and it's a bit custom - making assumption on our use of our conventions (for example that our logger is set as "log" attribute). But more "complete" and fool-proof reusable solution as flake8 extension is definitely preferrable, so if you want to do it, publish and share with others, I am more than happy to replace my "dirty script" with it ).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #24910: Automatically detect if non-lazy logging interpolation is used
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24910:
URL: https://github.com/apache/airflow/pull/24910#discussion_r916453270
##########
STATIC_CODE_CHECKS.rst:
##########
@@ -175,6 +175,8 @@ require Breeze Docker image to be build locally.
+--------------------------------------------------------+------------------------------------------------------------------+---------+
| check-integrations-are-consistent | Check if integration list is consistent in various places | |
+--------------------------------------------------------+------------------------------------------------------------------+---------+
+| check-lazy-logging | Check that all logging is lazy | |
Review Comment:
```suggestion
| check-lazy-logging | Check that all logging are lazy | |
```
?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #24910: Automatically detect if non-lazy logging interpolation is used
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #24910:
URL: https://github.com/apache/airflow/pull/24910
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] josh-fell commented on pull request #24910: Automatically detect if non-lazy logging interpolation is used
Posted by GitBox <gi...@apache.org>.
josh-fell commented on PR #24910:
URL: https://github.com/apache/airflow/pull/24910#issuecomment-1179304642
> Ahh... :). Maybe the flake8 might be better. My solution is really a quick&dirty hack which detects likely vast majority of the uses of logging but not all of them (and it's a bit custom - making assumption on our use of our conventions (for example that our logger is set as "log" attribute). But more "complete" and fool-proof reusable solution as flake8 extension is definitely preferrable, so if you want to do it, publish and share with others, I am more than happy to replace my "dirty script" with it ).
Tossed #24931 and #24933 out there for flake8-logging-format. Would love feedback if folks are willing 🙂
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #24910: Automatically detect if non-lazy logging interpolation is used
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24910:
URL: https://github.com/apache/airflow/pull/24910#discussion_r916453981
##########
scripts/ci/pre_commit/pre_commit_check_lazy_logging.py:
##########
@@ -0,0 +1,79 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import annotations
+
+import ast
+import pathlib
+import re
+import sys
+
+import astor as astor
+from rich.console import Console
+
+console = Console(color_system="standard", width=200)
+
+LOGGIN_MATCHER = re.compile(r'^log.?[a-z]*\.[a-z]*\(f.*["\']')
+SELF_LOG_MATCHER = re.compile(r'^self\.log\.[a-z]*\(f.*["\']')
+
+
+class LogFinder(astor.TreeWalk):
+ module_printed: bool = False
+ name: str = ''
+ error_count = 0
+
+ def pre_Call(self):
+ if isinstance(self.cur_node.func, ast.Attribute) and (
+ isinstance(self.cur_node.func.value, ast.Name)
+ and (
+ self.cur_node.func.value.id == "logger"
+ or self.cur_node.func.value.id == "logging"
+ or self.cur_node.func.value.id == "log"
+ )
+ or (self.cur_node.func.attr in ['debug', 'warning', 'info', "error", "critical"])
Review Comment:
There’s also `Logger.log`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #24910: Automatically detect if non-lazy logging interpolation is used
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24910:
URL: https://github.com/apache/airflow/pull/24910#issuecomment-1178443228
Finally I got to do it:
<img width="920" alt="Screenshot 2022-07-08 at 03 39 27" src="https://user-images.githubusercontent.com/595491/177899689-23000587-69d5-437e-83f3-7e78ba249bfb.png">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org