feat : Centralized Logging System #548
Conversation
- Created logging configuration file (logging_config.json) for centralized settings. - Developed core logging module (core.py) with color-coded log levels and formatting. - Implemented component-specific logging setup for backend and sync-microservice. - Introduced ColorFormatter for enhanced log message readability. - Added support for environment-specific logging configurations (development and production). - Created test modules to validate logging functionality across components. - Integrated Uvicorn logging with custom formatting and color coding. - Established a new logging structure to facilitate easy addition of new components.
* Checkpoint from VS Code for coding agent session * Refactor logging system: replace custom logging implementation with Loguru, update folder utilities for cleaner function definitions, and remove obsolete test files. * Refactor lifespan logging: replace print statements with logger calls for improved logging consistency
… across the codebase
…print statements for error and debug messages
…y formatting long lines
WalkthroughAdds centralized, JSON-configurable logging packages for backend and sync-microservice, replaces many print() calls with structured logging, adds stdout/stderr redirection and log-streaming threads, introduces a watcher helper, and updates requirements. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Setup as setup_logging(component)
participant Config as logging_config.json
participant Root as Root Logger
participant Formatter as ColorFormatter+ConsoleHandler
participant Intercept as InterceptHandler
participant Uvicorn as Uvicorn/Asyncio
App->>Setup: setup_logging(component)
Setup->>Config: load_config()
Config-->>Setup: component & env settings
Setup->>Root: configure root logger & handlers
Setup->>Formatter: attach ColorFormatter
Setup->>Intercept: create InterceptHandler
Setup->>Uvicorn: configure_uvicorn_logging(component)
rect rgba(150,200,150,0.12)
Note over App,Root: Runtime logging (app modules)
App->>Root: logger.info/debug/error(...)
Root->>Formatter: emit(LogRecord)
Formatter-->>App: formatted colored output
end
rect rgba(200,170,220,0.12)
Note over Uvicorn,Intercept: External log interception
Uvicorn->>Intercept: external LogRecord
Intercept->>Root: route via project logger
Root->>Formatter: format & output
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
utils/logging/logging_config.json (1)
38-38: Add newline at end of file.The file is missing a trailing newline. While not a functional issue, many tools and style guides expect files to end with a newline character.
Apply this diff:
"default_environment": "development" -} +} +backend/main.py (2)
91-91: Prefer Path.joinpath or use parentheses for multi-line clarity.The single-line
os.path.joinwith multiple arguments is readable but consider usingpathlib.Pathfor consistency with modern Python practices, or breaking across lines if the project style guide requires it.Example refactor using
pathlib.Path:+from pathlib import Path ... - project_root = os.path.abspath(os.path.join(os.path.dirname(__file__), "..")) - openapi_path = os.path.join(project_root, "docs", "backend", "backend_python", "openapi.json") + project_root = Path(__file__).parent.parent.resolve() + openapi_path = project_root / "docs" / "backend" / "backend_python" / "openapi.json"
128-129: Logger re-initialization in__main__is redundant.The logger is already initialized at module level (line 75). Re-initializing it here serves no purpose since
get_logger(__name__)returns the same logger instance with the same handlers. Remove the redundant call.Apply this diff:
if __name__ == "__main__": multiprocessing.freeze_support() # Required for Windows - logger = get_logger(__name__) logger.info("Starting PictoPy backend server...")sync-microservice/main.py (1)
45-47: Logger re-initialization in__main__is redundant.The logger is already initialized at line 17. Re-initializing here is unnecessary and creates confusion about which logger instance is active.
Apply this diff:
if __name__ == "__main__": - # Get a logger for this module - logger = get_sync_logger(__name__) logger.info("Starting PictoPy Sync Microservice...")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
backend/app/database/face_clusters.py(1 hunks)backend/app/database/faces.py(2 hunks)backend/app/database/folders.py(1 hunks)backend/app/database/images.py(7 hunks)backend/app/database/yolo_mapping.py(0 hunks)backend/app/logging/__init__.py(1 hunks)backend/app/logging/setup_logging.py(1 hunks)backend/app/models/FaceDetector.py(2 hunks)backend/app/models/FaceNet.py(2 hunks)backend/app/models/ObjectClassifier.py(2 hunks)backend/app/models/YOLO.py(2 hunks)backend/app/routes/albums.py(1 hunks)backend/app/routes/folders.py(6 hunks)backend/app/utils/face_clusters.py(8 hunks)backend/app/utils/folders.py(2 hunks)backend/app/utils/image_metatdata.py(4 hunks)backend/app/utils/images.py(8 hunks)backend/app/utils/microservice.py(4 hunks)backend/main.py(4 hunks)sync-microservice/app/core/lifespan.py(2 hunks)sync-microservice/app/database/folders.py(3 hunks)sync-microservice/app/logging/__init__.py(1 hunks)sync-microservice/app/logging/setup_logging.py(1 hunks)sync-microservice/app/utils/logger_writer.py(1 hunks)sync-microservice/app/utils/watcher.py(15 hunks)sync-microservice/main.py(2 hunks)sync-microservice/requirements.txt(1 hunks)utils/logging/__init__.py(1 hunks)utils/logging/logging_config.json(1 hunks)
💤 Files with no reviewable changes (1)
- backend/app/database/yolo_mapping.py
🧰 Additional context used
🧬 Code graph analysis (18)
backend/app/routes/folders.py (3)
backend/app/logging/setup_logging.py (2)
setup_logging(134-189)get_logger(192-202)backend/app/utils/images.py (1)
image_util_process_folder_images(23-75)backend/app/utils/API.py (1)
API_util_restart_sync_microservice_watcher(8-33)
sync-microservice/app/logging/__init__.py (1)
sync-microservice/app/logging/setup_logging.py (3)
setup_logging(134-189)get_sync_logger(205-217)configure_uvicorn_logging(261-300)
backend/app/models/FaceNet.py (1)
backend/app/logging/setup_logging.py (2)
setup_logging(134-189)get_logger(192-202)
backend/main.py (1)
backend/app/logging/setup_logging.py (3)
setup_logging(134-189)configure_uvicorn_logging(246-285)get_logger(192-202)
backend/app/models/FaceDetector.py (1)
backend/app/logging/setup_logging.py (2)
setup_logging(134-189)get_logger(192-202)
backend/app/logging/__init__.py (1)
backend/app/logging/setup_logging.py (3)
setup_logging(134-189)get_logger(192-202)configure_uvicorn_logging(246-285)
backend/app/utils/face_clusters.py (1)
backend/app/logging/setup_logging.py (2)
setup_logging(134-189)get_logger(192-202)
sync-microservice/app/database/folders.py (1)
sync-microservice/app/logging/setup_logging.py (1)
get_sync_logger(205-217)
sync-microservice/app/utils/watcher.py (2)
sync-microservice/app/logging/setup_logging.py (2)
setup_logging(134-189)get_sync_logger(205-217)sync-microservice/app/database/folders.py (1)
db_get_all_folders_with_ids(22-44)
utils/logging/__init__.py (1)
backend/app/logging/setup_logging.py (3)
setup_logging(134-189)get_logger(192-202)configure_uvicorn_logging(246-285)
backend/app/logging/setup_logging.py (1)
sync-microservice/app/logging/setup_logging.py (8)
ColorFormatter(16-110)format(63-110)load_config(113-131)setup_logging(134-189)get_logger(192-202)InterceptHandler(220-258)emit(239-258)configure_uvicorn_logging(261-300)
backend/app/utils/folders.py (3)
backend/app/database/folders.py (2)
db_update_parent_ids_for_subtree(208-231)db_delete_folders_batch(144-176)backend/app/schemas/folders.py (1)
ErrorResponse(100-103)backend/app/logging/setup_logging.py (2)
setup_logging(134-189)get_logger(192-202)
sync-microservice/app/core/lifespan.py (3)
sync-microservice/app/logging/setup_logging.py (2)
setup_logging(134-189)get_sync_logger(205-217)sync-microservice/app/database/folders.py (1)
db_check_database_connection(47-71)sync-microservice/app/utils/watcher.py (2)
watcher_util_start_folder_watcher(221-276)watcher_util_stop_folder_watcher(279-307)
backend/app/utils/images.py (2)
backend/app/logging/setup_logging.py (2)
setup_logging(134-189)get_logger(192-202)backend/app/database/images.py (1)
db_delete_images_by_ids(316-348)
sync-microservice/app/logging/setup_logging.py (1)
backend/app/logging/setup_logging.py (8)
ColorFormatter(16-110)format(63-110)load_config(113-131)setup_logging(134-189)get_logger(192-202)InterceptHandler(205-243)emit(224-243)configure_uvicorn_logging(246-285)
backend/app/utils/image_metatdata.py (1)
backend/app/logging/setup_logging.py (2)
setup_logging(134-189)get_logger(192-202)
sync-microservice/main.py (3)
sync-microservice/app/core/lifespan.py (1)
lifespan(19-57)sync-microservice/app/logging/setup_logging.py (3)
setup_logging(134-189)get_sync_logger(205-217)configure_uvicorn_logging(261-300)sync-microservice/app/utils/logger_writer.py (1)
LoggerWriter(12-47)
backend/app/utils/microservice.py (1)
backend/app/logging/setup_logging.py (2)
setup_logging(134-189)get_logger(192-202)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Backend Tests
🔇 Additional comments (75)
backend/app/database/folders.py (1)
385-387: LGTM! Clean type annotation formatting.The multi-line return type annotation improves readability for this complex tuple type without changing behavior.
backend/app/database/face_clusters.py (1)
198-200: LGTM! Consistent formatting improvement.The multi-line return type annotation improves readability and aligns with the formatting changes in related database modules.
backend/app/database/faces.py (2)
235-237: LGTM! Clean type annotation formatting.The multi-line return type annotation improves readability for this complex type without changing behavior.
271-272: LGTM! Good trailing comma practice.The trailing comma in the parameter list follows Python best practices—it makes future parameter additions cleaner and reduces diff noise.
backend/app/routes/albums.py (1)
32-32: Whitespace tweak looks good.Consistent spacing helps readability; no issues here.
backend/app/models/ObjectClassifier.py (1)
4-23: Logging integration looks solid.Swapping prints for the centralized logger keeps behavior intact while aligning with the new logging system.
sync-microservice/requirements.txt (1)
38-39: Dependency update aligns with logging changes.Adding
loguru==0.7.3here keeps the sync service in sync with the new centralized logging stack.backend/app/models/YOLO.py (2)
12-14: LGTM! Clean logger integration.The import and initialization of the module-level logger follows the centralized logging pattern consistently applied across the codebase.
36-36: LGTM! Appropriate log level for lifecycle event.Replacing the print statement with
logger.infois correct for logging the YOLO model session closure, which is a normal lifecycle event rather than an error or warning.backend/app/utils/folders.py (2)
10-13: LGTM! Proper imports for error handling and logging.The
ErrorResponseimport and logger initialization are both used appropriately in the file—ErrorResponsefor HTTPException details (lines 88-92, 98-101) and the logger for exception logging (line 175).
175-175: LGTM! Appropriate error logging with continuation strategy.Using
logger.errorto log the exception while continuing to process other folders is a reasonable fault-tolerance strategy for batch operations.backend/app/models/FaceNet.py (2)
6-9: LGTM! Consistent logger setup.The logger initialization follows the same pattern as other model files (e.g., YOLO.py). The inline comment is helpful, though optional since the pattern is self-documenting.
29-29: LGTM! Correct log level for cleanup operation.INFO level is appropriate for logging the FaceNet model session closure, consistent with similar lifecycle events in other model classes.
sync-microservice/app/database/folders.py (3)
4-6: LGTM! Correct logger for sync-microservice component.Using
get_sync_loggerinstead ofget_loggerensures logs are properly prefixed with[SYNC-MICROSERVICE]as per the centralized logging configuration.
41-41: LGTM! Appropriate error handling with graceful degradation.Logging the exception with
logger.errorand returning an empty list provides graceful degradation for database query failures.
70-70: LGTM! Consistent error handling pattern.The error logging and
Falsereturn value provide clear feedback on database connection failures while maintaining a predictable API contract.backend/app/utils/image_metatdata.py (4)
6-8: LGTM! Standard logger initialization.The logger setup follows the centralized logging pattern consistently applied across backend modules.
49-51: LGTM! Appropriate log level for non-critical failure.Using
logger.warningfor EXIF extraction failures is correct—the function continues to extract other metadata, so this is a partial failure rather than a critical error.
54-54: LGTM! Explicit re-raise improves clarity.The comment clarifies the intent to propagate
FileNotFoundErrorwhile catching other exceptions for graceful degradation—a good practice for exception handling documentation.
66-68: LGTM! Consistent warning-level logging for metadata retrieval failures.Both file size and creation date retrieval failures are correctly logged at WARNING level, maintaining consistency with the EXIF extraction error handling pattern and allowing partial metadata extraction to succeed.
Also applies to: 77-79
backend/app/database/images.py (7)
9-12: LGTM!Logger import and initialization follow the standard pattern established across the codebase.
90-90: LGTM!Appropriate use of
logger.errorfor database insertion failures.
166-166: LGTM!Proper error logging for database query failures.
238-238: LGTM!Consistent error logging for database update failures.
272-272: LGTM!Appropriate error logging for batch insertion failures.
310-310: LGTM!Proper error logging with graceful degradation (returns empty list).
341-344: LGTM!Appropriate use of
logger.infofor successful operations andlogger.errorfor failures. Good observability for tracking deletions.backend/app/models/FaceDetector.py (4)
9-12: LGTM!Standard logger initialization pattern.
24-24: LGTM!Appropriate use of
logger.infofor initialization confirmation.
29-29: LGTM!Proper error logging for image loading failures.
33-34: LGTM!Excellent use of log levels:
debugfor detailed box coordinates andinfofor aggregate face count. This provides good observability at different verbosity levels.backend/app/utils/face_clusters.py (7)
24-27: LGTM!Standard logger initialization pattern.
167-181: LGTM!Appropriate use of
logger.infofor clustering operation summaries. Provides good visibility into the clustering process.
343-343: LGTM!Proper error logging for database update failures.
394-394: LGTM!Appropriate error logging for face data retrieval failures.
494-494: LGTM!Proper error logging for image processing failures.
513-513: LGTM!Appropriate error logging for encoding failures.
552-552: LGTM!Proper error logging for face image generation failures.
backend/app/utils/images.py (9)
18-20: LGTM!Standard logger initialization pattern.
61-74: LGTM!Appropriate error logging with proper error recovery (continues processing remaining folders on individual failures).
91-96: LGTM!Appropriate error logging. The trailing comma on Line 96 is a style improvement that makes future diffs cleaner.
113-113: LGTM!Appropriate use of
logger.debugfor detailed operational data (image-class pairs).
201-201: LGTM!Proper error logging for folder reading failures.
221-221: LGTM!Appropriate error logging for thumbnail generation failures.
245-247: LGTM!Good use of logging levels:
infofor successful thumbnail removal (audit trail) anderrorfor failures.
251-251: LGTM!Appropriate use of
logger.infofor operational summary (obsolete images removed).
257-257: LGTM!Trailing comma is a style improvement that simplifies future diffs.
utils/logging/__init__.py (1)
1-9: LGTM! Clean package initialization.The package initialization correctly re-exports the three core logging functions and defines
__all__to control the public API surface. The structure follows Python packaging best practices.sync-microservice/app/logging/__init__.py (1)
1-9: LGTM! Sync-microservice logging package correctly initialized.The package exposes
get_sync_logger,configure_uvicorn_logging, andsetup_loggingwith proper__all__declaration. The use ofget_sync_logger(instead of genericget_logger) aligns with the component-specific logging pattern described in the PR objectives.backend/app/routes/folders.py (4)
14-14: LGTM! Logger properly initialized.The logger is correctly imported and initialized with
__name__, following standard Python logging practices. This ensures log messages will be tagged with the module name for traceability.Also applies to: 48-49
69-69: LGTM! Logging integrated into post-addition sequence.The logging calls correctly capture folder addition events at info level and exceptions at error level. The messages include sufficient context for troubleshooting.
Also applies to: 77-79
94-94: LGTM! Exception logging added.The error log correctly captures exceptions during AI tagging enablement with descriptive context.
116-116: LGTM! Sync sequence logging integrated.The logging calls appropriately capture folder synchronization events at info level and exceptions at error level with sufficient diagnostic context.
Also applies to: 125-127
sync-microservice/app/utils/watcher.py (11)
9-11: LGTM! Logger initialized with sync-specific function.The watcher correctly uses
get_sync_logger(__name__)to ensure logs are properly prefixed with the sync-microservice component tag, aligning with the centralized logging strategy.
53-53: LGTM! File change handling logging integrated.The logger calls appropriately capture file change events, folder deletion, parent folder identification, and edge cases at info level with clear diagnostic context.
Also applies to: 58-60, 69-69, 73-73
77-77: LGTM! Delete API call logged.The info log correctly captures the delete API invocation with folder count.
113-113: LGTM! Debug logging for folder matching.Debug level is appropriate for internal tracing of the parent folder matching logic.
134-136: LGTM! Sync folder API logging integrated.The logging correctly captures success at info level and various failure modes at error level, with sufficient context for troubleshooting (status codes, error messages, folder details).
Also applies to: 138-140, 143-143, 145-145
163-163: LGTM! Delete folders API logging integrated.The logging mirrors the sync API pattern with appropriate levels (info for success, error for failures) and comprehensive diagnostic context.
Also applies to: 165-167, 170-170, 172-172
183-183: LGTM! Watcher worker lifecycle logging.The logging appropriately tracks watcher thread lifecycle events (start, stop, errors) with clear messages for operational monitoring.
Also applies to: 186-186, 190-190, 192-192
212-212: LGTM! Warning for missing folders.Warning level is appropriate for folders that don't exist in the filesystem but are in the database, as this is a recoverable condition that should be noted.
231-231: LGTM! Comprehensive startup logging.The logging captures all startup states (already running, initialization, database query results, folder discovery, thread start, success/failure) at appropriate levels with sufficient detail for operational monitoring.
Also applies to: 234-234, 240-240, 243-243, 248-248, 258-260, 271-271, 275-275
284-284: LGTM! Stop sequence logging integrated.The logging appropriately captures the stop sequence states, including the graceful shutdown timeout scenario at warning level.
Also applies to: 288-288, 297-297, 299-299, 302-302
317-317: LGTM! Restart and utility function logging complete.The logging for restart operations, wait functions, and example main flow is consistent with the rest of the module, using appropriate levels and clear messages.
Also applies to: 344-344, 347-347, 353-353, 357-357, 361-361, 365-365, 367-367
sync-microservice/app/utils/logger_writer.py (3)
1-10: LGTM! Clear module documentation and imports.The module docstring clearly explains the purpose of redirecting stdout/stderr to the logging system. Import is minimal and correct.
12-41: LGTM! LoggerWriter correctly implements stream redirection.The class properly buffers partial writes and logs complete lines when newlines are encountered. The logic correctly handles:
- Buffering across multiple write calls
- Stripping trailing newlines before logging
- Skipping empty lines
This is a standard and robust pattern for redirecting stdout/stderr to logging.
43-47: LGTM! Flush method ensures no output is lost.The flush method correctly handles any remaining buffered content, ensuring incomplete lines are logged when the stream is closed or flushed.
backend/main.py (3)
30-36: LGTM! Logging setup at module load is correct.The setup order is appropriate:
setup_loggingconfigures the root logger and handlers, thenconfigure_uvicorn_loggingwires Uvicorn's loggers into the custom format. This ensures all subsequent logging calls use the centralized configuration.
74-76: Module-level logger initialization looks good.Creating the logger after setup ensures it inherits the configured handlers and formatters.
137-137: Logging configuration confirmed.configure_uvicorn_loggingis invoked beforeuvicorn.run, clearing default handlers for “uvicorn”, “uvicorn.error”, and “uvicorn.access”, whilelog_config=Nonedisables Uvicorn’s built-in logging. All access and error logs flow exclusively through ourInterceptHandlerwith no duplication or loss.sync-microservice/app/core/lifespan.py (2)
10-12: LGTM! Logger initialization is correct.Using
get_sync_logger(__name__)ensures the logger is component-scoped with the[SYNC-MICROSERVICE]prefix.
27-57: Logging replacement looks good.All startup, error, and shutdown messages are properly routed through the logger with appropriate levels (info for status, error for failures). The exception handling and shutdown flow maintain correct semantics.
backend/app/logging/__init__.py (1)
1-9: LGTM! Package initialization is correct.The
__init__.pyproperly re-exports the public API with__all__for clean imports. The docstring clarifies the package purpose.backend/app/utils/microservice.py (1)
48-51: Enable cross-platform ANSI support viacoloramaor terminal feature detection. Python’s subprocess will pass your ANSI codes unchanged, but legacy cmd.exe/PowerShell only renders VT sequences if you enable virtual-terminal processing (SetConsoleMode/registry), and many programs disable color when stdout isn’t a TTY. Callcolorama.init()(or wrap output withcolorama.AnsiToWin32), or checksys.stdout.isatty()to disable colors when unsupported.sync-microservice/main.py (1)
55-55: Logging setup validated:log_config=Nonebypasses Uvicorn’s built-in logging config, andconfigure_uvicorn_logging("sync-microservice")clears existing handlers and attaches a singleInterceptHandler(withpropagate=False) touvicorn,uvicorn.error, anduvicorn.access, ensuring all logs flow through your custom formatter without duplicates or gaps.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/utils/microservice.py (1)
303-303: Remove debug print statement.This
logger.debug(f"Python executable: {python_executable}").- print(python_executable) + logger.debug(f"Python executable: {python_executable}")
♻️ Duplicate comments (1)
backend/app/utils/microservice.py (1)
125-129: Use a list forcmdto ensure Windows compatibility.Passing a string to
subprocess.Popenwithoutshell=Truecan fail on Windows. Changecmdto a list to ensure consistent cross-platform behavior.- cmd = str(sync_executable) # Correct the command to use the actual path + cmd = [str(sync_executable)] logger.info(f"Starting sync microservice with command: {cmd}") process = subprocess.Popen(
🧹 Nitpick comments (2)
backend/app/utils/microservice.py (2)
74-78: Remove unused color constants.
MAGENTAandRESETare defined but never used in this file. Consider removing them to reduce clutter, or document their intended purpose if they're for future use.CYAN = "\033[96m" RED = "\033[91m" -MAGENTA = "\033[95m" -RESET = "\033[0m"
80-89: Remove unused parameters from stream_logs.The
prefixandcolorparameters are never used in the function body. Since the sync-microservice already formats its logs, these parameters are unnecessary. Based on learnings.-def stream_logs(pipe, prefix, color): +def stream_logs(pipe): """Read a process pipe and print formatted logs from sync-microservice.""" for line in iter(pipe.readline, ""): if line: # Trim any trailing newlines line = line.strip() if line: # All output from sync-microservice is now properly formatted by its logger print(line) pipe.close()Then update all callers to remove the unused arguments:
- Lines 140, 147:
args=(process.stdout,)andargs=(process.stderr,)- Lines 340, 345: same pattern
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/app/utils/microservice.py(4 hunks)backend/main.py(4 hunks)sync-microservice/app/utils/logger_writer.py(1 hunks)sync-microservice/main.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/main.py
- sync-microservice/main.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Hemil36
PR: AOSSIE-Org/PictoPy#548
File: backend/app/utils/microservice.py:53-62
Timestamp: 2025-10-01T20:44:19.127Z
Learning: In backend/app/utils/microservice.py, the stream_logs function uses print() instead of logger to forward sync-microservice output because the sync-microservice already formats its logs using its own logging setup (get_sync_logger and LoggerWriter). Using logger would result in double-prefixing.
📚 Learning: 2025-10-01T20:44:19.127Z
Learnt from: Hemil36
PR: AOSSIE-Org/PictoPy#548
File: backend/app/utils/microservice.py:53-62
Timestamp: 2025-10-01T20:44:19.127Z
Learning: In backend/app/utils/microservice.py, the stream_logs function uses print() instead of logger to forward sync-microservice output because the sync-microservice already formats its logs using its own logging setup (get_sync_logger and LoggerWriter). Using logger would result in double-prefixing.
Applied to files:
backend/app/utils/microservice.py
🧬 Code graph analysis (1)
backend/app/utils/microservice.py (1)
backend/app/logging/setup_logging.py (2)
setup_logging(134-189)get_logger(192-202)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Backend Tests
🔇 Additional comments (6)
backend/app/utils/microservice.py (6)
8-12: LGTM! Clean integration with centralized logging.The imports and logger initialization correctly integrate the new centralized logging system.
14-27: Cleanup logic is sound.The thread tracking and cleanup mechanism correctly ensures buffered logs are processed during shutdown. The 2-second timeout per thread is reasonable for graceful shutdown, though you may consider increasing it if subprocesses are verbose.
29-30: LGTM! Proper use of atexit for cleanup.Registering
cleanup_log_threadswithatexitensures log threads are joined on normal shutdown.
33-38: LGTM! Explicit cleanup function is good practice.This function allows callers to manually trigger log thread cleanup before interpreter exit, complementing the
atexitregistration.
138-154: LGTM! Thread management correctly addresses shutdown concerns.Setting
daemon=Falseand tracking threads in_log_threadsensures buffered logs are not lost during shutdown, addressing the previous review concern.
329-352: LGTM! Correct subprocess and thread handling.The
cmdlist and non-daemon thread setup ensure cross-platform compatibility and graceful shutdown with log preservation.
Hemil36
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/database/images.py (1)
202-246: Add exception handling for consistency.The
db_get_untagged_imagesfunction lacks a try-except block, making it inconsistent with all other database functions in this file. If an exception occurs during query execution, it won't be logged before propagating.Add exception handling similar to other functions:
def db_get_untagged_images() -> List[UntaggedImageRecord]: """ Find all images that need AI tagging. Returns images where: - The image's folder has AI_Tagging enabled (True) - The image has isTagged set to False Returns: List of dictionaries containing image data: id, path, folder_id, thumbnailPath, metadata """ conn = _connect() cursor = conn.cursor() try: cursor.execute( """ SELECT i.id, i.path, i.folder_id, i.thumbnailPath, i.metadata FROM images i JOIN folders f ON i.folder_id = f.folder_id WHERE f.AI_Tagging = TRUE AND i.isTagged = FALSE """ ) results = cursor.fetchall() untagged_images = [] for image_id, path, folder_id, thumbnail_path, metadata in results: from app.utils.images import image_util_parse_metadata md = image_util_parse_metadata(metadata) untagged_images.append( { "id": image_id, "path": path, "folder_id": str(folder_id) if folder_id is not None else None, "thumbnailPath": thumbnail_path, "metadata": md, } ) return untagged_images - + except Exception as e: + logger.exception(f"Error getting untagged images: {e}") + return [] finally: conn.close()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/app/database/face_clusters.py(1 hunks)backend/app/database/faces.py(2 hunks)backend/app/database/images.py(7 hunks)backend/app/utils/images.py(4 hunks)backend/requirements.txt(0 hunks)
💤 Files with no reviewable changes (1)
- backend/requirements.txt
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/app/database/face_clusters.py
- backend/app/utils/images.py
- backend/app/database/faces.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Hemil36
PR: AOSSIE-Org/PictoPy#548
File: backend/app/utils/microservice.py:53-62
Timestamp: 2025-10-01T20:44:19.146Z
Learning: In backend/app/utils/microservice.py, the stream_logs function uses print() instead of logger to forward sync-microservice output because the sync-microservice already formats its logs using its own logging setup (get_sync_logger and LoggerWriter). Using logger would result in double-prefixing.
🔇 Additional comments (2)
backend/app/database/images.py (2)
9-12: LGTM: Centralized logger properly initialized.The logger is correctly imported from the centralized logging setup and initialized with the module name, following the established pattern.
374-374: Appropriate use of info-level logging.Using
logger.infofor the successful deletion message is correct, as this is an informational event rather than an error.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/requirements.txt(1 hunks)
🧰 Additional context used
🪛 OSV Scanner (2.2.3)
backend/requirements.txt
[HIGH] 1-1: jinja2 3.1.4: Jinja2 vulnerable to sandbox breakout through attr filter selecting format method
[HIGH] 1-1: jinja2 3.1.4: Jinja has a sandbox breakout through malicious filenames
[HIGH] 1-1: jinja2 3.1.4: Jinja has a sandbox breakout through indirect reference to format method
[HIGH] 1-1: certifi 2024.2.2: undefined
(PYSEC-2024-230)
[HIGH] 1-1: certifi 2024.2.2: Certifi removes GLOBALTRUST root certificate
[CRITICAL] 1-1: h11 0.14.0: h11 accepts some malformed Chunked-Encoding bodies
[HIGH] 1-1: protobuf 5.26.1: protobuf-python has a potential Denial of Service issue
[HIGH] 1-1: python-multipart 0.0.9: Denial of service (DoS) via deformation multipart/form-data boundary
[HIGH] 1-1: setuptools 66.1.1: undefined
(PYSEC-2025-49)
[HIGH] 1-1: setuptools 66.1.1: setuptools has a path traversal vulnerability in PackageIndex.download that leads to Arbitrary File Write
[HIGH] 1-1: setuptools 66.1.1: setuptools vulnerable to Command Injection via package URL
[HIGH] 1-1: starlette 0.37.2: Starlette has possible denial-of-service vector when parsing large files in multipart forms
[HIGH] 1-1: starlette 0.37.2: Starlette Denial of service (DoS) via multipart/form-data
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Backend Tests
Hemil36
changed the title
|
@Hemil36 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/app/utils/microservice.py (1)
125-134: Use a list for thecmdargument to ensure Windows compatibility.Line 125 creates
cmdas a string, which can fail on Windows when passed tosubprocess.Popenwithoutshell=True. This was flagged in a previous review but remains unaddressed.Apply this diff:
- cmd = str(sync_executable) # Correct the command to use the actual path - logger.info(f"Starting sync microservice with command: {cmd}") - process = subprocess.Popen( - cmd, + [str(sync_executable)], stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, bufsize=1, # Line buffered output )You can then update the logger message accordingly:
- logger.info(f"Starting sync microservice from: {sync_executable}") + logger.info(f"Starting sync microservice from: {sync_executable}") - - # Start the sync microservice executable - cmd = str(sync_executable) # Correct the command to use the actual path - logger.info(f"Starting sync microservice with command: {cmd}")
🧹 Nitpick comments (1)
backend/app/utils/microservice.py (1)
74-78: Remove unused color constants.If
stream_logsis updated per the previous comment, these color constants become entirely unused and should be removed.Apply this diff:
-CYAN = "\033[96m" -RED = "\033[91m" -MAGENTA = "\033[95m" -RESET = "\033[0m" - -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/utils/microservice.py(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-01T20:44:19.146Z
Learnt from: Hemil36
PR: AOSSIE-Org/PictoPy#548
File: backend/app/utils/microservice.py:53-62
Timestamp: 2025-10-01T20:44:19.146Z
Learning: In backend/app/utils/microservice.py, the stream_logs function uses print() instead of logger to forward sync-microservice output because the sync-microservice already formats its logs using its own logging setup (get_sync_logger and LoggerWriter). Using logger would result in double-prefixing.
Applied to files:
backend/app/utils/microservice.py
🧬 Code graph analysis (1)
backend/app/utils/microservice.py (1)
backend/app/logging/setup_logging.py (2)
setup_logging(134-189)get_logger(192-202)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Backend Tests
🔇 Additional comments (4)
backend/app/utils/microservice.py (4)
14-30: LGTM! Thread cleanup mechanism correctly implemented.The thread tracking and cleanup logic properly addresses the previous concern about losing buffered logs on exit. Non-daemon threads are tracked and joined with appropriate timeouts.
33-38: LGTM! Public shutdown function provides explicit cleanup control.The
microservice_util_stop_sync_service()function correctly exposes thread cleanup for explicit shutdown scenarios beyond atexit.
138-153: LGTM! Thread management correctly implemented.The non-daemon threads are properly created, started, and tracked in
_log_threadsfor cleanup. This ensures logs are fully drained before exit.
328-352: LGTM! Command list and thread management correctly implemented.Unlike
_start_frozen_sync_service, this function correctly buildscmdas a list (lines 310-323), and the thread tracking follows the proper pattern.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/app/utils/microservice.py (1)
125-134: Use a list forcmdto ensure cross-platform compatibility.Line 125 constructs
cmdas a string and passes it tosubprocess.Popenat line 129 withoutshell=True. On Windows, this may fail or behave unexpectedly. Use a list for consistent cross-platform behavior.Apply this diff:
- cmd = str(sync_executable) # Correct the command to use the actual path + cmd = [str(sync_executable)] logger.info(f"Starting sync microservice with command: {cmd}") process = subprocess.Popen(
🧹 Nitpick comments (1)
backend/app/utils/microservice.py (1)
80-89: Remove unusedprefixandcolorparameters.The
stream_logsfunction acceptsprefixandcolorparameters but never uses them. The use ofprint(line)at line 88 is correct to avoid double-prefixing (as the sync-microservice already formats its logs), but the unused parameters should be removed to clean up the API.Apply this diff:
-def stream_logs(pipe, prefix, color): +def stream_logs(pipe): """Read a process pipe and print formatted logs from sync-microservice.""" for line in iter(pipe.readline, ""): if line: - # Trim any trailing newlines line = line.strip() if line: # All output from sync-microservice is now properly formatted by its logger print(line) pipe.close()Then update call sites at lines 139-140, 146-147, 338-339, and 344-345:
- args=(process.stdout, "SYNC-MICROSERVICE", CYAN), + args=(process.stdout,),- args=(process.stderr, "SYNC-MICROSERVICE-ERR", RED), + args=(process.stderr,),Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/utils/microservice.py(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Hemil36
PR: AOSSIE-Org/PictoPy#548
File: backend/app/utils/microservice.py:53-62
Timestamp: 2025-10-01T20:44:19.146Z
Learning: In backend/app/utils/microservice.py, the stream_logs function uses print() instead of logger to forward sync-microservice output because the sync-microservice already formats its logs using its own logging setup (get_sync_logger and LoggerWriter). Using logger would result in double-prefixing.
📚 Learning: 2025-10-01T20:44:19.146Z
Learnt from: Hemil36
PR: AOSSIE-Org/PictoPy#548
File: backend/app/utils/microservice.py:53-62
Timestamp: 2025-10-01T20:44:19.146Z
Learning: In backend/app/utils/microservice.py, the stream_logs function uses print() instead of logger to forward sync-microservice output because the sync-microservice already formats its logs using its own logging setup (get_sync_logger and LoggerWriter). Using logger would result in double-prefixing.
Applied to files:
backend/app/utils/microservice.py
🧬 Code graph analysis (1)
backend/app/utils/microservice.py (1)
backend/app/logging/setup_logging.py (2)
setup_logging(134-189)get_logger(192-202)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Backend Tests
🔇 Additional comments (4)
backend/app/utils/microservice.py (4)
8-30: LGTM! Thread management and cleanup are well-implemented.The introduction of global thread tracking with
_log_threads, thecleanup_log_threadsfunction with appropriate timeouts, andatexitregistration properly addresses the concern about losing buffered logs on shutdown.
33-38: LGTM! Public shutdown API is clear.Provides a clean public interface for explicitly stopping the sync service and cleaning up log threads.
136-153: LGTM! Thread management is correct.The use of non-daemon threads with proper tracking in
_log_threadsensures buffered logs are not lost on shutdown. The threads will be properly cleaned up via theatexithandler.
328-351: LGTM! Popen and threading setup are correct.The command is properly constructed as a list (lines 310-323), and the threading setup correctly mirrors the frozen service approach with non-daemon threads tracked in
_log_threads.
…ce utility functions
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/app/utils/microservice.py (1)
125-134: Use a list for the command to ensure cross-platform compatibility.Line 125 constructs
cmdas a string (str(sync_executable)), which can fail on Windows when passed tosubprocess.Popenwithoutshell=True. A previous review flagged this issue. For consistent, secure behavior across platforms, use a list instead.Apply this diff:
- cmd = str(sync_executable) # Correct the command to use the actual path + cmd = [str(sync_executable)] logger.info(f"Starting sync microservice with command: {cmd}") process = subprocess.Popen(
🧹 Nitpick comments (3)
backend/app/utils/microservice.py (3)
33-38: Consider removing this wrapper or clarifying its purpose.
microservice_util_stop_sync_service()is a thin wrapper aroundcleanup_log_threads(), which is already registered withatexit. If this function provides a public API for explicit shutdown scenarios beyond automatic cleanup, please clarify in the docstring. Otherwise, consider removing it to reduce redundancy.
74-77: Remove unused color constants.The
CYAN,RED,MAGENTA, andRESETconstants are defined but never used. Thestream_logsfunction accepts acolorparameter but does not apply it (intentionally, per your clarification about avoiding double-prefixing). These constants should be removed to eliminate dead code.Apply this diff:
-CYAN = "\033[96m" -RED = "\033[91m" -MAGENTA = "\033[95m" -RESET = "\033[0m" - - def stream_logs(pipe, prefix, color):
80-89: Remove unusedprefixandcolorparameters.The
stream_logsfunction acceptsprefixandcolorparameters but does not use them in the function body. Since the sync-microservice already formats its logs (per your earlier clarification), these parameters serve no purpose and should be removed to simplify the API and eliminate confusion.Apply this diff to the function signature:
-def stream_logs(pipe, prefix, color): +def stream_logs(pipe): """Read a process pipe and print formatted logs from sync-microservice."""Then update the call sites at lines 140, 147, 334, and 340 to remove the unused arguments:
- args=(process.stdout, "SYNC-MICROSERVICE", CYAN), + args=(process.stdout,),- args=(process.stderr, "SYNC-MICROSERVICE-ERR", RED), + args=(process.stderr,),Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/utils/microservice.py(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-01T20:44:19.146Z
Learnt from: Hemil36
PR: AOSSIE-Org/PictoPy#548
File: backend/app/utils/microservice.py:53-62
Timestamp: 2025-10-01T20:44:19.146Z
Learning: In backend/app/utils/microservice.py, the stream_logs function uses print() instead of logger to forward sync-microservice output because the sync-microservice already formats its logs using its own logging setup (get_sync_logger and LoggerWriter). Using logger would result in double-prefixing.
Applied to files:
backend/app/utils/microservice.py
🧬 Code graph analysis (1)
backend/app/utils/microservice.py (1)
backend/app/logging/setup_logging.py (2)
setup_logging(134-189)get_logger(192-202)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Backend Tests
🔇 Additional comments (4)
backend/app/utils/microservice.py (4)
8-12: LGTM! Logger initialization follows best practices.The integration with the centralized logging system via
get_logger(__name__)is correct and consistent with the PR's objectives.
14-30: LGTM! Cleanup mechanism addresses daemon thread concerns.The global thread tracking and
atexitregistration ensure log threads are gracefully joined during shutdown, preventing log loss. The 2-second timeout per thread is reasonable for typical scenarios.
136-153: LGTM! Non-daemon threads with tracking prevent log loss.The use of
daemon=Falseand tracking threads in_log_threadsensures buffered logs are processed before shutdown, addressing the concern from earlier reviews. The threading implementation is sound.
302-346: LGTM! Command construction and threading are correct.The command is properly constructed as a list (lines 308-318), and the threading approach for log forwarding is consistent with
_start_frozen_sync_service. The implementation correctly handles subprocess output streaming.
… in watcher utility
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
sync-microservice/app/utils/watcher.py (2)
74-74: Replace magic number with logging constant.Use the
logging.DEBUGconstant instead of the magic number10for better code clarity and maintainability.Apply this diff:
- if logger.isEnabledFor(10): # DEBUG level is 10 + if logger.isEnabledFor(logging.DEBUG):
88-92: Remove unused variables.The variables
total_changes,thumbnail_changes, andother_changesare computed but never used after being incremented. This wastes computation cycles on every file change event.If these counters are not needed for logging or debugging, remove them:
- # Initialize counters - total_changes = len(changes) - thumbnail_changes = 0 - other_changes = 0 affected_folders = {} # folder_path -> folder_id mapping # First pass - count changes and identify affected folders for change, file_path in changes: - # Count by file type - if "thumbnails" in file_path: - thumbnail_changes += 1 - else: - other_changes += 1 - # Process deletionsAlternatively, if these counters were intended for logging, add a log statement that uses them.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/logging/setup_logging.py(1 hunks)sync-microservice/app/utils/watcher.py(13 hunks)utils/logging/logging_config.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- utils/logging/logging_config.json
- backend/app/logging/setup_logging.py
🧰 Additional context used
🧬 Code graph analysis (1)
sync-microservice/app/utils/watcher.py (2)
sync-microservice/app/database/folders.py (1)
db_get_all_folders_with_ids(22-44)sync-microservice/app/logging/setup_logging.py (2)
setup_logging(134-189)get_sync_logger(205-217)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Backend Tests
🔇 Additional comments (3)
sync-microservice/app/utils/watcher.py (3)
1-16: LGTM! Clean logger initialization.The imports and logger setup follow the centralized logging pattern correctly. Configuring third-party loggers (httpx, httpcore) to WARNING level is a good practice to reduce log noise.
163-218: LGTM! Proper structured logging for API calls.The API call functions now use appropriate logging levels (info for success, error for failures) with sufficient context in error messages.
246-417: LGTM! Comprehensive logging coverage.All utility functions now use structured logging with appropriate levels and context. The replacement of print statements with logger calls is thorough and consistent with the centralized logging objectives.
|
@rahulharpal1603 Is there any changes needed to be done in core logging system ? So that I do the needful in one commit |
…proved clarity and consistency
No, just fix the lint errors also. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
sync-microservice/app/utils/watcher_helpers.py (3)
20-22: Remove unnecessary None check.The lambda function checks
if x[1] is not Nonebut the type signature declareschanges: Set[Tuple[Change, str]]where the path is alwaysstr, notOptional[str]. This defensive check is unnecessary and suggests a type mismatch.Apply this diff to simplify the sorting key:
- for change, path in sorted( - changes, key=lambda x: x[1] if x[1] is not None else "" - ): + for change, path in sorted(changes, key=lambda x: x[1]):
30-31: Clarify indentation intent.Line 30 prepends 4 spaces to each line, but those lines already contain the " - " prefix from line 28. This results in double indentation: " " + " - " = 6-space indent. If this is intentional for hierarchical formatting, consider adding a comment explaining the indentation scheme.
32-33: Avoid broad exception handler that may hide bugs.Catching all exceptions with
except Exception as ecan mask programming errors (TypeError, AttributeError, etc.) that should be fixed rather than silently converted to an error message. Consider catching only specific exceptions you expect (e.g.,AttributeErrorif accessing missing attributes).If you want to keep the broad handler for robustness, at least log the full traceback to aid debugging:
except Exception as e: - return f"Error formatting changes: {str(e)}" + import traceback + return f"Error formatting changes: {str(e)}\n{traceback.format_exc()}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sync-microservice/app/utils/watcher.py(13 hunks)sync-microservice/app/utils/watcher_helpers.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
sync-microservice/app/utils/watcher.py (3)
sync-microservice/app/database/folders.py (1)
db_get_all_folders_with_ids(22-44)sync-microservice/app/logging/setup_logging.py (2)
setup_logging(134-189)get_sync_logger(205-217)sync-microservice/app/utils/watcher_helpers.py (1)
format_debug_changes(5-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Backend Tests
🔇 Additional comments (4)
sync-microservice/app/utils/watcher.py (4)
12-20: LGTM! Good practice silencing noisy third-party loggers.Configuring
httpx,httpcore, andwatchfilesloggers to WARNING level prevents log noise while maintaining error visibility. The module-level logger initialization is clean and consistent with the centralized logging pattern.
203-205: Conditional import inside loop is unusual but acceptable here.Importing
format_debug_changesonly when DEBUG is enabled is a micro-optimization that avoids the import overhead in production. While unconventional (PEP 8 prefers top-level imports), this pattern is acceptable for debug-only utilities. The guard ensures the import cost is paid only when needed.
61-82: Refactor improves efficiency by deduplicating folder syncs.The two-pass approach (lines 64-78 collect affected folders, lines 81-82 sync once per folder) eliminates redundant API calls when multiple files change in the same folder. This is a solid improvement over the inline approach.
142-154: Verify logger calls include all relevant context for error triage.The logger calls capture folder_path, folder_id, status codes, and exception messages. Ensure that for production debugging, you have request correlation IDs or timestamps elsewhere in the log formatter (likely handled by
setup_logging.py). The current context is good for immediate triage.Confirm that
setup_logging.pyadds timestamps and any request correlation IDs to all log records, as mentioned in the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
sync-microservice/app/utils/watcher_helpers.py (3)
5-14: Consider documenting the output format.The docstring could mention that the output is indented and the format of each line (e.g., "Returns indented, formatted lines with change type and path").
22-29: Simplify change-type mapping and indentation logic.Two readability improvements:
- Nested ternary is hard to read (lines 22-26): A dictionary mapping would be clearer and more maintainable.
- Redundant indentation steps (lines 27-28): The code adds " - " (2 spaces) in line 27, then adds " " (4 more spaces) in line 28, resulting in 6-space indentation. This two-step process is confusing.
Apply this diff to simplify both:
- for change, path in sorted( - changes, key=lambda x: x[1] if x[1] is not None else "" - ): - change_type = ( - "deleted" - if change == Change.deleted - else "modified" if change == Change.modified else "added" - ) - debug_changes.append(f" - {change_type}: {path}") - indented = "\n".join(" " + line for line in debug_changes) - return indented + change_type_map = { + Change.deleted: "deleted", + Change.modified: "modified", + Change.added: "added" + } + for change, path in sorted( + changes, key=lambda x: x[1] if x[1] is not None else "" + ): + change_type = change_type_map.get(change, "unknown") + debug_changes.append(f" - {change_type}: {path}") + return "\n".join(debug_changes)
30-31: Minor format inconsistency in error output.The error message returns a single-line string, while normal output is multi-line with indentation. If the caller expects consistent formatting, this mismatch could cause issues. Consider maintaining the same indentation format for errors or documenting this behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sync-microservice/app/utils/watcher_helpers.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Backend Tests
🔇 Additional comments (2)
sync-microservice/app/utils/watcher_helpers.py (2)
1-2: LGTM!Imports are appropriate and minimal for the function's requirements.
15-21: LGTM!Early return for empty input is good practice. The defensive None check in the sort key is unlikely to be needed (watchfiles paths are strings), but it's harmless defensive programming.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
sync-microservice/app/utils/watcher_helpers.py (2)
22-28: Consider refactoring nested ternary for readability.The nested ternary operator works but is harder to read. Consider a dictionary mapping or match-case (Python 3.10+) for cleaner code.
Apply this diff to use a dictionary mapping:
- change_type = ( - "deleted" - if change == Change.deleted - else "modified" - if change == Change.modified - else "added" - ) + change_type_map = { + Change.deleted: "deleted", + Change.modified: "modified", + Change.added: "added", + } + change_type = change_type_map.get(change, "unknown")
32-33: Overly broad exception handling.Catching all exceptions may hide bugs. Consider being more specific about expected exceptions (e.g.,
TypeError,AttributeError) or letting unexpected exceptions propagate during development.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sync-microservice/app/utils/watcher_helpers.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Backend Tests
🔇 Additional comments (1)
sync-microservice/app/utils/watcher_helpers.py (1)
1-2: LGTM!The imports are appropriate and correctly match the function's type hints and logic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
utils/logging/logging_config.json (1)
49-49: Add newline at end of file.The file is missing a trailing newline, which is a standard convention for most text files and source files.
"default_environment": "development" } +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
utils/logging/logging_config.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Backend Tests
🔇 Additional comments (1)
utils/logging/logging_config.json (1)
35-40: Verify watchfiles log level aligns with the intended logging strategy.In past review comments, you noted that watchfiles should be set to WARNING because you're "overriding Watcher Logs and doing formatted logs manually." The current configuration sets watchfiles and watchfiles.main to INFO level in development. This may produce excessive verbosity if the goal was to suppress watchfiles library output.
Clarify whether INFO is the intended level or if it should revert to WARNING per your earlier reasoning.
|
@coderabbitai resolve |
✅ Actions performedComments resolved. |
2 participants
This pull request implements code changes to introduce a centralized and colorized logging system for the PictoPy backend and sync-microservice, replacing all direct print statements with structured logging via a new shared logging utility.
This PR focuses only on code changes. Documentation and usage instructions will be submitted in a separate PR.
Fixes: #526
Implementation Overview
Centralized Configuration
utils/logging/logging_config.jsonserving as the single source of truth for logging settings, including log levels, formatting, and environment-specific overrides.Core Logging Utility
backend/app/logging/setup_logging.pyandsync-microservice/app/logging/setup_logging.py.Backend Modules Refactoring
printstatements with structured logger calls in modules where print statements were present.Sync-Microservice Integration
Screenshots
Summary by CodeRabbit
New Features
Improvements
Chores