feat: Add dynamic module discovery to pylogger #5
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change adds dynamic logger discovery that automatically configures logging for all modules in the template_agent package hierarchy. **What this fixes:** - Previous implementation required manual updates to logger list when adding new modules - New modules wouldn't get proper logging configuration automatically - Required developers to remember to update get_uvicorn_log_config() **What this adds:** - _discover_app_loggers() function that dynamically walks the package tree - Automatic discovery of all submodules and subpackages - Caching mechanism to avoid repeated package traversal - Graceful fallback to common patterns if package import fails **Benefits:** - Zero maintenance - new modules automatically get logging configured - Better developer experience - no manual logging setup needed - Consistent logging behavior across all modules - Performance optimized with result caching **Testing:** - All 39 existing tests pass - Ruff linting passes - Backwards compatible - no breaking changes This improvement was successfully implemented and tested in the harbor-agent fork where it has been running in production.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
Improvements based on PR review by @webbnh: 1. Use underscore for unused variables in pkgutil.walk_packages() - Changed 'for importer, modname, ispkg' to 'for _, modname, _' - More Pythonic to explicitly mark unused variables 2. Verify fallback paths exist before adding to logger list - Added try/except ImportError for each fallback pattern - Only adds logger names for modules that can actually be imported - Prevents logging configuration for non-existent modules 3. Minimize exception handling scope - Removed AttributeError from exception tuple (impossible due to hasattr guard) - Minimized try block to only cover importlib.import_module(package_name) - Moved package.__path__ check and pkgutil.walk_packages outside try block - Improves code clarity and prevents unintended exception masking
Author
|
@webbnh thanks for the feedback. Check all three comments in last commit. |
webbnh
reviewed
Oct 29, 2025
webbnh
left a comment
webbnh
left a comment
There was a problem hiding this comment.
The changes look good, but I'm wondering if there's a gap.
This comment was marked as resolved.
This comment was marked as resolved.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
|
@mcastrol the pre-commit is failing. it needs a fix (run |
…ixes, and comprehensive tests - Add Optional[List[str]] type annotation to _DISCOVERED_LOGGERS_CACHE to fix mypy unreachable code error - Apply ruff formatting fixes (trailing comma, quote consistency, line formatting) - Add comprehensive pytest test suite for _discover_app_loggers function with 6 test cases - Tests cover real package discovery, caching, fallback patterns, and edge cases - All 45 tests passing, pre-commit hooks satisfied
Author
|
@tuhinsharma121 done. please check. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Implementation improvements: - Use named field access (pkg.name) instead of tuple unpacking in walk_packages() This makes the code more explicit and maintainable by using ModuleInfo's field name Test improvements: - Move module import to top of file (pylogger_module) Eliminates redundant imports in each test method - Remove cache reset from individual test methods setup_method() already handles this for all tests - Change identity check to equality check for cached results Tests interface contract (equality) not implementation detail (identity) - Add assertions for nonexistent patterns in fallback test Stronger assertion than just checking for no errors - Use ModuleNotFoundError instead of generic ImportError in mocks Matches actual Python behavior more accurately - Add "MOCK:" prefix to mock exception messages Makes it clear exceptions come from test mocks, not real code - Use AssertionError for unexpected mock calls Causes test to fail obviously if mock is called incorrectly - Fix mock function signatures to accept all parameters side_effect(name, package=None) matches importlib.import_module signature - Use ModuleInfo objects in walk_packages mock Accurately represents actual return type from pkgutil - Return generator instead of list from walk_packages mock Matches actual pkgutil.walk_packages() behavior - Remove implementation detail assertion (mock_walk.assert_called_once) Tests functionality, not how it's achieved - Simplify assertions where any() is not needed More readable and consistent with other tests All changes follow best practices: - Tests enforce interface contracts without restricting implementation - Mocks accurately represent real function signatures and return types - Stronger assertions provide better test coverage - Code is more maintainable and conventional Addresses all feedback from code review.
Author
|
@webbnh check the changes. |
webbnh
reviewed
Nov 4, 2025
webbnh
left a comment
webbnh
left a comment
There was a problem hiding this comment.
@mcastrol, it all looks good to me, now, with the possible exception of the comment that I made earlier. You haven't updated that part of the code, and you haven't responded to the comment.
|
@tuhinsharma121, apparently this PR needs testing approval again. |
Address PR feedback by refactoring _discover_app_loggers function to use a single common exit point and eliminate duplicated cache-and-return code. Changes: - Use try-except-else pattern for clearer control flow - Invert hasattr check to avoid early returns - Single exit point at end of function handles caching for all paths - Eliminates three instances of duplicated code This makes the code more maintainable and prevents future bugs from inconsistent updates across multiple exit points.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change adds dynamic logger discovery that automatically configures logging for all modules in the template_agent package hierarchy.
What this fixes:
What this adds:
Benefits:
Testing:
This improvement was successfully implemented and tested in the harbor-agent fork where it has been running in production.