-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(auth): lower regional access boundary logs from warning to debug. #17571
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
Open
nbayati
wants to merge
4
commits into
googleapis:main
Choose a base branch
from
nbayati:rab-log-info-level
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
cc424a7
fix(auth): lower regional access boundary logs from warning to info
nbayati f7183b2
test(auth): update async RAB logging tests for INFO level
nbayati ff169aa
refactor(auth): lower RAB lookup log to DEBUG
nbayati fad1e4a
fix unit tests
nbayati File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -219,10 +219,9 @@ def start_blocking_refresh(self, credentials, request): | |
| """ | ||
| # Async credentials do not support blocking lookups. | ||
| if inspect.iscoroutinefunction(credentials._lookup_regional_access_boundary): | ||
| if _helpers.is_logging_enabled(_LOGGER): | ||
| _LOGGER.warning( | ||
| "Blocking Regional Access Boundary lookup is not supported for async credentials." | ||
| ) | ||
| _LOGGER.debug( | ||
| "Blocking Regional Access Boundary lookup is not supported for async credentials." | ||
| ) | ||
| self.process_regional_access_boundary_info(None) | ||
| return | ||
|
|
||
|
|
@@ -234,12 +233,11 @@ def start_blocking_refresh(self, credentials, request): | |
| credentials._lookup_regional_access_boundary(request, fail_fast=True) | ||
| ) | ||
| except Exception as e: | ||
| if _helpers.is_logging_enabled(_LOGGER): | ||
| _LOGGER.warning( | ||
| "Blocking Regional Access Boundary lookup raised an exception: %s", | ||
| e, | ||
| exc_info=True, | ||
| ) | ||
| _LOGGER.debug( | ||
| "Blocking Regional Access Boundary lookup raised an exception: %s", | ||
| e, | ||
| exc_info=True, | ||
| ) | ||
| regional_access_boundary_info = None | ||
|
|
||
| self.process_regional_access_boundary_info(regional_access_boundary_info) | ||
|
|
@@ -265,12 +263,11 @@ async def start_blocking_refresh_async(self, credentials, request): | |
| ) | ||
| ) | ||
| except Exception as e: | ||
| if _helpers.is_logging_enabled(_LOGGER): | ||
| _LOGGER.warning( | ||
| "Regional Access Boundary lookup raised an exception: %s", | ||
| e, | ||
| exc_info=True, | ||
| ) | ||
| _LOGGER.debug( | ||
| "Regional Access Boundary lookup raised an exception: %s", | ||
| e, | ||
| exc_info=True, | ||
| ) | ||
| regional_access_boundary_info = None | ||
|
|
||
| self.process_regional_access_boundary_info(regional_access_boundary_info) | ||
|
|
@@ -297,14 +294,12 @@ def process_regional_access_boundary_info(self, regional_access_boundary_info): | |
| cooldown_expiry=None, | ||
| cooldown_duration=DEFAULT_REGIONAL_ACCESS_BOUNDARY_COOLDOWN, | ||
| ) | ||
| if _helpers.is_logging_enabled(_LOGGER): | ||
| _LOGGER.debug("Regional Access Boundary lookup successful.") | ||
| _LOGGER.debug("Regional Access Boundary lookup successful.") | ||
| else: | ||
| # On failure, calculate cooldown and update state. | ||
| if _helpers.is_logging_enabled(_LOGGER): | ||
| _LOGGER.warning( | ||
| "Regional Access Boundary lookup failed. Entering cooldown." | ||
| ) | ||
| _LOGGER.debug( | ||
| "Regional Access Boundary lookup failed. Entering cooldown." | ||
| ) | ||
|
|
||
| next_cooldown_expiry = ( | ||
| _helpers.utcnow() + current_data.cooldown_duration | ||
|
|
@@ -367,12 +362,11 @@ def run(self): | |
| self._credentials._lookup_regional_access_boundary(self._request) | ||
| ) | ||
| except Exception as e: | ||
| if _helpers.is_logging_enabled(_LOGGER): | ||
| _LOGGER.warning( | ||
| "Asynchronous Regional Access Boundary lookup raised an exception: %s", | ||
| e, | ||
| exc_info=True, | ||
| ) | ||
| _LOGGER.debug( | ||
| "Asynchronous Regional Access Boundary lookup raised an exception: %s", | ||
| e, | ||
| exc_info=True, | ||
| ) | ||
| regional_access_boundary_info = None | ||
|
|
||
| self._rab_manager.process_regional_access_boundary_info( | ||
|
|
@@ -419,13 +413,12 @@ def start_refresh(self, credentials, request, rab_manager): | |
| try: | ||
| copied_request = copy.deepcopy(request) | ||
| except Exception as e: | ||
| if _helpers.is_logging_enabled(_LOGGER): | ||
| _LOGGER.warning( | ||
| "Could not deepcopy transport for background RAB refresh. " | ||
| "Skipping background refresh to avoid thread safety issues. " | ||
| "Exception: %s", | ||
| e, | ||
| ) | ||
| _LOGGER.debug( | ||
| "Could not deepcopy transport for background RAB refresh. " | ||
| "Skipping background refresh to avoid thread safety issues. " | ||
| "Exception: %s", | ||
| e, | ||
| ) | ||
| return | ||
|
|
||
| self._worker = _RegionalAccessBoundaryRefreshThread( | ||
|
|
@@ -479,14 +472,13 @@ async def _close_cloned_request(lookup_request, is_cloned): | |
| if is_async := inspect.isawaitable(maybe_coro): | ||
| await maybe_coro | ||
| except Exception as e: | ||
| if _helpers.is_logging_enabled(_LOGGER): | ||
| adapter_type = " asynchronous " if is_async else " " | ||
| _LOGGER.warning( | ||
| "Failed to cleanly close cloned%srequest transport: %s", | ||
| adapter_type, | ||
| e, | ||
| exc_info=True, | ||
| ) | ||
| adapter_type = " asynchronous " if is_async else " " | ||
| _LOGGER.debug( | ||
| "Failed to cleanly close cloned%srequest transport: %s", | ||
| adapter_type, | ||
| e, | ||
| exc_info=True, | ||
| ) | ||
|
|
||
|
|
||
| class _AsyncRegionalAccessBoundaryRefreshManager(object): | ||
|
|
@@ -532,12 +524,11 @@ def start_refresh(self, credentials, request, rab_manager): | |
| is_cloned, | ||
| ) = _prepare_async_lookup_callable(request) | ||
| except Exception as e: | ||
| if _helpers.is_logging_enabled(_LOGGER): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto on the removal of this check |
||
| _LOGGER.warning( | ||
| "Synchronous cloning of request for Regional Access Boundary lookup failed: %s", | ||
| e, | ||
| exc_info=True, | ||
| ) | ||
| _LOGGER.debug( | ||
| "Synchronous cloning of request for Regional Access Boundary lookup failed: %s", | ||
| e, | ||
| exc_info=True, | ||
| ) | ||
| rab_manager.process_regional_access_boundary_info(None) | ||
| return | ||
|
|
||
|
|
@@ -549,12 +540,11 @@ async def _worker(): | |
| ) | ||
| ) | ||
| except Exception as e: | ||
| if _helpers.is_logging_enabled(_LOGGER): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto |
||
| _LOGGER.warning( | ||
| "Asynchronous Regional Access Boundary lookup raised an exception: %s", | ||
| e, | ||
| exc_info=True, | ||
| ) | ||
| _LOGGER.debug( | ||
| "Asynchronous Regional Access Boundary lookup raised an exception: %s", | ||
| e, | ||
| exc_info=True, | ||
| ) | ||
| regional_access_boundary_info = None | ||
| finally: | ||
| await _close_cloned_request(lookup_request, is_cloned) | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 did we get rid of the is_logging_enabled check here? Was that intentional?
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.
Yes it was intentional. Previously, these were logged a WARNING. Because warnings are printed by default, we had to manually wrap it in that if check (which checks for DEBUG level) to hide it from normal users.
Now that we are logging them as _LOGGER.debug, they are already hidden by default. The logging library automatically filters out DEBUG logs, so we no longer need to write a manual if check to hide them.