-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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.
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
Set default wasb Azure http logging level to warning; fixes #16224 #18896
Conversation
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
|
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. |
@@ -110,6 +112,12 @@ def __init__(self, wasb_conn_id: str = default_conn_name, public_read: bool = Fa | |||
self.public_read = public_read | |||
self.connection = self.get_conn() | |||
|
|||
logger = logging.getLogger("azure.core.pipeline.policies.http_logging_policy") | |||
try: | |||
logger.setLevel(os.environ.get("AZURE_HTTP_LOGGING_LEVEL", logging.WARNING)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
If this configurability is really desired, I’d use airflow.settings
for this.
from airflow import settings
logger.setLevel(getattr(settings, "AZURE_HTTP_LOGGING_LEVEL", logging.WARNING))
So a user can add AZURE_HTTP_LOGGING_LEVEL
to airflow_local_settings
if they so desire. This will need an entry in documentation as well.
With that, I won’t add the ValueError
block. If the user provides something that crashes Airflow, that’s the user’s problem and we don’t need to do things too magically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can make those updates and reopen in a new pull request since the bot closed this one. I was mostly thinking the user might want to tweak the log level for these in particular and I was unaware of the standard settings approach. Thanks for the feedback!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can re-open if you wish to continue work on it (you will need to rebase to latest main)
This would be really nice to have! @havocbane did you give up on getting this merged in? I think it's a quite necessary configuration to have to avoid getting overwhelmed with nonsense service logging from the Azure library. |
I also rebased it (just in case) |
Awesome work, congrats on your first merged pull request! |
Closes: #16224
Set default Azure wasb http logging level to warning and allow the user to override with an environment variable named "AZURE_HTTP_LOGGING_LEVEL". In the case that a bad level is passed through the environment variable, this will default back to the warning level.
If accepted, please tag the merge with the label hacktoberfest-accepted.