[BugFix] Fix legacy logger propagation leaking unformatted logs to stderr#7617
[BugFix] Fix legacy logger propagation leaking unformatted logs to stderr#7617gongweibao wants to merge 1 commit intoPaddlePaddle:developfrom
Conversation
…derr Legacy loggers (legacy.*) had propagate=True (commented out since PaddlePaddle#3431), causing all log levels to propagate to the root logger and output to stderr without timestamps. This made downstream log parsers unable to extract timestamps from stderr log files. Set propagate=False for both get_trace_logger and _get_legacy_logger so logs only go through their configured handlers with proper formatting. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Thanks for your contribution! |
There was a problem hiding this comment.
Pull request overview
该 PR 修复 get_trace_logger() / _get_legacy_logger() 创建的 legacy.* logger 默认向 root logger 传播,导致日志以 root logger 的默认格式泄漏到 stderr(缺少期望的格式/时间戳)的问题,使 legacy logger 的输出与 FastDeploy 的日志体系隔离。
Changes:
- 在
get_trace_logger()与_get_legacy_logger()中显式设置logger.propagate = False - 更新单元测试断言,使其与新的传播行为一致
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| fastdeploy/logger/logger.py | 禁用 legacy logger 向 root logger 的传播,避免 stderr 被未格式化日志污染 |
| tests/logger/test_logger.py | 更新 legacy logger 的 propagate 断言以匹配修复后的行为 |
Comments suppressed due to low confidence (1)
tests/logger/test_logger.py:88
- 本次同时修改了
get_trace_logger()的logger.propagate行为(改为 False),但当前测试只覆盖了_get_legacy_logger()的 propagate。建议在test_get_trace_logger_basic(或单独用例)里也断言logger.propagate为 False,避免后续回归。
legacy_logger = self.logger._get_legacy_logger("test", "test.log")
self.assertFalse(legacy_logger.propagate)
def test_get_trace_logger_basic(self):
"""Test basic functionality of get_trace_logger"""
logger = self.logger.get_trace_logger("test_trace", "trace_test.log")
| @@ -246,8 +246,7 @@ def get_trace_logger(self, name, file_name, without_formater=False, print_to_con | |||
| logger.addHandler(console_handler) | |||
| console_handler.propagate = False | |||
There was a problem hiding this comment.
这里对 console_handler 设置 propagate 没有实际效果:propagate 是 logging.Logger 的属性,不是 logging.Handler 的属性。该行会让人误以为 handler 能控制传播,建议删除该行,统一由 logger.propagate = False 控制。
| console_handler.propagate = False |
| @@ -311,8 +310,7 @@ def _get_legacy_logger(self, name, file_name, without_formater=False, print_to_c | |||
| logger.addHandler(console_handler) | |||
| console_handler.propagate = False | |||
There was a problem hiding this comment.
这里对 console_handler 设置 propagate 没有实际效果:propagate 是 logging.Logger 的属性,不是 logging.Handler 的属性。建议删除该行,避免误导,统一由 logger.propagate = False 控制。
| console_handler.propagate = False |
PaddlePaddle-bot
left a comment
There was a problem hiding this comment.
🤖 AI Code Review |
2026-04-24 21:37:03
📋 Review 摘要
PR 概述:修复 legacy.* 命名空间下的 logger 因 propagate=True 导致日志泄漏到 stderr 的问题。
变更范围:fastdeploy/logger/logger.py、tests/logger/test_logger.py
影响面 Tag:BugFix
问题
| 级别 | 文件 | 概述 |
|---|---|---|
| 🟡 建议 | tests/logger/test_logger.py:84 |
get_trace_logger() 同样被修复,但缺少对应的 propagate=False 测试断言 |
总体评价
修复思路正确,根因分析清晰,PR 描述详尽。仅建议在测试中补充 get_trace_logger() 的 propagate 断言,保持两处修复的测试覆盖对称。
| """Test log propagation settings""" | ||
| legacy_logger = self.logger._get_legacy_logger("test", "test.log") | ||
| self.assertTrue(legacy_logger.propagate) | ||
| self.assertFalse(legacy_logger.propagate) |
There was a problem hiding this comment.
🟡 建议 get_trace_logger() 缺少 propagate 测试覆盖
本次 PR 同时修复了 get_trace_logger() 和 _get_legacy_logger() 中的 propagate 设置,但测试用例 test_logger_propagate 仅验证了 _get_legacy_logger() 的行为,get_trace_logger() 缺少对应断言。
建议补充:
def test_logger_propagate(self):
"""Test log propagation settings"""
legacy_logger = self.logger._get_legacy_logger("test", "test.log")
self.assertFalse(legacy_logger.propagate)
# Also verify get_trace_logger
trace_logger = self.logger.get_trace_logger("test_trace", "test_trace.log")
self.assertFalse(trace_logger.propagate)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #7617 +/- ##
==========================================
Coverage ? 71.67%
==========================================
Files ? 419
Lines ? 57856
Branches ? 9077
==========================================
Hits ? 41467
Misses ? 13561
Partials ? 2828
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Motivation
Legacy loggers (
legacy.*) created byget_trace_logger()and_get_legacy_logger()hadpropagateleft asTrue(commented out since PR #3431). This caused all log levels (INFO/WARNING/ERROR) to propagate to the Python root logger, which outputs to stderr using the defaultLEVEL:name:messageformat — without timestamps.Impact:
*-stderr.log) are polluted with INFO-level logs that shouldn't be therepaddlerl diagnose) to display(no ts)and fail to filter events by timeRoot cause: PR #3431 introduced the
legacy.*namespace (logging.getLogger(f"legacy.{name}")) but keptlogger.propagate = Falsecommented out with the note "maintain original logic". The original code didn't need it because loggers were at the top level. After the namespace change,legacy.api_serverbecame a child of the root logger, and propagation leaked all logs to stderr.Modifications
logger.propagate = Falsein bothget_trace_logger()and_get_legacy_logger()assertTruetoassertFalseto match the fixUsage or Command
No usage change. After this fix:
%(asctime)stimestamps)Accuracy Tests
N/A — logging-only change, no impact on model outputs.
Checklist
pre-commitbefore commit.releasebranch, make sure the PR has been submitted to thedevelopbranch, then cherry-pick it to thereleasebranch with the[Cherry-Pick]PR tag.