Addition of TimedRotatingFileHandler to log_config.yaml for iCtrl's logging configuration #46
Conversation
…e logged in files
WalkthroughThe pull request updates the logging configuration across several files. In Changes
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
li-ruihao
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
log_config.yaml(2 hunks)
🔇 Additional comments (1)
log_config.yaml (1)
2-2: Verify impact of enabling existing loggers.While the change is justified by the logger in
application/__init__.py, let's verify there are no other loggers that could be affected.✅ Verification successful
Setting
disable_existing_loggers: Falseis correct and safeAll logger instances in the codebase follow the standard Python pattern using
getLogger(__name__), which creates properly hierarchical module-level loggers. These loggers should remain enabled to maintain the logging hierarchy and proper configuration inheritance from the root logger.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for logger initializations across the codebase rg -l "getLogger|basicConfig" --type py # Search for potential custom logger implementations ast-grep --pattern 'class $_(Logger|$_Logger)'Length of output: 168
Script:
#!/bin/bash # Check logger initialization patterns in the identified files echo "=== application/__init__.py ===" rg "getLogger|basicConfig" -A 2 application/__init__.py echo -e "\n=== application/features/VNC.py ===" rg "getLogger|basicConfig" -A 2 application/features/VNC.py echo -e "\n=== application/routes/vnc.py ===" rg "getLogger|basicConfig" -A 2 application/routes/vnc.pyLength of output: 723
log_config.yaml
Outdated
| file_rotating: | ||
| class: logging.handlers.TimedRotatingFileHandler | ||
| level: DEBUG | ||
| filename: ictrl_log.log | ||
| formatter: default | ||
| when: 'midnight' | ||
| interval: 1 | ||
| backupCount: 14 |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider enhancing the rotating file handler configuration.
While the basic setup is functional, consider the following improvements:
- Use absolute path or environment variable for log file location
- Add file permissions configuration
- Configure error handling for file operations
- Add size limits to prevent disk space issues
Example enhancement:
file_rotating:
class: logging.handlers.TimedRotatingFileHandler
level: DEBUG
- filename: ictrl_log.log
+ filename: ${ICTRL_LOG_DIR}/ictrl_log.log
formatter: default
when: 'midnight'
interval: 1
backupCount: 14
+ encoding: utf-8
+ delay: true
+ mode: 0o644
+ maxBytes: 52428800 # 50MBCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
log_config.yaml (1)
Line range hint
1-26: Document temporary nature of this solution.Since this is a temporary solution pending PR #43's custom CLP rotating handler:
version: 1 +# TODO: Temporary logging configuration using TimedRotatingFileHandler +# To be replaced with custom CLP rotating handler (see PR #43) disable_existing_loggers: False
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
application/__init__.py(1 hunks)log_config.yaml(2 hunks)
🔇 Additional comments (4)
log_config.yaml (4)
2-2: LGTM: Disabling existing loggers.The change to
disable_existing_loggers: Falseis appropriate given the logger initialization inapplication/__init__.py.
26-26: Consider environment-specific logging configurations.This comment was previously made and remains valid. Consider separating configurations for different environments (development, staging, production).
15-22: 🛠️ Refactor suggestionEnhance file handler security and robustness.
While the basic TimedRotatingFileHandler setup works, consider these security and robustness improvements:
file_rotating: class: logging.handlers.TimedRotatingFileHandler level: DEBUG - filename: logs/ictrl_log.log + filename: %(ICTRL_LOG_DIR)s/ictrl_log.log formatter: default when: 'midnight' interval: 1 backupCount: 14 + encoding: utf-8 + mode: 0o644 + maxBytes: 52428800 # 50MBConsider:
- Use environment variable for log directory
- Set explicit file permissions
- Add size limits to prevent disk space issues
- Specify UTF-8 encoding for proper character handling
Likely invalid or redundant comment.
15-22: Verify no conflicts with existing log rotation.Let's verify there are no conflicts with existing log rotation mechanisms:
✅ Verification successful
No conflicts found with log rotation configuration
The log rotation setup in
log_config.yamlis the only logging configuration in the codebase, and there are no conflicting rotation mechanisms present.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing log rotation configurations or handlers rg -l "RotatingFileHandler|TimedRotatingFileHandler" --type pyLength of output: 64
Script:
#!/bin/bash # Search for YAML/JSON files that might contain logging configuration echo "=== Searching for logging config files ===" fd -e yaml -e yml -e json | xargs rg -l "logging|handler|rotate" echo -e "\n=== Searching for any log rotation related terms ===" rg -i "log.?rotat|rotating.?log" --type yaml --type json --type pyLength of output: 415
log_config.yaml
Outdated
| formatter: default | ||
| when: 'midnight' | ||
| interval: 1 | ||
| backupCount: 14 |
There was a problem hiding this comment.
How did we arrive at this count? Do we have an estimate of how many logs iCtrl generates per hour? Then how many (bytes of) logs do we plan to keep on the user's machine? Note this should not exceed the application environment requirements in your previous analysis, say in the Project Proposal.
There was a problem hiding this comment.
Initially, I just thought it was a good idea to keep a backup for the past 2 weeks' logs. However let's do some calculation given what we wrote in the project proposal.
We are using the following format for our log message:
'%(asctime)s %(levelname)s [%(name)s:%(lineno)d] %(message)s'.
Let's make some very bad assumptions for each value to estimate the length of a very long log message.
- %(asctime)s: This value is fixed to be 23 characters
- %(levelname)s: the longest level name is CRITICAL, which is 8 characters
- %(name)s: Assume the longest Python file name to be 20 characters
- %(lineno)d: Assume the longest line number to be 5 digits(characters)
- %(message)s: Assume a very detailed log message that has 200 characters
- Other characters such as space sum up to a total of 6 characters, and a newline character consists of CR and LF which adds 2 to 6 becomes 8 characters
In total, there are 264 characters, so each message has 528 bytes assuming each character is 2 bytes according to the UTF standard, which by default is 2 bytes each.
Assuming 5 log messages are generated each second, which is 2640 bytes/second. Then each 3-hour period will generate 60 x 60 x 3 x 2640 = 28,512,000 bytes of data which is 27.1912 MB. Let's round it up to 28 MB to include some metadata generated from file rotation and make the calculation easier.
The requirement on the project proposal said that the size of the logs generated by iCtrl cannot exceed 32GB. Originally we are backing up 2 weeks of data, that is 112 rotation periods. The total size for log backups would be 112 x 28 = 3,136 MB = 3.0625 GB, which is far from breaking the constraint requirement.
Therefore, I think 2 weeks of backup data is appropriate, and the calculations above show that the system can allow more backups, if the assumptions are bad enough.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
application/__init__.py(1 hunks)application/paths.py(1 hunks)log_config.yaml(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
application/__init__.py
24-24: application.paths imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
34-34: Undefined name paths
(F821)
35-35: Undefined name paths
(F821)
35-35: Undefined name paths
(F821)
🔇 Additional comments (6)
log_config.yaml (3)
15-22: LGTM! The timedRotatingFile configuration is well-thought-out.The 3-hour rotation interval and 112 backup count are justified by the storage calculations in the past review comments. The dynamic filename assignment is a good practice.
26-26: LGTM! Both console and file logging are correctly configured.The root logger is properly configured to use both handlers.
Line range hint
2-2: Verify the impact of disable_existing_loggers: True.Based on the past review comments and PR objectives, this setting might conflict with the logger initialized in
application/__init__.py. Consider setting it toFalseas originally intended.-disable_existing_loggers: True +disable_existing_loggers: Falseapplication/__init__.py (3)
33-35: LGTM! The logging setup is robust.The code correctly:
- Sets the log file path dynamically
- Creates the logs directory if it doesn't exist
- Applies the configuration
🧰 Tools
🪛 Ruff (0.8.2)
34-34: Undefined name
paths(F821)
35-35: Undefined name
paths(F821)
35-35: Undefined name
paths(F821)
41-44: LGTM! Logger initialization is properly handled.The logger initialization is correctly placed in both the exception and success paths, ensuring logging is available regardless of configuration success.
24-24: 🧹 Nitpick (assertive)Fix the import statement.
The static analysis tool flags the import as unused. Use a direct import to make the usage clear.
-import application.paths +from application import pathsLikely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.2)
24-24:
application.pathsimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
Description
Since the logging integration pull requests such as PR #43 are still under review and the implementation of a CLP rotating handler is not completed yet, Python's native TimedRotatingFileHandler was added into log_config.yaml as a temporary solution to allow log messages to be outputted to log files.
Validation performed
The following are the manual tests performed:
application/__init__.pysuccessfully executes and the root logger was able to add the TimedRotatingFileHandler into its list of handlers.application/__init__.pysuccessfully logs the message into a log file.Additional Comments
/, however windows paths normally prefer\\. This is something to pay attention to but I've already tested it on my machine(windows OS) and it works correctly.Summary by CodeRabbit
New Features
Improvements