This repository was archived by the owner on Jan 19, 2024. It is now read-only.
Avoid mutating the root logger#119
Merged
srinathnarayanan merged 2 commits intoAzure:masterfrom May 2, 2018
c-w:fix-root-logger-mutation
Merged
Avoid mutating the root logger#119srinathnarayanan merged 2 commits intoAzure:masterfrom c-w:fix-root-logger-mutation
srinathnarayanan merged 2 commits intoAzure:masterfrom
c-w:fix-root-logger-mutation
Conversation
Using `logging.basicConfig(level=logging.INFO)` mutates the root logger for the current Python interpreter. This means that any library which imports the pydocumentdb library will have its log level set to INFO which leads to unexpected behavior. Some libraries even have tests to defend against this behavior, e.g. Celery: https://github.com/celery/celery/blob/120770929f/t/unit/conftest.py#L231-L244 To fix this unexpected behavior, this commit introduces a new logger that's specific to the endpoint discovery retry policy and only mutates the log level of that specific logger.
| self._max_retry_attempt_count = _EndpointDiscoveryRetryPolicy.Max_retry_attempt_count | ||
| self.current_retry_attempt_count = 0 | ||
| self.retry_after_in_milliseconds = _EndpointDiscoveryRetryPolicy.Retry_after_in_milliseconds | ||
| logging.basicConfig(format='%(levelname)s:%(message)s', level=logging.INFO) |
Contributor
There was a problem hiding this comment.
Can you also please retain the format of the logger.
Contributor
Author
There was a problem hiding this comment.
I didn't do that originally because it adds a little bit of boilerplate, but sure thing. Emulated the full basicConfig code path in bb74225, modeled on the Python 3.6.4 implementation.
Contributor
Author
There was a problem hiding this comment.
Ping @srinathnarayanan -- could you please take another look at the pull request?
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
Using
logging.basicConfig(level=logging.INFO)mutates the root logger for the current Python interpreter. This means that any library which imports the pydocumentdb library will have its log level set toINFOwhich leads to unexpected behavior.Some libraries even have tests to defend against this behavior, e.g. Celery: https://github.com/celery/celery/blob/120770929f/t/unit/conftest.py#L231-L244
To fix this unexpected behavior, this commit introduces a new logger that's specific to the endpoint discovery retry policy and only mutates the log level of that specific logger.