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/01/13 19:53:23 UTC
[GitHub] [airflow] o-nikolas opened a new pull request #20858: Move some base_aws logging from info to debug level
o-nikolas opened a new pull request #20858:
URL: https://github.com/apache/airflow/pull/20858
Hey folks, this issue was brought up in [this Slack thread](https://apache-airflow.slack.com/archives/CCRR5EBA7/p1641501716009300), where base_aws.py spams scheduler logs with logging regarding how the boto session is being constructed.
base_aws.py was annotated with logging starting in ~2.0, but this
logging was added at info level. Meaning every time a boto session
is created many log lines are added regarding how the session was
constructed. Most of this logging is better suited at the debug
level to avoid spamming the Airflow logs every time a boto session
is created, which can be very frequent.
I've gone through and moved some of the heavy hitters to debug level (including messages that are logged unconditionally), but I tried to leave some of the less common logging to be at info level since I suspected push back. Let me know what folks think!
Here is an example snippet from cloudwatch showing the kind of log spam we're talking about:
<!--
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 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 change, 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 [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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] o-nikolas commented on a change in pull request #20858: Move some base_aws logging from info to debug level
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on a change in pull request #20858:
URL: https://github.com/apache/airflow/pull/20858#discussion_r786231758
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -199,8 +199,6 @@ def _read_credentials_from_connection(self) -> Tuple[Optional[str], Optional[str
self.extra_config.get("profile"),
)
self.log.info("Credentials retrieved from extra_config['s3_config_file']")
- else:
- self.log.info("No credentials retrieved from Connection")
Review comment:
Hey,
There is perhaps a way to modify the logic and the way the conditions are constructed to reduce the scope of the negative case but nothing jumps out at me as being trivial. I really would rather not modify functional code in this PR as it makes it much more risky to cause regressions for the simple goal of log level clean up.
I really do think everything you need is already there: If you're trying to use a connection (in any of the 3 ways above), you'll get a log line printed telling you that you are doing so. If you're trying to use a connection and you don't see such a log being printed, something has gone wrong and you should re-asses. The else logging adds no additional data.
Does adding back the else condition at debug level as a compromise not satisfy your particular use case?
--
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] o-nikolas commented on a change in pull request #20858: Move some base_aws logging from info to debug level
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on a change in pull request #20858:
URL: https://github.com/apache/airflow/pull/20858#discussion_r787128533
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -162,7 +162,7 @@ def _refresh_credentials(self) -> Dict[str, Any]:
raise RuntimeError(f'sts_response_http_status={sts_response_http_status}')
credentials = sts_response['Credentials']
expiry_time = credentials.get('Expiration').isoformat()
- self.log.info(f'New credentials expiry_time:{expiry_time}')
+ self.log.debug(f'New credentials expiry_time:{expiry_time}')
Review comment:
Looks good to me, I've updated the PR with your suggested change :+1:
--
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 #20858: Move some base_aws logging from info to debug level
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20858:
URL: https://github.com/apache/airflow/pull/20858#issuecomment-1012518177
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 main 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.
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 merged pull request #20858: Move some base_aws logging from info to debug level
Posted by GitBox <gi...@apache.org>.
uranusjr merged pull request #20858:
URL: https://github.com/apache/airflow/pull/20858
--
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 change in pull request #20858: Move some base_aws logging from info to debug level
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20858:
URL: https://github.com/apache/airflow/pull/20858#discussion_r784511432
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -199,8 +199,6 @@ def _read_credentials_from_connection(self) -> Tuple[Optional[str], Optional[str
self.extra_config.get("profile"),
)
self.log.info("Credentials retrieved from extra_config['s3_config_file']")
- else:
- self.log.info("No credentials retrieved from Connection")
Review comment:
Personally I’d appreciate having this log since otherwise it’s more difficult to figure out why something _didn’t_ happen.
--
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 change in pull request #20858: Move some base_aws logging from info to debug level
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20858:
URL: https://github.com/apache/airflow/pull/20858#discussion_r786582995
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -162,7 +162,7 @@ def _refresh_credentials(self) -> Dict[str, Any]:
raise RuntimeError(f'sts_response_http_status={sts_response_http_status}')
credentials = sts_response['Credentials']
expiry_time = credentials.get('Expiration').isoformat()
- self.log.info(f'New credentials expiry_time:{expiry_time}')
+ self.log.debug(f'New credentials expiry_time:{expiry_time}')
Review comment:
Unrelated but should this be
```suggestion
self.log.debug('New credentials expiry_time: %s', expiry_time)
```
--
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 change in pull request #20858: Move some base_aws logging from info to debug level
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20858:
URL: https://github.com/apache/airflow/pull/20858#discussion_r785646440
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -199,8 +199,6 @@ def _read_credentials_from_connection(self) -> Tuple[Optional[str], Optional[str
self.extra_config.get("profile"),
)
self.log.info("Credentials retrieved from extra_config['s3_config_file']")
- else:
- self.log.info("No credentials retrieved from Connection")
Review comment:
Would it be possible to add a log only when we know we’re trying to get a credential? Maybe log somewhere else?
--
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] o-nikolas commented on a change in pull request #20858: Move some base_aws logging from info to debug level
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on a change in pull request #20858:
URL: https://github.com/apache/airflow/pull/20858#discussion_r785076651
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -199,8 +199,6 @@ def _read_credentials_from_connection(self) -> Tuple[Optional[str], Optional[str
self.extra_config.get("profile"),
)
self.log.info("Credentials retrieved from extra_config['s3_config_file']")
- else:
- self.log.info("No credentials retrieved from Connection")
Review comment:
Hmm, interesting. The problem with this log line is that it's one of the worst offenders for log spam, since it sits in an else that's guaranteed to run even if you're not trying to use a connection for credentials at all (see the example screenshot I've attached in the conversation tab). Which as far as negative case logging goes, isn't that useful IMHO. This log line also adds no additional runtime context (it logs no runtime data that's useful for debugging) and it can simply be inferred if you don't see any of the other log lines from above.
However, if all that is still not convincing, then perhaps we can agree to add it back, but at debug level?
Let me know what you think!
--
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