-
Notifications
You must be signed in to change notification settings - Fork 1
Feat/custome handler #16
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.
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
Conversation
* Introduced `custom_handlers` field in `LoggingSettings` to allow users to specify custom logging handlers. * Updated `_setup_custom_handlers` method in `LoggingConfig` to configure these handlers if provided.
* Incremented the version number in `pyproject.toml` to reflect the latest changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds support for custom logging handlers in the chromatrace library, allowing users to extend logging functionality by providing their own handler instances. The implementation follows the existing pattern of setting up handlers with formatters and filters, though there are some areas that could be improved for better type safety, error handling, and test coverage.
Key changes:
- Added
custom_handlersfield toLoggingSettingsfor users to specify custom logging handlers - Implemented
_setup_custom_handlersmethod to configure custom handlers with appropriate formatters and filters - Fixed type annotation for
handlerparameter fromlogging.handlerstologging.Handler
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/chromatrace/logging_settings.py | Adds custom_handlers field to LoggingSettings with Field import to support default factory |
| src/chromatrace/logging_config.py | Implements custom handler setup logic and fixes type annotation for handler parameter |
| pyproject.toml | Bumps version from 0.2.15 to 0.2.16 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return | ||
|
|
||
| for handler in self.settings.custom_handlers: | ||
| if not isinstance(handler, logging.Handler): |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invalid handlers are silently skipped with continue, which could lead to confusion when a user's custom handler doesn't appear in the logger. Consider logging a warning when an invalid handler is encountered to help with debugging.
Suggested change:
for handler in self.settings.custom_handlers:
if not isinstance(handler, logging.Handler):
logging.getLogger(__name__).warning(
f"Skipping invalid custom handler: {type(handler).__name__}. "
"Expected logging.Handler instance."
)
continue| if not isinstance(handler, logging.Handler): | |
| if not isinstance(handler, logging.Handler): | |
| logging.getLogger(__name__).warning( | |
| f"Skipping invalid custom handler: {type(handler).__name__}. " | |
| "Expected logging.Handler instance." | |
| ) |
| def _setup_custom_handlers(self, logger: logging.Logger): | ||
| if not hasattr(self.settings, "custom_handlers"): | ||
| return | ||
|
|
||
| for handler in self.settings.custom_handlers: | ||
| if not isinstance(handler, logging.Handler): | ||
| continue | ||
| formatter = self._get_formatter( | ||
| colored=self.settings.use_console_colored_formatter | ||
| ) | ||
| self._set_logger_settings(handler, formatter, logger) |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new custom_handlers feature lacks test coverage. Since the repository has comprehensive test coverage for other logging features (see test/logger_test.py), this new functionality should also be tested. Consider adding tests that verify:
- Custom handlers are properly added to the logger
- Custom handlers receive the correct formatter
- Custom handlers get the appropriate filters (tracing, application level)
- Invalid handlers are handled gracefully
| formatter = self._get_formatter( | ||
| colored=self.settings.use_console_colored_formatter | ||
| ) |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Custom handlers always receive a formatter based on use_console_colored_formatter, which may not be appropriate for all custom handlers. For example, if a user adds a file-based custom handler, they might want use_file_colored_formatter instead, or a completely different formatter.
Consider one of these approaches:
- Allow users to pass handlers that already have formatters set, and only apply a formatter if the handler doesn't already have one
- Add a configuration option to specify which formatter setting applies to custom handlers
- Document this behavior clearly so users know custom handlers will use the console formatter settings
| use_syslog_colored_formatter: bool = False | ||
| use_file_colored_formatter: bool = False | ||
| show_process_id: bool = False | ||
| custom_handlers: list = Field(default_factory=list) |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type annotation for custom_handlers should be more specific. Since this field is expected to contain logging handlers, it should be typed as list[logging.Handler] instead of just list. This improves type safety and makes the API clearer to users.
Suggested change:
custom_handlers: list[logging.Handler] = Field(default_factory=list)| custom_handlers: list = Field(default_factory=list) | |
| custom_handlers: list[logging.Handler] = Field(default_factory=list) |
| if not hasattr(self.settings, "custom_handlers"): | ||
| return | ||
|
|
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hasattr check on line 48 is unnecessary since custom_handlers is now a defined field in the LoggingSettings Pydantic model with a default value. The attribute will always exist, so this check can be removed.
The method can be simplified to:
def _setup_custom_handlers(self, logger: logging.Logger):
for handler in self.settings.custom_handlers:
if not isinstance(handler, logging.Handler):
continue
formatter = self._get_formatter(
colored=self.settings.use_console_colored_formatter
)
self._set_logger_settings(handler, formatter, logger)| if not hasattr(self.settings, "custom_handlers"): | |
| return |
This pull request introduces support for custom logging handlers in the
chromatracelibrary, allowing users to extend logging functionality by specifying their own handlers. The changes also include a version bump and a minor type fix.Custom Logging Handlers Support:
custom_handlersfield to theLoggingSettingsclass, allowing users to specify a list of custom logging handlers._setup_custom_handlersmethod inlogging_config.pyto initialize and configure custom handlers when present in the settings.Other Changes:
0.2.15to0.2.16inpyproject.toml.handlerparameter in_set_logger_settingsfromlogging.handlerstologging.Handler.Fieldfrompydanticto support the new field definition inLoggingSettings.