Skip to content
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

Add a log rotation function to the file handler #56

Merged
merged 9 commits into from
May 18, 2024

Conversation

ilikecake
Copy link
Contributor

Adds a simple log rotation function to the file handler. This will keep the log files from growing without bound.

This update adds two optional variables to the FileHandler class.

  • LogFileSizeLimit: the size limit of the log file in bytes.
  • OldFilePostfix: What to append to the log filename to save the old log.

If the file size limit is set, when the emit function is called, the size of the log file will be compared to the limit. If the file is larger than the limit, it will be renamed and a new log file will be started. The old log file is renamed by appending OldFilePostfix to the file name. If there is another file with this name, it will be deleted.

If the file size limit is not set, the class acts the same as before this change.

I have tested this on an ESP32-S3 Feather running CircuitPython 9.0.3.

@tannewt tannewt requested a review from a team April 29, 2024 17:17
@dhalbert
Copy link
Contributor

dhalbert commented May 6, 2024

We are not ignoring this, but this adds quite a bit of code to a simple library, and we need to check the consequences of doing that: does it break some existing examples, etc.?

@ilikecake
Copy link
Contributor Author

Thanks for the review. I considered making this a separate library that monitored log sizes. I decided to modify this library instead because:

  1. As it currently works, the file writer will eventually fill all the free space on the drive and then crash. Given what people probably use it for, that is pretty unlikley. However, it still seems bad to not catch those potential errors.
  2. I (hopefully) wrote the updates such that any current code should still work without any changes needed.

Copy link
Contributor

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

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

Thanks for adding this functionality @ilikecake!

I tested the existing examples and confirmed that this PR does not cause any issues with them.

Instead of implementing this functionality inside of FileHandler class directly I think it would be best to create RotatingFileHandler as a subclass of FileHandler and have it be a strict subset of the CPython API and functionality: https://docs.python.org/3/library/logging.handlers.html#rotatingfilehandler

We don't necessarily need to implement 100% percent of it's functionality, but any thing we do implement should match the CPython API to whatever extent is possible.

So RotatingFileHandler would have the argument named maxBytes instead of LogFileSizeLimit and we would leave out the OldFilePostfix argument and behavior. But we could still create another subclass of RotatingFileHandler like CustomNameRotatingFileHandler or something which does have the OldFilePostfix argument and behavior. Even though the CPython API doesn't have that option that I can find, it does seem like nice functionality to me, but we should just move it so that it doesn't break CPython compatibility.

Update the code to make a subclass of filehandler that implements some of the RotatingFileHandler from: https://docs.python.org/3/library/logging.handlers.html#rotatingfilehandler
Remove some debug statements
@ilikecake
Copy link
Contributor Author

Thanks for the review. I have updated the code per your suggestion. I have done some tests with my hardware and it seems to be working correctly. Is there a way to generate the doc files locally so that I can make sure the formatting is right? I try the procedure from here, but it gives me an error.

WARNING: autodoc: failed to import module 'adafruit_logging'; the following exception was raised:
Traceback (most recent call last):
  File "C:\Users\pat\AppData\Local\Programs\Python\Python312\Lib\site-packages\sphinx\ext\autodoc\importer.py", line 143, in import_module
    return importlib.import_module(modname)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\pat\AppData\Local\Programs\Python\Python312\Lib\importlib\__init__.py", line 90, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap>", line 1387, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1360, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1331, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 935, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 995, in exec_module
  File "<frozen importlib._bootstrap>", line 488, in _call_with_frames_removed
  File "C:\Users\pat\Documents\Projects\GitHub\logging\Adafruit_CircuitPython_Logging\adafruit_logging.py", line 184, in <module>
    class StreamHandler(Handler):
  File "C:\Users\pat\Documents\Projects\GitHub\logging\Adafruit_CircuitPython_Logging\adafruit_logging.py", line 195, in StreamHandler
    def __init__(self, stream: Optional[WriteableStream] = None) -> None:
                                        ^^^^^^^^^^^^^^^
NameError: name 'WriteableStream' is not defined

Copy link
Contributor

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

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

@ilikecake thanks for implementing those changes. This is great functionality to have for this library!

These changes are looking good to me now. I tested successfully with the existing FileHandler example and a modified one to use the RotatingFileHandler on a Memeento 9.0.4.

With regards to the docs build: Currently the docs build infrastructure does not support python 3.12. You found the right process, but it's not currently compatible with that version of python. For now you'll need to use python 3.11 or lower in order to build the docs locally.

The typing_extensions module is different now so this: from typing_extensions import Protocol is raising an exception which then results in the WriteableStream protocol class not being created.

@FoamyGuy FoamyGuy merged commit f683fef into adafruit:main May 18, 2024
1 check passed
@dhalbert
Copy link
Contributor

I think that if pylint is bumped to 3.1.0 in .pre-commit-config.yaml, Python 3.12 will work.

adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request May 19, 2024
@ilikecake ilikecake deleted the filehandler-roll-logs branch June 3, 2024 03:11
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