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 19:31:28 UTC
[GitHub] [airflow] josh-fell opened a new pull request, #24933: Apply flake8-logging-format changes to providers
josh-fell opened a new pull request, #24933:
URL: https://github.com/apache/airflow/pull/24933
Related: #23597 #24910
This is some pre-work for hopefully/maybe including the [flake8-logging-format](https://github.com/globality-corp/flake8-logging-format) extension in CI. There are roughly 88 file changes so splitting these up into smaller chunks.
After the initial sweeps are completed we can do a final pass when formally implementing the extension in CI.
I'm not entirely certain on if there are impacts to useful info that will be missed with log handlers but would love feedback on general patterns. Core PR to come later based on feedback.
[ ] Fix failing tests
---
**^ 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] uranusjr commented on a diff in pull request #24933: Apply flake8-logging-format changes to providers
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24933:
URL: https://github.com/apache/airflow/pull/24933#discussion_r935136127
##########
airflow/providers/amazon/aws/hooks/glue.py:
##########
@@ -107,8 +107,8 @@ def get_iam_execution_role(self) -> Dict:
glue_execution_role = iam_client.get_role(RoleName=self.role_name)
self.log.info("Iam Role Name: %s", self.role_name)
return glue_execution_role
- except Exception as general_error:
- self.log.error("Failed to create aws glue job, error: %s", general_error)
+ except Exception:
+ self.log.error("Failed to create aws glue job.")
Review Comment:
Do we want to use `exception` here? (same for many below)
--
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 a diff in pull request #24933: Apply flake8-logging-format changes to providers
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #24933:
URL: https://github.com/apache/airflow/pull/24933#discussion_r934605742
##########
tests/providers/amazon/aws/log/test_s3_task_handler.py:
##########
@@ -107,9 +106,8 @@ def test_hook_raises(self):
mock_error.assert_called_once_with(
'Could not create an S3Hook with connection id "%s". Please make '
- 'sure that apache-airflow[aws] is installed and the S3 connection exists. Exception : "%s"',
Review Comment:
This an example of where I'm unsure if accounting for `G200` error ("_Logging statements should not include the exception in logged string [use exception or exc_info=True]_") will negatively affect what's logged and, I suppose more importantly, what the log handlers will ingest.
Here the "Exception: <exception message>" is part of the logging line directly. But does this matter when `exc_info=True` or when there is a following `raise`? Is there really any lost information in the logs?
--
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 #24933: Apply flake8-logging-format changes to providers
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24933:
URL: https://github.com/apache/airflow/pull/24933#discussion_r935136127
##########
airflow/providers/amazon/aws/hooks/glue.py:
##########
@@ -107,8 +107,8 @@ def get_iam_execution_role(self) -> Dict:
glue_execution_role = iam_client.get_role(RoleName=self.role_name)
self.log.info("Iam Role Name: %s", self.role_name)
return glue_execution_role
- except Exception as general_error:
- self.log.error("Failed to create aws glue job, error: %s", general_error)
+ except Exception:
+ self.log.error("Failed to create aws glue job.")
Review Comment:
Do we want to use `exception` here? (and those in the same file)
--
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 #24933: Apply flake8-logging-format changes to providers
Posted by GitBox <gi...@apache.org>.
josh-fell commented on PR #24933:
URL: https://github.com/apache/airflow/pull/24933#issuecomment-1229643553
Not stale. Just need to find some time to tackle #24933 first. There are similar patterns and want to make sure there is consensus and consistency.
--
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 #24933: Apply flake8-logging-format changes to providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24933:
URL: https://github.com/apache/airflow/pull/24933#discussion_r935353075
##########
airflow/providers/amazon/aws/hooks/glue.py:
##########
@@ -107,8 +107,8 @@ def get_iam_execution_role(self) -> Dict:
glue_execution_role = iam_client.get_role(RoleName=self.role_name)
self.log.info("Iam Role Name: %s", self.role_name)
return glue_execution_role
- except Exception as general_error:
- self.log.error("Failed to create aws glue job, error: %s", general_error)
+ except Exception:
+ self.log.error("Failed to create aws glue job.")
Review Comment:
Yep. Sounds strange. We shoud catch specific exception that we know about and let all the rest buble up directly, there is no point in extra logging here unless we want to provide any "specific" information resulting in helping the user to react to some known exceptions. There is no point in logging meaningless log here - the user knows, Glue job failed to be created already and providing this extra line with no actual "Help" for the user and without instructions on what to do is borderline harrasing the user "Hello you already know we failed, so let us repeat it here").
--
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 a diff in pull request #24933: Apply flake8-logging-format changes to providers
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #24933:
URL: https://github.com/apache/airflow/pull/24933#discussion_r935599193
##########
airflow/providers/alibaba/cloud/hooks/oss.py:
##########
@@ -221,8 +221,8 @@ def delete_object(
try:
self.get_bucket(bucket_name).delete_object(key)
except Exception as e:
- self.log.error(e)
- raise AirflowException(f"Errors when deleting: {key}")
+ self.log.error("Errors when deleting %s", key)
+ raise AirflowException(e)
Review Comment:
Ah yes, this totally makes sense. Agreed. I'll do a clean sweep of all the provider files I'm touching for this as well.
--
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 a diff in pull request #24933: Apply flake8-logging-format changes to providers
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #24933:
URL: https://github.com/apache/airflow/pull/24933#discussion_r934609114
##########
tests/providers/google/cloud/log/test_gcs_task_handler.py:
##########
@@ -172,7 +172,7 @@ def test_failed_write_to_remote_on_close(self, mock_blob, mock_client, mock_cred
(
"airflow.providers.google.cloud.log.gcs_task_handler.GCSTaskHandler",
logging.ERROR,
- "Could not write logs to gs://bucket/remote/log/location/1.log: Failed to connect",
Review Comment:
Another example of a potential impact to missing log information that I'm unsure if it's _actually_ an issue.
--
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 a diff in pull request #24933: Apply flake8-logging-format changes to providers
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #24933:
URL: https://github.com/apache/airflow/pull/24933#discussion_r935622253
##########
airflow/providers/amazon/aws/hooks/glue.py:
##########
@@ -107,8 +107,8 @@ def get_iam_execution_role(self) -> Dict:
glue_execution_role = iam_client.get_role(RoleName=self.role_name)
self.log.info("Iam Role Name: %s", self.role_name)
return glue_execution_role
- except Exception as general_error:
- self.log.error("Failed to create aws glue job, error: %s", general_error)
+ except Exception:
+ self.log.error("Failed to create aws glue job.")
Review Comment:
>Do we want to use exception here? (same for many below)
Generally if there was a `raise` I stuck to using `error` so the traceback wasn't logged twice.
Oof yeah @potiuk there are a _lot_ of generic exceptions in these files. I'll clean them up.
--
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 #24933: Apply flake8-logging-format changes to providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24933:
URL: https://github.com/apache/airflow/pull/24933#discussion_r935353075
##########
airflow/providers/amazon/aws/hooks/glue.py:
##########
@@ -107,8 +107,8 @@ def get_iam_execution_role(self) -> Dict:
glue_execution_role = iam_client.get_role(RoleName=self.role_name)
self.log.info("Iam Role Name: %s", self.role_name)
return glue_execution_role
- except Exception as general_error:
- self.log.error("Failed to create aws glue job, error: %s", general_error)
+ except Exception:
+ self.log.error("Failed to create aws glue job.")
Review Comment:
Yep. Sounds strange. We shoud catch specific exception that we know about and let all the rest buble up directly, there is no point in extra logging here unless we want to provide any "specific" information resulting in helping the user to react to some known exceptions. There is no point in logging meaningless log here - the user knows, Glue job failed to be created already and providing this extra line with no actual "Help" for the user and instructions on what to do is borderline harrasing the user "Hello you already know we failed, so let us repeat it here").
--
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 a diff in pull request #24933: Apply flake8-logging-format changes to providers
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #24933:
URL: https://github.com/apache/airflow/pull/24933#discussion_r934606889
##########
tests/providers/amazon/aws/log/test_s3_task_handler.py:
##########
@@ -107,9 +106,8 @@ def test_hook_raises(self):
mock_error.assert_called_once_with(
'Could not create an S3Hook with connection id "%s". Please make '
- 'sure that apache-airflow[aws] is installed and the S3 connection exists. Exception : "%s"',
Review Comment:
An option could be ignoring the `G200` error as well as part of `flake8-logging-format` should we want to introduce it. @kaxil Do you have an opinion here since you'd taken a crack at using this flake8 plugin elsewhere?
--
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 #24933: Apply flake8-logging-format changes to providers
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #24933:
URL: https://github.com/apache/airflow/pull/24933#discussion_r935349386
##########
airflow/providers/alibaba/cloud/hooks/oss.py:
##########
@@ -221,8 +221,8 @@ def delete_object(
try:
self.get_bucket(bucket_name).delete_object(key)
except Exception as e:
- self.log.error(e)
- raise AirflowException(f"Errors when deleting: {key}")
+ self.log.error("Errors when deleting %s", key)
+ raise AirflowException(e)
Review Comment:
Agree :). We used to use that pattern in the past but unless you use dedicated AirflowSkip or AirflowFailException it's better to raise the original exception
--
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 #24933: Apply flake8-logging-format changes to providers
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #24933:
URL: https://github.com/apache/airflow/pull/24933#discussion_r935135276
##########
airflow/providers/alibaba/cloud/hooks/oss.py:
##########
@@ -221,8 +221,8 @@ def delete_object(
try:
self.get_bucket(bucket_name).delete_object(key)
except Exception as e:
- self.log.error(e)
- raise AirflowException(f"Errors when deleting: {key}")
+ self.log.error("Errors when deleting %s", key)
+ raise AirflowException(e)
Review Comment:
Since this is a provider, I wonder if we should just switch to re-raise the original exception. Coercing the error to AirflowException offers no benefits at all
--
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 a diff in pull request #24933: Apply flake8-logging-format changes to providers
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #24933:
URL: https://github.com/apache/airflow/pull/24933#discussion_r934605742
##########
tests/providers/amazon/aws/log/test_s3_task_handler.py:
##########
@@ -107,9 +106,8 @@ def test_hook_raises(self):
mock_error.assert_called_once_with(
'Could not create an S3Hook with connection id "%s". Please make '
- 'sure that apache-airflow[aws] is installed and the S3 connection exists. Exception : "%s"',
Review Comment:
This an example of where I'm unsure if accounting for `G200` error ("_Logging statements should not include the exception in logged string [use exception or exc_info=True]_") will negatively affect what's logged and, I suppose more importantly, what the log handlers will ingest.
Here the "Exception: <exception message>" is part of the logging line directly but disallowed as part of the flake8 plugin. But does this matter when `exc_info=True` or when there is a following `raise`? Is there really any lost information in the logs? Maybe there is a slick workaround that I'm missing?
--
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 a diff in pull request #24933: Apply flake8-logging-format changes to providers
Posted by GitBox <gi...@apache.org>.
josh-fell commented on code in PR #24933:
URL: https://github.com/apache/airflow/pull/24933#discussion_r934605742
##########
tests/providers/amazon/aws/log/test_s3_task_handler.py:
##########
@@ -107,9 +106,8 @@ def test_hook_raises(self):
mock_error.assert_called_once_with(
'Could not create an S3Hook with connection id "%s". Please make '
- 'sure that apache-airflow[aws] is installed and the S3 connection exists. Exception : "%s"',
Review Comment:
This an example of where I'm unsure if accounting for `G200` error (Logging statements should not include the exception in logged string (use exception or exc_info=True) will negatively affect what's logged and, I suppose more importantly, what the log handlers will ingest.
Here the "Exception: <exception message>" is part of the logging line directly. But does this matter when `exc_info=True` or when there is a following `raise`? Is there really any lost information in the logs?
--
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] github-actions[bot] closed pull request #24933: Apply flake8-logging-format changes to providers
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #24933: Apply flake8-logging-format changes to providers
URL: https://github.com/apache/airflow/pull/24933
--
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] github-actions[bot] commented on pull request #24933: Apply flake8-logging-format changes to providers
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #24933:
URL: https://github.com/apache/airflow/pull/24933#issuecomment-1278312815
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
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