Apply flake8-logging-format changes to providers #24933
Conversation
boring-cyborg
bot
added
provider:cncf-kubernetes
josh-fell
force-pushed
the
flake8-logging-format-providers
branch
from
2e33f19
to
4c80aab
Compare
| @@ -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"', | |||
There was a problem hiding this 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: " 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?
There was a problem hiding this 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?
| @@ -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", | |||
There was a problem hiding this comment.
Another example of a potential impact to missing log information that I'm unsure if it's actually an issue.
josh-fell
force-pushed
the
flake8-logging-format-providers
branch
from
4c80aab
to
b141bb3
Compare
| self.log.error(e) | ||
| raise AirflowException(f"Errors when deleting: {key}") | ||
| self.log.error("Errors when deleting %s", key) | ||
| raise AirflowException(e) |
There was a problem hiding this 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
There was a problem hiding this 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
There was a problem hiding this 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.
| 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.") |
There was a problem hiding this comment.
Do we want to use exception here? (same for many below)
There was a problem hiding this 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").
There was a problem hiding this 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.
|
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 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. |
github-actions
bot
added
the
stale
Related: #23597 #24910
This is some pre-work for hopefully/maybe including the 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.^ 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.rstor{issue_number}.significant.rst, in newsfragments.