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.

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

Closed
wants to merge 9 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
977e1e0
Add print statements to highlight the bug
Aug 4, 2022
824893b
Fix the rollover for the blf writer
Aug 4, 2022
6f0a298
Fix change to run with all logger types
Aug 4, 2022
276b253
Remove unnecessary statement
Aug 4, 2022
c195001
Remove unnecessary print statement for file.tell()
Aug 4, 2022
c68f155
Add **options to API of all Writers
j-c-cook Aug 5, 2022
3c35108
Add type definition to **options
j-c-cook Aug 6, 2022
611fc4f
Update help documentation for -s logger option
j-c-cook Aug 6, 2022
55401ea
Refactor line width
j-c-cook Aug 6, 2022
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
4 changes: 1 addition & 3 deletions can/io/asc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,9 +349,7 @@ class ASCWriter(FileIOMessageWriter):
FORMAT_EVENT = "{timestamp: 9.6f} {message}\n"

def __init__(
self,
file: Union[StringPathLike, TextIO],
channel: int = 1,
self, file: Union[StringPathLike, TextIO], channel: int = 1, **options: Any
) -> None:
"""
:param file: a path-like object or as file-like object to write to
Expand Down
11 changes: 7 additions & 4 deletions can/io/blf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import datetime
import time
import logging
from typing import List, BinaryIO, Generator, Union, Tuple, Optional, cast
from typing import List, BinaryIO, Generator, Union, Tuple, Optional, cast, Any

from ..message import Message
from ..util import len2dlc, dlc2len, channel2int
Expand Down Expand Up @@ -358,9 +358,6 @@ class BLFWriter(FileIOMessageWriter):

file: BinaryIO

#: Max log container size of uncompressed data
max_container_size = 128 * 1024

#: Application identifier for the log writer
application_id = 5

Expand All @@ -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 •
edited
Loading

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.

) -> None:
"""
:param file: a path-like object or as file-like object to write to
Expand Down Expand Up @@ -400,6 +398,11 @@ def __init__(
self.compression_level = compression_level
self._buffer: List[bytes] = []
self._buffer_size = 0
#: 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
Comment on lines +401 to +405
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.

if append:
# Parse file header
data = self.file.read(FILE_HEADER_STRUCT.size)
Expand Down
3 changes: 2 additions & 1 deletion can/io/canutils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"""

import logging
from typing import Generator, TextIO, Union
from typing import Generator, TextIO, Union, Any

from can.message import Message
from .generic import FileIOMessageWriter, MessageReader
Expand Down Expand Up @@ -132,6 +132,7 @@ def __init__(
file: Union[StringPathLike, TextIO],
channel: str = "vcan0",
append: bool = False,
**options: Any,
):
"""
:param file: a path-like object or as file-like object to write to
Expand Down
4 changes: 2 additions & 2 deletions can/io/csv.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
"""

from base64 import b64encode, b64decode
from typing import TextIO, Generator, Union
from typing import TextIO, Generator, Union, Any

from can.message import Message
from .generic import FileIOMessageWriter, MessageReader
Expand Down Expand Up @@ -87,7 +87,7 @@ class CSVWriter(FileIOMessageWriter):
file: TextIO

def __init__(
self, file: Union[StringPathLike, TextIO], append: bool = False
self, file: Union[StringPathLike, TextIO], append: bool = False, **options: Any
) -> None:
"""
:param file: a path-like object or a file-like object to write to.
Expand Down
6 changes: 4 additions & 2 deletions can/logger.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,8 +170,9 @@ def main() -> None:
"--file_size",
dest="file_size",
type=int,
help="Maximum file size in bytes. Rotate log file when size threshold "
"is reached.",
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.",
Comment on lines +173 to +175
Copy link
Owner

Choose a reason for hiding this comment

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

👍🏼

default=None,
)

Expand Down Expand Up @@ -215,6 +216,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.

logger = SizedRotatingLogger(
base_filename=results.log_file, max_bytes=results.file_size, **options
)
Expand Down