Skip to content

Fix the sized rotating logger size bug for the blf writer #1360

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

Closed

Conversation

j-c-cook
Copy link
Contributor

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

I see two options here:

  1. The -s option for the blf rolling logger will not define the file size to be written, but rather the buffer size prior to compression
  2. A mathematical equation is to be written that computes the expected file size of the buffer based on the compression integer passed to zlib.compress. (This would require either some knowledge of how the compression functions, or could be an equation fit made from a table of collected values based on a range of compressed buffer sizes vs. the input integer.)

I plan to move forward shortly with a commit that will implement the solution (1).

closes #1359

@codecov
Copy link

codecov bot commented Aug 4, 2022

Codecov Report

Merging #1360 (55401ea) into develop (b9d9d01) will increase coverage by 0.00%.
The diff coverage is 85.71%.

@@           Coverage Diff            @@
##           develop    #1360   +/-   ##
========================================
  Coverage    66.06%   66.06%           
========================================
  Files           86       86           
  Lines         8973     8976    +3     
========================================
+ Hits          5928     5930    +2     
- Misses        3045     3046    +1     

@@ -215,6 +215,7 @@ def main() -> None:

options = {"append": results.append}
if results.file_size:
options["max_container_size"] = results.file_size # bytes
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm now passing in max_container_size as an option.

@@ -370,6 +367,7 @@ def __init__(
append: bool = False,
channel: int = 1,
compression_level: int = -1,
**options: Any,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added **options to BLFWriter API.

Copy link
Owner

Choose a reason for hiding this comment

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

Assuming that compression_level is specific to the BLFWriter, perhaps it should be moved into the options dict as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Arguments specific to a Writer could be made more accessible if able to be set by **options . Currently, the only way to set compression_level would be to bypass the Logger (or RollingLogger) object and call the BLFWriter object itself.

When going about these changes, I'll want to make sure that the API is either unaffected by this improvement, or that a deprecation warning is added (similar to what I did here for SizedRotatingLogger).

Copy link
Collaborator

@zariiii9003 zariiii9003 Aug 8, 2022

Choose a reason for hiding this comment

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

It is specific to the BlfWriter (and soon Mf4Writer) but i think it's more readable if it is not hidden away in **options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have confirmed that compression_level can be modified by passing via the argument in **kwargs. @zariiii9003's CLI arguments https://github.com/zariiii9003/python-can/tree/extra_args2 will provide the ability to modify variables such as compression_level. For example:

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

Once #1366 is merged, I will close this PR. The combination of #1367 and #1366 will resolve everything I had intended from this PR.

Comment on lines +401 to +405
#: Max log container size of uncompressed data
if "max_container_size" in options:
self.max_container_size = options["max_container_size"]
else:
self.max_container_size = 128 * 1024 # bytes
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is now an adjustable max buffer size.

@j-c-cook
Copy link
Contributor Author

j-c-cook commented Aug 4, 2022

@zariiii9003 Ready for review.

@j-c-cook j-c-cook marked this pull request as ready for review August 4, 2022 21:27
Comment on lines +173 to +175
help="Maximum file size in bytes (or for the case of blf, maximum "
"buffer size before compression and flush to file). Rotate log "
"file when size threshold is reached.",
Copy link
Owner

Choose a reason for hiding this comment

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

👍🏼

@@ -370,6 +367,7 @@ def __init__(
append: bool = False,
channel: int = 1,
compression_level: int = -1,
**options: Any,
Copy link
Owner

Choose a reason for hiding this comment

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

Assuming that compression_level is specific to the BLFWriter, perhaps it should be moved into the options dict as well?

@zariiii9003
Copy link
Collaborator

@hardbyte i see you approved this but personally i think bringing implementation details of the BlfWriter (options["max_container_size"] = results.file_size # bytes) into the logger script is not a pretty solution.

What do you think about adding a size property to all FileIOMessageWriter subclasses? That way we could delegate size estimation to the logger implementations.

@j-c-cook j-c-cook marked this pull request as draft August 8, 2022 02:41
@j-c-cook j-c-cook closed this Aug 23, 2022
@j-c-cook j-c-cook deleted the issue1359_BLFWriterFileSize branch August 23, 2022 14:21
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.

Sized Rotating Logger bug for BLFWriter file_size (-s) argument
3 participants