Skip to content

CLPFileHandler integration for iCtrl's logging framework #49

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

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

CLPFileHandler integration for iCtrl's logging framework #49

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
625f42f
Log exception with exc_info in custom Flask exception handler.
xx12345798 Jul 25, 2024
190dfe0
Log response in custom Flask after request handler
xx12345798 Sep 8, 2024
4f35129
addressed the comments of using response.get_data instead of response…
xx12345798 Sep 15, 2024
392a426
use jsonify to replace response dumped by json.dump
xx12345798 Sep 27, 2024
f937db9
Merge branch 'junhaoliao:main' into flask-logging
xx12345798 Sep 27, 2024
4efa440
remove the unused json import and revert the response of type applica…
xx12345798 Jan 5, 2025
abf9d30
fixed missing %s
xx12345798 Jan 5, 2025
2bcb814
merge
xx12345798 Jan 5, 2025
ada79e7
add missing %s into the log message of file application/routes/vnc.py…
xx12345798 Jan 5, 2025
5c60c87
Merge branch 'main' of https://github.com/junhaoliao/iCtrl into CLPFi…
xx12345798 Apr 4, 2025
f593d9e
adding CLPFileHandler into iCtrl as log handlers, also updated the re…
xx12345798 Apr 4, 2025
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
10 changes: 10 additions & 0 deletions application/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,22 @@
from flask import Flask, Blueprint, jsonify
from werkzeug.exceptions import HTTPException
from werkzeug.serving import WSGIRequestHandler
from clp_logging.handlers import CLPFileHandler
from datetime import datetime
Comment on lines +28 to +29
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix imported but unused CLPFileHandler.

The CLPFileHandler is imported but never directly used in the code. Either remove the unused import or use it directly in the code.

-from clp_logging.handlers import CLPFileHandler
 from datetime import datetime
📝 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.

Suggested change
from clp_logging.handlers import CLPFileHandler
from datetime import datetime
from datetime import datetime
🧰 Tools
🪛 Ruff (0.8.2)

28-28: clp_logging.handlers.CLPFileHandler imported but unused

(F401)


logger = logging.getLogger(__name__)

try:
with open('log_config.yaml') as config_file:
config = yaml.safe_load(config_file.read())
if 'handlers' in config and 'CLP_file' in config['handlers']:
logDir = "logs"
os.makedirs(logDir, exist_ok=True)

currentDateTime = datetime.now().strftime('%Y-%m-%d_%H-%M-%S')
filename = f"{currentDateTime}.clp.zst"
log_path = os.path.join(logDir, filename)
Comment on lines +40 to +42
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Improve log filename format.

Consider adding a prefix to the log filename for better identification, especially if this application is part of a larger system with multiple logging components.

     currentDateTime = datetime.now().strftime('%Y-%m-%d_%H-%M-%S')
-    filename = f"{currentDateTime}.clp.zst"
+    filename = f"ictrl_{currentDateTime}.clp.zst"
     log_path = os.path.join(logDir, filename)
📝 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.

Suggested change
currentDateTime = datetime.now().strftime('%Y-%m-%d_%H-%M-%S')
filename = f"{currentDateTime}.clp.zst"
log_path = os.path.join(logDir, filename)
currentDateTime = datetime.now().strftime('%Y-%m-%d_%H-%M-%S')
- filename = f"{currentDateTime}.clp.zst"
+ filename = f"ictrl_{currentDateTime}.clp.zst"
log_path = os.path.join(logDir, filename)
🧰 Tools
🪛 Ruff (0.8.2)

40-40: datetime.datetime.now() called without a tz argument

(DTZ005)

config['handlers']['CLP_file']['fpath'] = log_path
Comment on lines +36 to +43
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add timezone awareness to datetime operations.

The code uses datetime.now() without a timezone parameter, which could lead to inconsistencies in environments with different timezone settings. Consider using datetime.now(timezone.utc) for consistent timestamps.

-    currentDateTime = datetime.now().strftime('%Y-%m-%d_%H-%M-%S')
+    from datetime import timezone
+    currentDateTime = datetime.now(timezone.utc).strftime('%Y-%m-%d_%H-%M-%S')
     filename = f"{currentDateTime}.clp.zst"

Also, consider adding error handling for the case where log directory creation fails:

     logDir = "logs"
-    os.makedirs(logDir, exist_ok=True)
+    try:
+        os.makedirs(logDir, exist_ok=True)
+    except OSError as e:
+        logger.warning(f"Failed to create logs directory: {e}")
+        # Use current directory as fallback
+        logDir = "."
📝 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.

Suggested change
if 'handlers' in config and 'CLP_file' in config['handlers']:
logDir = "logs"
os.makedirs(logDir, exist_ok=True)
currentDateTime = datetime.now().strftime('%Y-%m-%d_%H-%M-%S')
filename = f"{currentDateTime}.clp.zst"
log_path = os.path.join(logDir, filename)
config['handlers']['CLP_file']['fpath'] = log_path
if 'handlers' in config and 'CLP_file' in config['handlers']:
logDir = "logs"
try:
os.makedirs(logDir, exist_ok=True)
except OSError as e:
logger.warning(f"Failed to create logs directory: {e}")
# Use current directory as fallback
logDir = "."
from datetime import timezone
currentDateTime = datetime.now(timezone.utc).strftime('%Y-%m-%d_%H-%M-%S')
filename = f"{currentDateTime}.clp.zst"
log_path = os.path.join(logDir, filename)
config['handlers']['CLP_file']['fpath'] = log_path
🧰 Tools
🪛 Ruff (0.8.2)

40-40: datetime.datetime.now() called without a tz argument

(DTZ005)

logging.config.dictConfig(config)
except Exception:
# Fallback to a basic configuration
Expand Down
9 changes: 4 additions & 5 deletions log_config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,11 @@ formatters:
datefmt: '%Y-%m-%d %H:%M:%S'

handlers:
console:
class: logging.StreamHandler
CLP_file:
class: clp_logging.handlers.CLPFileHandler
level: DEBUG
formatter: default
stream: ext://sys.stderr
fpath: 'example.clp.zst'

Comment on lines +10 to 14
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Handler configuration needs a more descriptive file path.

The fpath: 'example.clp.zst' appears to be a placeholder value that will be overridden at runtime. Consider using a more descriptive default value or adding a comment to indicate this is a placeholder.

    CLP_file:
        class: clp_logging.handlers.CLPFileHandler
        level: DEBUG
-        fpath: 'example.clp.zst'
+        fpath: 'default.clp.zst'  # This will be dynamically set at runtime
📝 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.

Suggested change
CLP_file:
class: clp_logging.handlers.CLPFileHandler
level: DEBUG
formatter: default
stream: ext://sys.stderr
fpath: 'example.clp.zst'
CLP_file:
class: clp_logging.handlers.CLPFileHandler
level: DEBUG
fpath: 'default.clp.zst' # This will be dynamically set at runtime

root:
level: DEBUG
handlers: [console]
handlers: [CLP_file]
1 change: 1 addition & 0 deletions requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,5 @@ zipstream-new==1.1.8
paramiko==3.0.0
numpy==1.24.2
pyyaml==6.0.1
clp-logging==0.0.14
git+https://github.com/junhaoliao/simple-websocket-server#egg=SimpleWebSocketServer
Loading