Skip to content

bpo-30962: Add caching to Logger.isEnabledFor() #2752

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.

Sign up for GitHub

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

Merged
vsajip merged 7 commits into from
Jul 30, 2017
Merged

bpo-30962: Add caching to Logger.isEnabledFor() #2752

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
26a8efc
Add caching to Logger.isEnabledFor()
avylove Jul 18, 2017
aeec4d9
Fix bugs in proposed caching mechanism
avylove Jul 18, 2017
7deaaab
Save newly cached result to local variable to avoid race condition
avylove Jul 18, 2017
f3b23ad
Missing clearing of root logger cache
avylove Jul 19, 2017
d10e0e2
Add test for log caching
avylove Jul 28, 2017
d6d28ec
Update log caching tests
avylove Jul 28, 2017
11fee13
reset manager.disable on log test teardown
avylove Jul 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions Lib/test/test_logging.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4406,6 +4406,29 @@ def test_basic(self):
self.assertTrue(found, msg=msg)


class CachingTest(BaseTest):
Copy link
Member

@vsajip vsajip Jul 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could add this to an existing test case class rather than adding a separate class just for this one method.

Copy link
Contributor Author

@avylove avylove Jul 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll move it to the LoggerTest class

def test_caching(self):
root = self.root_logger
level1 = logging.getLogger("abc")
Copy link
Member

@vsajip vsajip Jul 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are loggers, not levels, and the names should reflect that. I understand you're talking about level in the logger hierarchy, but still, the use of level for the loggers is potentially confusing.

Copy link
Contributor Author

@avylove avylove Jul 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Will fix

level2 = logging.getLogger("abc.def")

root.setLevel(logging.ERROR)
self.assertEqual(level2._cache, {}) # Cache is empty

self.assertTrue(level2.isEnabledFor(logging.ERROR))
self.assertEqual(level2._cache, {logging.ERROR: True}) # Cache is populated
Copy link
Member

@vsajip vsajip Jul 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What values would the logger level cache dict have? Either empty or a level pointing to True? Could there be multiple keys in this dict?

Copy link
Contributor Author

@avylove avylove Jul 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keys are the levels passes to isEnabledFor(), values are always True or False. I can lookup more levels to get a more well-rounded test.

self.assertEqual(root._cache, {}) # Root cache is empty
self.assertTrue(level2.isEnabledFor(logging.ERROR))

level1.setLevel(logging.CRITICAL)
self.assertEqual(level2._cache, {}) # Cache is empty
self.assertEqual(root._cache, {}) # Root cache is empty

self.assertFalse(level2.isEnabledFor(logging.ERROR))
self.assertTrue(root.isEnabledFor(logging.ERROR))
self.assertEqual(root._cache, {logging.ERROR: True}) # Root cache is populated

Copy link
Member

@vsajip vsajip Jul 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should do level2.setLevel(logging.NOTSET). and see that the isEnabledFor() for the two loggers and the root logger are as expected.

Also, it makes sense to add some tests for getEffectiveLevel() to ensure that the values are as expected without the presence of caching.


class MiscTestCase(unittest.TestCase):
def test__all__(self):
blacklist = {'logThreads', 'logMultiprocessing',
Expand Down