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
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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.
i think there can be some pitfalls in the derivation, though the difference likely won't be more than 50%.
each character is 2 bytes according to the UTF standard, which by default is 2 bytes each.
which UTF standard are we referring to? if it's UTF-16, where is it configured and why we want to configure so, given iCtrl only outputs English logs?
%(name)s: Assume the longest Python file name to be 20 characters
20 characters are still too small. Even with rigid standards such as MISRA 2012, identifiers names are regulated to have max 31 characters. I think we can make worst case assumptions. On most file systems, max file name length is 255 characters.
%(message)s: Assume a very detailed log message that has 200 characters
this is fine. people often use 140 which is an old limit of Tweets.
The original comment meant to say the derivation should be somewhere documented publicly. If you think it can be too many comments to add in the YAML file, at least we want to show a link of this thread in a comment before the number.
There was a problem hiding this comment.
which UTF standard are we referring to? if it's UTF-16, where is it configured and why we want to configure so, given iCtrl only outputs English logs?
Thank you for raising this up. My calculation was an estimation of the worst case so I wanted to show that it wouldn't be much of a performance issue even for the worst case. In our case for iCtrl, I believe we would only use UTF-8 because we only log in English letters, so we can proceed with the calculation of 1 byte/character.
20 characters are still too small. Even with rigid standards such as MISRA 2012, identifiers names are regulated to have max 31 characters. I think we can make worst case assumptions. On most file systems, max file name length is 255 characters.
Yes on most file system the maximum file name length is 255 characters. I believe the reason for that assumption was due to the naming convention for our logs, and I was also assuming no one would change the names manually. We can proceed with assuming 255 characters / file name in the worst case.
this is fine. people often use 140 which is an old limit of Tweets.
Surely, however I just found from Log and event storage best practices that event log entries usually average around 200 bytes, so I think it is reasonable to assume every log message to be around 200 characters.
The original comment meant to say the derivation should be somewhere documented publicly. If you think it can be too many comments to add in the YAML file, at least we want to show a link of this thread in a comment before the number.
Yea I think adding a link to the yaml file would be better. I will go ahead and do it.
The following is a recalculation based on the above items:
number of characters in a message * number of bytes/character * number of messages logged / second * 3 hours * 112 rotation periods(2 weeks)
= (23 + 8 + 255 + 5 + 200 + 8) * 1 * 5 * (60 * 60 * 3) * 112
= 2.81 GB
Still far from breaking the constraint requirement.
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)
| logging.config.dictConfig(config) | ||
| except Exception: | ||
| # Fallback to a basic configuration | ||
| logging.basicConfig(format='%(asctime)s %(levelname)s [%(name)s:%(lineno)d] %(message)s', level=logging.INFO, force=True) | ||
|
|
||
| logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
Can we keep Line 29 and remove this?
There was a problem hiding this comment.
The reasoning for this was explained in PR#48, but I forgot I also made the changes here since these changes were not merged. I can collapse the other PR onto this.
application/__init__.py
Outdated
| try: | ||
| with open('log_config.yaml') as config_file: | ||
| config = yaml.safe_load(config_file.read()) | ||
|
|
||
| config['handlers']['timedRotatingFile']['filename'] = paths.LOG_FILE_PATH | ||
| paths.makedir_if_not_exists(paths.LOGS_DIR_PATH) |
There was a problem hiding this comment.
can you explain how this works?
- shouldn't we create the file before configuring logging?
- does makedir_if_not_exists work for files?
There was a problem hiding this comment.
There was a problem hiding this comment.
Answer to question 1:
Per Python documentation for TimedRotatingHandler under logging handlers, it says this upon returning a new instance of the TimedRotatingFileHandler class
The specified file is opened and used as the stream for logging.
Although not explicitly mentioning that the specified file is created, it does perform file creation if the file does not exist already, and I've tested it on my Windows machine. Unless you think it is safe to create the file first before configuring logging, I do not think this is a concern.
Answer to question 2:
Please note that paths.LOGS_DIR_PATH is not the same as paths.LOG_FILE_PATH, the first specifies the directory that stores the logs, which I wrote the code to create the directory if it does not exist already to prevent any issues or errors for loggin
Explanation for how this works:
paths.LOGS_DIR_PATH and paths.LOG_FILE_PATH are two FINAL variables(not explicitly annotated or commented, but they shouldn't be changed during runtime in any case) declared and assigned in paths.py.
config['handlers']['timedRotatingFile']['filename'] = paths.LOG_FILE_PATH: this is to set the current log's file name as well as the file prefix for older log files. Upon file rotation, the naming of the log files will show the behaviour per Python's documentation on TimedRotatingFileHandler as follows:
The system will save old log files by appending extensions to the filename. The extensions are date-and-time based, using the strftime format %Y-%m-%d_%H-%M-%S or a leading portion thereof, depending on the rollover interval.
Here is a screenshot of some sample logs that were produced during the testing of the TimedRotatingFileHandler, where ictrl_log is the current log:
paths.makedir_if_not_exists(paths.LOGS_DIR_PATH): Already explained
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
log_config.yaml (1)
2-2: 🛠️ Refactor suggestionConsider setting
disable_existing_loggersto False.According to the PR objectives, this value should be False to prevent disabling the logger initialized in
application/__init__.py. The current value of True may cause problems with existing loggers.-disable_existing_loggers: True +disable_existing_loggers: False
♻️ Duplicate comments (2)
application/paths.py (1)
42-44: 🧹 Nitpick (assertive)LGTM! Consider adding docstrings.
The path construction is correct and follows the existing pattern. Consider adding docstrings to document these paths, especially since they're used across multiple files.
+# Directory for storing log files, created during logging initialization LOGS_DIR_PATH = os.path.join(PROFILE_PATH, 'logs') makedir_if_not_exists(LOGS_DIR_PATH) +# Main log file path, used by TimedRotatingFileHandler LOG_FILE_PATH = os.path.join(LOGS_DIR_PATH, 'ictrl_log.log')log_config.yaml (1)
15-23: 🧹 Nitpick (assertive)Document temporary nature of this logging solution.
Since this is a temporary solution pending the custom CLP rotating handler (PR #43), consider adding a TODO comment indicating this is temporary.
timedRotatingFile: + # TODO: Temporary solution using TimedRotatingFileHandler, to be replaced with custom CLP rotating handler (see PR #43) class: logging.handlers.TimedRotatingFileHandler level: DEBUG formatter: default when: H interval: 3 # See discussion on backupCount decision: https://github.com/junhaoliao/iCtrl/pull/46/files/0c148d0e47dc9ae42014f10a4a04e0aaffca7cbd#r1921487166 backupCount: 112 # filename: To be dynamically assigned upon application initialization.
📜 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)
🔇 Additional comments (3)
log_config.yaml (2)
19-22: LGTM! Rotation settings look appropriate.The 3-hour rotation period and 112 backup count (approximately 2 weeks of logs) are well-justified based on the detailed discussion in the PR comments. The link to the discussion is also helpful for future reference.
27-27: LGTM! Root handler configuration is correct.The root logger now correctly includes both the console and timedRotatingFile handlers, which ensures logs are directed to both outputs.
application/__init__.py (1)
34-34: Set the filename for the timedRotatingFile handler.The code correctly sets the filename for the timedRotatingFile handler using the path defined in paths.py, which ensures logs are stored in the appropriate location.
🧰 Tools
🪛 Ruff (0.8.2)
34-34: Undefined name
paths(F821)
| logging.config.dictConfig(config) | ||
| except Exception: | ||
| # Fallback to a basic configuration | ||
| logging.basicConfig(format='%(asctime)s %(levelname)s [%(name)s:%(lineno)d] %(message)s', level=logging.INFO, force=True) | ||
|
|
||
| logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider initializing the logger only once.
The logger is initialized in both the exception and else blocks. Consider initializing it once before the try block and reusing it to reduce duplication.
+logger = logging.getLogger(__name__)
try:
with open('log_config.yaml') as config_file:
config = yaml.safe_load(config_file.read())
config['handlers']['timedRotatingFile']['filename'] = paths.LOG_FILE_PATH
logging.config.dictConfig(config)
except Exception:
# Fallback to a basic configuration
logging.basicConfig(format='%(asctime)s %(levelname)s [%(name)s:%(lineno)d] %(message)s', level=logging.INFO, force=True)
- logger = logging.getLogger(__name__)
logger.exception("Logging setup failed")
else:
- logger = logging.getLogger(__name__)
logger.warning("Logging setup is completed with config=%s", config)Also applies to: 43-43
| @@ -21,22 +21,26 @@ | |||
| import os | |||
| import sys | |||
|
|
|||
| import application.paths | |||
There was a problem hiding this comment.
Fix the import statement to match usage.
The code imports application.paths but later uses paths directly, which will cause an undefined name error.
-import application.paths
+import application.paths as paths📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import application.paths | |
| import application.paths as paths |
🧰 Tools
🪛 Ruff (0.8.2)
24-24: application.paths imported 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
Summary by CodeRabbit
New Features
Improvements