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 2021/03/31 20:34:19 UTC
[GitHub] [airflow] dstandish opened a new pull request #15120: Do not log info when ssm secret not found
dstandish opened a new pull request #15120:
URL: https://github.com/apache/airflow/pull/15120
Not finding a connection in a particular backend is not and event that merits logging. That's why we have 3 connections in the search path.
It's only when the connection is not found in all 3 backends is something exceptional happening.
Log level should be DEBUG.
Also, use of the word 'error' is misleading, because not finding a parameter is normal.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on pull request #15120: Do not log info when ssm secret not found
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on pull request #15120:
URL: https://github.com/apache/airflow/pull/15120#issuecomment-812394314
By logging error, it can be helpful if the user expects to use a specific backend for the secret or fails for unexpected reasons (PermissionDenied). https://github.com/apache/airflow/blob/4d1b2e985492894e6064235688cf8f381b1e8858/airflow/providers/google/cloud/_internal_client/secret_manager_client.py#L92-L98
But regardless of the final decision of using error or debug, do you mind also fix the `aws/secrets/secrets_manager.py` ? It's using `log.debug` right now, so better to make it consistent
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #15120: Do not log info when ssm secret not found
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #15120:
URL: https://github.com/apache/airflow/pull/15120#issuecomment-812583657
> But regardless of the final decision of using error or debug, do you mind also fix the `aws/secrets/secrets_manager.py` ? It's using `log.debug` right now, so better to make it consistent
done
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish edited a comment on pull request #15120: Do not log info when ssm secret not found
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #15120:
URL: https://github.com/apache/airflow/pull/15120#issuecomment-812579636
> By logging error, it can be helpful if the user expects to use a specific backend for the secret or fails for unexpected reasons (PermissionDenied).
Yes in theory it can be helpful. BUT it can also be misleading! It's like when your airflow cluster goes down and you try and get it back up before your boss notices. You _could_ tell them but is it really helpful to do so 😉?
At my company when I implemented ssm secrets backend a user saw this error and coincidentally happened to be having other problems with a dag he was working on. Not finding the cred was exactly what should have happened because the setup is supposed to use the aws instance profile not airflow connection. But they saw this "error" in the logs and it made it harder for them to find what was the actual issue because they were distracted by this "error". And they had a tough time understanding that this was fully expected and unrelated to their actual issue.
So, while those of us who understand what's happening under the hood can make sense of this "error", users may not.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #15120: Do not log info when ssm secret not found
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15120:
URL: https://github.com/apache/airflow/pull/15120#issuecomment-812733161
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish edited a comment on pull request #15120: Do not log info when ssm secret not found
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #15120:
URL: https://github.com/apache/airflow/pull/15120#issuecomment-812579636
> By logging error, it can be helpful if the user expects to use a specific backend for the secret or fails for unexpected reasons (PermissionDenied).
Yes in theory it can be helpful. BUT it can also be misleading! It's like when your airflow cluster goes down and you try and get it back up before your boss notices. You _could_ tell them but is it really helpful to do so 😉?
At my company when I implemented ssm secrets backend a user saw this error and coincidentally happened to be having other problems with a dag he was working on. Not finding the cred was exactly what should have happened because the setup is supposed to use the aws instance profile not airflow connection. So it's saying "error" and he never really believed that this was fully expected and unrelated to his issue. 🙂
So, while those of us who understand what's happening under the hood can make sense of the error, users may not.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #15120: Do not log info when ssm secret not found
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #15120:
URL: https://github.com/apache/airflow/pull/15120
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish edited a comment on pull request #15120: Do not log info when ssm secret not found
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #15120:
URL: https://github.com/apache/airflow/pull/15120#issuecomment-812579636
> By logging error, it can be helpful if the user expects to use a specific backend for the secret or fails for unexpected reasons (PermissionDenied).
Yes in theory it can be helpful. BUT it can also be misleading! It's like when your airflow cluster goes down and you try and get it back up before your boss notices. You _could_ tell them but is it really helpful to do so 😉?
At my company when I implemented ssm secrets backend a user saw this error and coincidentally happened to be having other problems with a dag he was working on. Not finding the cred was exactly what should have happened because the setup is supposed to use the aws instance profile not airflow connection. But they saw this "error" in the logs and it made it harder for them to find what was the actual issue because they were distracted by this "error". And they had a tough time understanding that this was fully expected and unrelated to their actual issue.
So, while those of us who understand what's happening under the hood can make sense of the error, users may not.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on pull request #15120: Do not log info when ssm secret not found
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #15120:
URL: https://github.com/apache/airflow/pull/15120#issuecomment-812579636
> By logging error, it can be helpful if the user expects to use a specific backend for the secret or fails for unexpected reasons (PermissionDenied).
Yes in theory it can be helpful. BUT it can also be misleading! It's like when your airflow cluster goes down and you try and get it back up before your boss notices ;) You _could_ tell them but is it really helpful to do so 😉?
At my company when I implemented ssm secrets backend a user saw this error and coincidentally happened to be having other problems with a dag he was working on. Not finding the cred was exactly what should have happened because the setup is supposed to use the aws instance profile not airflow connection. So it's saying "error" and he never really believed that this was fully expected and unrelated to his issue. 🙂
So, while those of us who understand what's happening under the hood can make sense of the error, users may not.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish edited a comment on pull request #15120: Do not log info when ssm secret not found
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #15120:
URL: https://github.com/apache/airflow/pull/15120#issuecomment-812579636
> By logging error, it can be helpful if the user expects to use a specific backend for the secret or fails for unexpected reasons (PermissionDenied).
Yes in theory it can be helpful. BUT it can also be misleading! If a particular connection uses env vars or metastore you'll see an "error" in every log and this is misleading and confusing for users.
At my company when I implemented ssm secrets backend a user saw this error and coincidentally happened to be having other problems with a dag he was working on. Not finding the cred was exactly what should have happened because the setup is supposed to use the aws instance profile not airflow connection. But they saw this "error" in the logs and it made it harder for them to find what was the actual issue because they were distracted by this "error". And they had a tough time understanding that this was fully expected and unrelated to their actual issue.
So, while those of us who understand what's happening under the hood can make sense of this "error", users may not.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish edited a comment on pull request #15120: Do not log info when ssm secret not found
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #15120:
URL: https://github.com/apache/airflow/pull/15120#issuecomment-812579636
> By logging error, it can be helpful if the user expects to use a specific backend for the secret or fails for unexpected reasons (PermissionDenied).
Yes in theory it can be helpful. BUT it can also be misleading! It's like when your airflow cluster goes down and you try and get it back up before your boss notices. You _could_ tell them but is it really helpful to do so 😉?
At my company when I implemented ssm secrets backend a user saw this error and coincidentally happened to be having other problems with a dag he was working on. Not finding the cred was exactly what should have happened because the setup is supposed to use the aws instance profile not airflow connection. But they saw this "error" in the logs and it made it harder for them to find what was the actual issue because they were distracted by this "error". And they had a tough time understanding that this was fully expected and unrelated to thier actual issue. 🙂
So, while those of us who understand what's happening under the hood can make sense of the error, users may not.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org