Skip to content

feat(logging): Implement centralized logging with loguru#81

Open
briansumma wants to merge 1 commit intoNorthShoreAutomation:mainfrom
briansumma:feature/logging
Open

feat(logging): Implement centralized logging with loguru#81
briansumma wants to merge 1 commit intoNorthShoreAutomation:mainfrom
briansumma:feature/logging

Conversation

@briansumma
Copy link
Copy Markdown
Contributor

@briansumma briansumma commented May 19, 2025

Centralized Logging Implementation

This PR introduces a centralized logging system for the Pythonik SDK using the loguru library.

Changes

  1. New Module: _logger.py

    • Creates a single source of truth for logging configuration
    • Configures loguru with a stderr handler by default
    • Respects LOGURU_LEVEL environment variable with "INFO" as default
  2. Updates to Base Classes

    • Replaces print statements with structured logging in Spec.parse_response and Spec.send_request
    • Provides formatted output for better debugging
  3. Import Standardization

    • Updates imports in affected modules to use the centralized logger
    • Ensures consistent logging approach across the codebase
  4. Comprehensive Test Suite

    • Tests for core logger configuration and environment variable handling
    • Tests for verification of logging in the base Spec class
    • Tests for import patterns across modules
    • Integration tests for end-to-end validation

Benefits

  • Improved Debugging: Structured logs with configurable verbosity
  • Environment Control: Adjust log levels via environment variables
  • Consistent Interface: Standardized logging approach throughout the codebase
  • Future Extensibility: Foundation for more advanced logging features

Testing

All tests pass as shown in the attached pytest logs. The implementation has been thoroughly tested for both functionality and edge cases.

Usage Examples

Configure log level via environment variable:

LOGURU_LEVEL=DEBUG python your_script.py

Default level is INFO, which keeps output clean while still showing important information.

Notes for Reviewers

  • The changes are minimally invasive and maintain backward compatibility
  • No changes to API interfaces or behavior
  • Only debug-level logging has been implemented to avoid excessive output
  • Tests have been designed to verify behavior rather than implementation details

Add centralized logger configuration in _logger.py that:
- Configures loguru as the logging system for the entire codebase
- Removes default handlers and configures a stderr handler
- Respects LOGURU_LEVEL environment variable with default level "INFO"
- Standardizes logging across modules

Replace print statements with structured logging in the Spec class:
- Log request URLs at debug level with proper formatting
- Log response data at debug level for successful requests
- Update imports in affected modules to use the central logger

Tests:
- Added test_logger.py to verify core logger configuration
- Added test_base_spec_logging.py to test logging in Spec base class
- Added test_imports.py to verify correct logger imports
- Added test_logger_integration.py for end-to-end tests

This change improves debugging capabilities and provides a consistent
approach to logging throughout the codebase.
@briansumma briansumma changed the title Centralized Logging Implementation feat(logging): Implement centralized logging with loguru May 19, 2025
# try to populate the model
if response.ok:
print(response.text)
logger.debug(response.text)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thanks for doing this, we need to get rid of the noise in pythonik...

@@ -0,0 +1,18 @@
import sys
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for taking a swing at this, id prefer though if we followed loguru's suggested way of doing things here:

https://github.com/Delgan/loguru?tab=readme-ov-file#suitable-for-scripts-and-libraries

If i understand that doc, we'd disable our library logs by default and allow the user to opt in to enabling it

@briansumma
Copy link
Copy Markdown
Contributor Author

The logger from pythonik._logger should match the same behavior as the logger from loguru but instead of the default level being DEBUG, it is INFO. You can still change the log level in the environment like you do the default logger, e.g., export LOGURU_LEVEL=DEBUG, export LOGURU_FORMAT="{time} - {message}", so I don't think it negates that document link "Suitable for scripts and libraries" you sent.

I think the only change would be to import the logger from pythonik/_logger.py instead of directly from loguru, i.e.,

from pythonik._logger import logger

config = {
    "handlers": [
        {"sink": sys.stdout, "format": "{time} - {message}"},
        {"sink": "file.log", "serialize": True},
    ],
    "extra": {"user": "someone"}
}
logger.configure(**config)

...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants