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

Generalize the RotatingLogger; size, time or first of the two trigger the rollover #1365

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

j-c-cook
Copy link
Contributor

@j-c-cook j-c-cook commented Aug 7, 2022

A RotatingLogger object is created that constrains the file by file size, time or both. In the case of when both are constrained, the first that occurs will trigger the rollover.

  • Adds a deprecation warning to the TimedRotatingLogger. The TimedRotatingLogger now inherits from the generalized RotatingLogger.
  • When time is a constraint for the RotatingLogger, don't rollover if there have been no messages received since the last rollover
  • Update the doc string under RotatingLogger
  • Fix the tests to make them run with these changes

closes #1364

- Adds a deprecation warning to the TimedRotatingLogger. The
  TimedRotatingLogger now inherits from the generalized
  RotatingLogger.
@j-c-cook
Copy link
Contributor Author

The BLFWriter will be able to be used like the following:

python -m can.logger -i socketcan -c vcan0 -b 250000 -f file.blf -t 600 --compression-level=1 --max-container-size=200000

Here, a rollover would occur every 5 seconds, or when the buffer and file size becomes 200000. Note that the default max_container_size is 128 * 1024.

Another option would be:

python -m can.logger -i socketcan -c vcan0 -b 250000 -f file.log -t 600 -s 100000

A rollover would occur every 5 seconds, or when the file size becomes 100000 bytes.

@j-c-cook
Copy link
Contributor Author

j-c-cook commented Aug 11, 2022

  • When time is a constraint for the RotatingLogger, don't rollover if there have been no messages received since the last rollover

This is inherent to the implementation of logger. When a message is received then the logger is called. The file cannot rollover without there being a message passed to logger.

python-can/can/logger.py

Lines 246 to 251 in 88fc8e5

try:
while True:
msg = bus.recv(1)
if msg is not None:
logger(msg)
except KeyboardInterrupt:

@codecov
Copy link

codecov bot commented Sep 29, 2022

Codecov Report

Merging #1365 (22bf537) into develop (4b1acde) will increase coverage by 0.12%.
The diff coverage is 100.00%.

❗ Current head 22bf537 differs from pull request most recent head cabafb4. Consider uploading reports for the commit cabafb4 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1365      +/-   ##
===========================================
+ Coverage    65.16%   65.29%   +0.12%     
===========================================
  Files           81       81              
  Lines         8853     8779      -74     
===========================================
- Hits          5769     5732      -37     
+ Misses        3084     3047      -37     

@j-c-cook j-c-cook marked this pull request as ready for review September 29, 2022 15:17
@j-c-cook
Copy link
Contributor Author

@zariiii9003 This RotatingLogger class has been successfully running on multiple devices, that I manage, for about a month now (prior to the refactoring I've done today). There have been tens and possibly hundreds of hours of testing on the class by now. It is ready for a review. Please let me know what you think, thanks.

@j-c-cook j-c-cook mentioned this pull request Dec 25, 2022
Copy link
Collaborator

@felixdivo felixdivo left a comment

Choose a reason for hiding this comment

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

Regarding the discussion in #1364: I generally think that separating what can be split is a fair goal. In this case, however, I think having one class for this makes a lot of sense since it does simplify both user code and the implementation (I think having cooperating sub-classes to have a logger that supports both variants of rotations will not be easier to understand than this).

There are also a few nice small improvements in here that should be merged regardless of the new features!

We should finish the talk about the general API in #1364 before merging here.

can/logger.py Outdated Show resolved Hide resolved
can/io/logger.py Show resolved Hide resolved
@felixdivo felixdivo added this to the Next Release milestone Dec 28, 2022
@felixdivo felixdivo added enhancement file-io about reading & writing to files labels Dec 28, 2022
j-c-cook and others added 5 commits December 28, 2022 10:25
Co-authored-by: Felix Divo <[email protected]>
Here is what was stated in the warning message in the `docs` workflow:
Warning, treated as error:
/home/runner/work/python-can/python-can/can/io/logger.py:docstring of can.io.logger.RotatingLogger:6:py:attr reference target not found: namer
Error: Process completed with exit code 2.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement file-io about reading & writing to files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TimedRotatingLogger
2 participants