Skip to content

Set logger parents and levels#108

Merged
pamelafox merged 14 commits into
microsoft:mainfrom
pamelafox:logger-parent
Oct 12, 2022
Merged

Set logger parents and levels#108
pamelafox merged 14 commits into
microsoft:mainfrom
pamelafox:logger-parent

Conversation

@pamelafox

Copy link
Copy Markdown
Member

This is my proposal for handling levels in the logger hierarchy in a way which doesn't require parent traversal on every logging call.

This PR adds two new properties to Logger: effective_level and children. The effective_level is based on the level of the parents, or is equal to level if that's explicitly set with setLevel. When setLevel is called on a logger with children, then that level becomes the effective_level of any child with a level of NOT_SET.

I believe it is necessary to keep track of both level and effective_level so that we know which children's effective levels would be affected by a parent change.

Let me know what you think of this approach.

@codecov-commenter

codecov-commenter commented Oct 9, 2022

Copy link
Copy Markdown

Codecov Report

Merging #108 (2180ca3) into main (de33c22) will increase coverage by 0.10%.
The diff coverage is 98.71%.

❗ Current head 2180ca3 differs from pull request most recent head 55f70e8. Consider uploading reports for the commit 55f70e8 to get more accurate results

@@            Coverage Diff             @@
##             main     #108      +/-   ##
==========================================
+ Coverage   88.83%   88.94%   +0.10%     
==========================================
  Files          11       11              
  Lines        2069     2098      +29     
==========================================
+ Hits         1838     1866      +28     
- Misses        231      232       +1     
Impacted Files Coverage Δ
src/picologging/logger.cxx 84.50% <97.50%> (-0.66%) ⬇️
src/picologging/__init__.py 93.98% <100.00%> (+1.28%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@pamelafox

Copy link
Copy Markdown
Member Author

I'm adding additional tests, the C code isn't quite right yet.

Comment thread src/picologging/logger.cxx Outdated
@tonybaloney

Copy link
Copy Markdown
Collaborator

I like the approach, it seems clean and shouldn't really impact the performance of the critical path.

In the tests, it'll be important to use docstrings to describe the setup of the test, e.g.:

def test_scenario_1():
   """
  Before: 

   root - 100
    -- child 1 (NOTSET) - effective 100
    -- child 2 (200) - effective 200

  After:
   root - 200
    -- child 1 (NOTSET) - effective 200
    -- child 2 (200) - effective 300?
   """

@tonybaloney

Copy link
Copy Markdown
Collaborator

If you write a detailed docstring like that, Github copilot will probably write the test for you as well :-)

@pamelafox

Copy link
Copy Markdown
Member Author

I just ran the tests from the CPython test_logging.py file that I had previously marked as unable to pass due to lack of hierarchy support, and they all pass! I could add the CPython tests here, but I think my current tests are pretty good coverage.
I added in the docstrings as well.
Let me know if you'd like any other changes!

@pamelafox

pamelafox commented Oct 12, 2022

Copy link
Copy Markdown
Member Author

TODO:

  • Increase code coverage
  • PlaceHolder -> _Placeholder
  • Run memray tests / add another if needed

Comment thread src/picologging/logger.cxx
@pamelafox
pamelafox merged commit 2d06a87 into microsoft:main Oct 12, 2022
@pamelafox
pamelafox deleted the logger-parent branch October 12, 2022 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants