Fix: 优化日志体系，避免重复落盘，缓解日志文件过大的问题#267
Merged
Merged
Conversation
Contributor
审阅者指南将自定义 Action 日志统一接入共享日志系统，使所有日志都通过已配置的 root/app logger 并带日志轮转，同时移除单独的 agent.log 路径，并收紧对噪声较大的第三方 logger 的配置。 统一自定义 Action 日志流水线流程图flowchart LR
A[CustomAction code
get_logger] --> B[utils.logger
get_logger]
B --> C[AppLogger maante]
C --> D[RootLogger]
D --> E[ConsoleHandler]
D --> F[TimedRotatingFileHandler
debug/custom/runtime.log]
D -.configured level .-> G[Noisy third party loggers
cv2/numpy/PIL/...]
文件级变更
可能关联的 Issue
技巧与命令与 Sourcery 交互
自定义使用体验访问你的控制台 来：
获取帮助Original review guide in EnglishReviewer's GuideUnifies custom action logging with the shared logging system so all logs go through the configured root/app logger with rotation, and removes the separate agent.log path while tightening how noisy third‑party loggers are configured. Flow diagram for unified custom action logging pipelineflowchart LR
A[CustomAction code
get_logger] --> B[utils.logger
get_logger]
B --> C[AppLogger maante]
C --> D[RootLogger]
D --> E[ConsoleHandler]
D --> F[TimedRotatingFileHandler
debug/custom/runtime.log]
D -.configured level .-> G[Noisy third party loggers
cv2/numpy/PIL/...]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - 我发现了两个问题，并给出了一些整体层面的反馈：
- 在
_setup_loguru_logger中覆盖_ROOT_LOGGER.handlers（以及在_setup_std_logger中清空 handlers）可能会意外清除由嵌入该库的应用程序所设置的日志配置；建议改为在现有 handlers 基础上追加你的 handler，或提供一个显式的可选开关来接管 root logger。 - 由于
get_logger现在返回的是logging.getLogger(name or _APP_LOGGER_NAME)，而setup_logger只配置了 root logger 和 app logger，其他任何具名 logger 都完全依赖日志传播；如果你期望自定义组件使用自己的名称进行日志记录，建议在文档中说明，或显式配置这些 logger 的日志级别/传播行为。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Overwriting `_ROOT_LOGGER.handlers` in `_setup_loguru_logger` (and clearing handlers in `_setup_std_logger`) can unexpectedly wipe logging configuration set up by the embedding application; consider appending your handlers or providing an opt-in flag for taking over the root logger.
- Since `get_logger` now returns `logging.getLogger(name or _APP_LOGGER_NAME)` and `setup_logger` only configures the root and the app logger, any other named loggers rely solely on propagation; if you expect custom components to log with their own names, consider documenting or explicitly configuring those logger levels/propagation behavior.
## Individual Comments
### Comment 1
<location path="agent/utils/logger.py" line_range="186-187" />
<code_context>
- logging.root.setLevel(logging.DEBUG)
- for name in ("cv2", "numpy", "PIL", "matplotlib", "urllib3", "asyncio"):
- logging.getLogger(name).setLevel(logging.WARNING)
+ _ROOT_LOGGER.handlers = [_InterceptHandler()]
+ _ROOT_LOGGER.setLevel(logging.DEBUG)
+ _configure_noisy_loggers()
</code_context>
<issue_to_address>
**issue (bug_risk):** Overwriting root handlers can clobber existing logging configuration in embedding applications.
Assigning `[_InterceptHandler()]` to `_ROOT_LOGGER.handlers` (plus the `handlers.clear()` path for std logging) wipes out any handlers configured by the host process or other libraries, making this change globally invasive. In environments where logging is preconfigured (frameworks, notebooks, multi-service apps), consider appending the handler only when missing or guarding the reset behind an explicit opt-in flag to avoid unexpectedly discarding existing setups.
</issue_to_address>
### Comment 2
<location path="agent/utils/logger.py" line_range="190-193" />
<code_context>
+ _configure_noisy_loggers()
- return _loguru_logger
+ app_logger = get_logger(_APP_LOGGER_NAME)
+ app_logger.setLevel(logging.NOTSET)
+ app_logger.propagate = True
+ return app_logger
</code_context>
<issue_to_address>
**question (bug_risk):** Changing `setup_logger` to return an app `logging.Logger` instead of the underlying loguru logger may break call sites relying on loguru APIs.
Previously `setup_logger()` returned the loguru logger (`_loguru_logger`), exposing loguru-specific methods like `.bind` and `.catch`. It now returns a stdlib `logging.Logger`, so any callers that stored this value and used loguru APIs will break or change behavior. If callers depend on that richer API, consider either preserving the original return type or splitting responsibilities (e.g., a `get_app_logger()` plus a `setup_logging()` that returns `None`).
</issue_to_address>帮我变得更有用！请对每条评论点 👍 或 👎，我会根据你的反馈改进后续的 review。
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- Overwriting
_ROOT_LOGGER.handlersin_setup_loguru_logger(and clearing handlers in_setup_std_logger) can unexpectedly wipe logging configuration set up by the embedding application; consider appending your handlers or providing an opt-in flag for taking over the root logger. - Since
get_loggernow returnslogging.getLogger(name or _APP_LOGGER_NAME)andsetup_loggeronly configures the root and the app logger, any other named loggers rely solely on propagation; if you expect custom components to log with their own names, consider documenting or explicitly configuring those logger levels/propagation behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Overwriting `_ROOT_LOGGER.handlers` in `_setup_loguru_logger` (and clearing handlers in `_setup_std_logger`) can unexpectedly wipe logging configuration set up by the embedding application; consider appending your handlers or providing an opt-in flag for taking over the root logger.
- Since `get_logger` now returns `logging.getLogger(name or _APP_LOGGER_NAME)` and `setup_logger` only configures the root and the app logger, any other named loggers rely solely on propagation; if you expect custom components to log with their own names, consider documenting or explicitly configuring those logger levels/propagation behavior.
## Individual Comments
### Comment 1
<location path="agent/utils/logger.py" line_range="186-187" />
<code_context>
- logging.root.setLevel(logging.DEBUG)
- for name in ("cv2", "numpy", "PIL", "matplotlib", "urllib3", "asyncio"):
- logging.getLogger(name).setLevel(logging.WARNING)
+ _ROOT_LOGGER.handlers = [_InterceptHandler()]
+ _ROOT_LOGGER.setLevel(logging.DEBUG)
+ _configure_noisy_loggers()
</code_context>
<issue_to_address>
**issue (bug_risk):** Overwriting root handlers can clobber existing logging configuration in embedding applications.
Assigning `[_InterceptHandler()]` to `_ROOT_LOGGER.handlers` (plus the `handlers.clear()` path for std logging) wipes out any handlers configured by the host process or other libraries, making this change globally invasive. In environments where logging is preconfigured (frameworks, notebooks, multi-service apps), consider appending the handler only when missing or guarding the reset behind an explicit opt-in flag to avoid unexpectedly discarding existing setups.
</issue_to_address>
### Comment 2
<location path="agent/utils/logger.py" line_range="190-193" />
<code_context>
+ _configure_noisy_loggers()
- return _loguru_logger
+ app_logger = get_logger(_APP_LOGGER_NAME)
+ app_logger.setLevel(logging.NOTSET)
+ app_logger.propagate = True
+ return app_logger
</code_context>
<issue_to_address>
**question (bug_risk):** Changing `setup_logger` to return an app `logging.Logger` instead of the underlying loguru logger may break call sites relying on loguru APIs.
Previously `setup_logger()` returned the loguru logger (`_loguru_logger`), exposing loguru-specific methods like `.bind` and `.catch`. It now returns a stdlib `logging.Logger`, so any callers that stored this value and used loguru APIs will break or change behavior. If callers depend on that richer API, consider either preserving the original return type or splitting responsibilities (e.g., a `get_app_logger()` plus a `setup_logging()` that returns `None`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Owner
|
加一下开发群喵 1092630280 |
Contributor
Author
加了喵 |
Collaborator
|
其实应该把所有用agent\custom\action\Common\logger.py的地方换成agent\utils\logger.py，之前用的算是历史遗留问题（没发现从m9a那里copy来了logger |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
原本日志体系有两条链路 ，
agent/custom/action/Common/logger.py中额外维护了一套独立日志输出，持续写入debug/agent.log，这可能造成同类日志重复记录且该链路没有轮转与保留策略，导致仅使用几天后debug文件夹大小持续增长至50GB所以将
custom action的独立日志并入统一日志体系，复用agent/utils/logger.py中已有的轮转与保留策略Summary by Sourcery
将自定义动作日志记录统一到共享的应用程序日志记录器中，以减少重复的日志写入，并利用现有的轮转/保留策略。
Enhancements:
get_logger帮助函数集中获取日志记录器，并使用一致的应用程序日志记录器名称。Original summary in English
Summary by Sourcery
Unify custom action logging with the shared application logger to reduce duplicate log writes and leverage existing rotation/retention policies.
Enhancements: