Skip to content

feat: 使用 zap 重构日志体系并统一日志调用方式#963

Open
SisyphusSQ wants to merge 4 commits intoalibaba:developfrom
SisyphusSQ:develop
Open

feat: 使用 zap 重构日志体系并统一日志调用方式#963
SisyphusSQ wants to merge 4 commits intoalibaba:developfrom
SisyphusSQ:develop

Conversation

@SisyphusSQ
Copy link
Copy Markdown
Contributor

背景

当前 MongoShake 仍大量直接依赖 third_party/log4go,日志调用方式分散,后续维护和统一演进成本较高。

本 PR 在尽量保持现有外部日志行为稳定的前提下,将日志底座切换为基于 zappkg/log,并完成主流程调用收敛。

本次改动

  • 新增 pkg/log,以 zap 作为新的日志实现底座
  • 迁移主流程代码,不再直接依赖 third_party/log4go
  • 统一日志调用方式,收敛到 Infof / Debugf / Warnf / Errorf 等格式化接口
  • 保留 verbose 三态语义
    • 0:仅文件
    • 1:文件 + stdout
    • 2:仅 stdout
  • 保留 log.flush 配置语义
  • 新增 log.max_size_mb / log.max_age,将轮转策略收敛为按大小轮转、按天数清理
  • 删除仓库内 third_party/log4go 目录及相关直接引用
  • 补充 pkg/log 定点测试,覆盖输出格式、默认文件名、panic 前刷盘、初始化前安全调用等回归点

兼容性处理

  • cmd/collector 保持先 SanitizeOptions() 再初始化 logger,避免破坏 log.file / log.level 默认值语义
  • 全局 Logger 提供默认 nop logger,避免初始化前日志调用触发空指针
  • Crash/Crashf 相关关键路径保持 critical -> flush -> panic 的兼容方向,不直接退化为 zap 原生 Fatal
  • 默认输出仍保持纯文本风格,不引入 JSON 默认输出

配置与文档

  • 已同步 conf/collector.conf
  • 已同步 conf/receiver.conf
  • 本轮 review 修正未新增额外正式文档需求

验证

  • git diff --check origin/develop...HEAD
  • go test -count=1 ./pkg/log
  • go test -count=1 ./cmd/collector ./cmd/receiver
  • go test -count=1 ./common -run TestBlockMongoUrlPassword
  • go test -count=1 ./receiver ./receiver/configure ./modules ./quorum
  • go test -count=1 ./tunnel/kafka

说明

  • 当前仓库不以 go test ./... 作为默认完成标准
  • 原因是 collector/filter 等路径存在已知历史测试噪音和环境依赖,不属于本 PR 新引入问题

关联ISSUE:#959

@zhongli-james
Copy link
Copy Markdown
Collaborator

我之前有一些待PR的分支提交(包括上次你PR里会导致部分unit test跑不过等)。估计会有一些冲突,辛苦解决冲突后再提一下呢。抱歉

@SisyphusSQ
Copy link
Copy Markdown
Contributor Author

done,再看看。每次开发,我会同步这个仓库的代码的,看样子就一个差异点

@zhongli-james
Copy link
Copy Markdown
Collaborator

PR 方向正确,完成度高。86 个文件的迁移工作量大且机械替换部分无明显遗漏。以下是需要关注的优化点:

  1. bufferedWriteSyncer 缺少定时刷盘机制
type bufferedWriteSyncer struct {
    mu     sync.Mutex
    buffer *bufio.Writer
    target zapcore.WriteSyncer
}

问题:当 log.flush = false 时,日志写入 buffer 后只有在 Sync() 被调用时才刷盘。如果进程异常退出(SIGKILL / OOM)但不触发 panic 或正常 shutdown,buffer 中的日志将永久丢失。
原 log4go 的 channel-based 模型虽然也有这个问题,但 channel 的 buffer 只有 32 条消息(很快会满触发写入)。而这里是 32KB 的字节 buffer,可能积压更多数据。
建议:添加一个后台 goroutine 做定时 flush(例如每 1-3 秒),或直接使用 zap 官方的 zapcore.BufferedWriteSyncer(zap v1.21+ 内置,支持 FlushInterval):

&zapcore.BufferedWriteSyncer{
    WS:            fileSyncer,
    Size:          32 * 1024,
    FlushInterval: 3 * time.Second,
}
  1. Panicf / Crash 的 panic value 不一致
func (l *ZapLogger) Panicf(format string, args ...any) {
    message := fmt.Sprintf(format, args...)
    l.write(zapcore.PanicLevel, message)
    _ = l.Sync()
    panic(message)  // panic value 是 string
}

func (l *ZapLogger) Panic(args ...any) {
    message := formatArgs(args...)
    l.write(zapcore.PanicLevel, message)
    _ = l.Sync()
    panic(args)  // panic value 是 []any !
}

问题:Panic(args ...any) 的 panic value 是 args(一个 slice),而原 log4go 的 Crash(args ...interface{}) 也是 panic(args)。但 Panicf panic 的是 string。这造成了上层 recover 时类型不一致。另外,调用方存在 l.Logger.Panicf("%v", err) 替代原来的 LOG.Crash(err) 的模式。原来 recover 到的是 []interface{}{err},现在是格式化后的 string。如果有地方做了 recover().(error) 类型断言,会 panic。
建议:统一 panic value 为 string,或者让 Crash(args) 保持与旧行为一致(panic 原始参数),并在 PR 描述中明确标注这一行为变更。

  1. Criticalf 使用 DPanicLevel 语义模糊
func (l *ZapLogger) Criticalf(format string, args ...any) {
    l.write(zapcore.DPanicLevel, message)
}

问题:DPanicLevel 在 zap 中的设计意图是"Development Panic"——开发环境下 panic,生产环境仅记录。但这里 Criticalf 被用作纯日志记录(不 panic),用 DPanicLevel 可能在某些 zap 配置下意外触发 panic(如果 logger 是 Development 模式创建的)。
建议:Criticalf 使用 zapcore.ErrorLevel 或自定义一个 CriticalLevel(zap 支持自定义 level),或者至少确保 New() 中创建的 logger 不是 zap.NewDevelopment()。

  1. 旧 Warn / Error / Critical 返回 error 的语义被静默丢弃
    原代码中大量存在:
_ = LOG.Critical("Connect to mongo cluster failed. %v", err)
新代码```go
l.Logger.Criticalf("Connect to mongo cluster failed. %v", err)

问题:原 log4go 的 Warn/Error/Critical 返回 error,虽然绝大多数调用方都 _ = 忽略了,但如果有调用方依赖返回值(如 return LOG.Error(...)),迁移后会编译报错——这确实通过编译验证了。但 PR 描述中没有明确提及这一 API 差异,建议补充。

这里我理解没问题,可忽略。

  1. common/common.go 中 InitialLogger 保留但逻辑有冗余
func InitialLogger(logDir, logFile, level string, logFlush bool, verbose int) error {
    return InitialLoggerWithRotation(logDir, logFile, level, logFlush, verbose, 20, 7)
}

func InitialLoggerWithRotation(logDir, logFile, level string,
    logFlush bool, verbose, maxSizeMB, maxAge int) error {
    if logDir == "" {
        logDir = "logs"
    }
    if verbose != 2 {
        if _, err := os.Stat(logDir); err != nil && os.IsNotExist(err) {
            if err := os.MkdirAll(logDir, os.ModeDir|os.ModePerm); err != nil {
                return fmt.Errorf("create log.dir[%v] failed[%v]", logDir, err)
            }
        }
    }
    return l.New(level, logDir, logFile, logFlush, maxSizeMB, maxAge, verbose)
}

问题:InitialLoggerWithRotation 中做了 MkdirAll,但 pkg/log.New() 内部也做了 os.MkdirAll(见 logger.go 中 if verbose == 0 || verbose == 1 分支)。目录创建逻辑被重复执行了两次。
建议:移除 InitialLoggerWithRotation 中的目录创建,让 pkg/log.New() 完全负责;或者反过来,让 New() 不做目录创建,由调用方保证。不要两边都做。

  1. logFile 默认值处理分散在多处
- cmd/collector/sanitize.go:if conf.Options.LogFileName == "" { conf.Options.LogFileName = "mongoshake.log" }
- pkg/log/logger.go:func normalizeLogFile(logFile string) string { if logFile == "" { return defaultLogFile } }
- common/common.go:InitialLoggerWithRotation 中 logDir 有默认处理

问题:默认值逻辑散落三处。如果 sanitize 设置了默认值,pkg/log 的 normalize 就永远不会触发(冗余);如果 sanitize 没跑(比如 receiver 路径),则依赖 pkg/log 内部的 normalize。
建议:将默认值处理收敛到 pkg/log.New() 内部,调用方传入空字符串即代表使用默认值。sanitize 中只需做「配置文件读取到的值是否合法」的校验,不需要填默认值。

  1. write() 方法绕过了 zap 的正常调用链
func (l *ZapLogger) write(level zapcore.Level, message string) {
    core := l.logger.Desugar().Core()
    if !core.Enabled(level) {
        return
    }
    _ = core.Write(zapcore.Entry{
        Level:   level,
        Time:    time.Now(),
        Message: message,
    }, nil)
}

问题:

  • 每次调用 Desugar() 会创建新的 *zap.Logger 实例(虽然开销不大但不必要)
  • 直接调用 core.Write() 绕过了 zap 的 Check() 机制和 hook 链
  • Panicf / Fatalf 中先 write() 再手动 Sync() + panic()/os.Exit(),但 Criticalf 只 write() 不 sync——如果后续有人误以为 Critical 也会 flush 就会丢日志
    建议:缓存 Desugar() 的结果(在 New() 时存储),或者对 Criticalf 直接用 l.logger.Error(message) 即可(zap 会正确走完整链路)。
  1. 测试覆盖的盲区
    测试覆盖了:输出格式、默认文件名、panic 前刷盘、初始化前安全调用、verbose=2(stdout only)。未覆盖的关键场景:
  • verbose=1(文件+stdout 双输出):是否真的写入了两个目标?
  • log.flush = false 时 buffered writer 的行为:写入后立即读文件应为空,Sync 后才有内容
  • Fatalf 的行为(会 os.Exit,需要用子进程测试)
  • 日志级别过滤:设置 info 级别后 Debugf 不应输出
  1. 导入别名 l 可读性较低
    所有文件统一使用 l "github.com/alibaba/MongoShake/v2/pkg/log" 作为导入别名,调用形式为 l.Logger.Infof(...)。问题:l 作为包别名太短,容易与局部变量冲突(如 for l := range ...),且在代码中 l.Logger.Xxx 的可读性不如 log.Logger.Xxx 或直接提供包级函数 log.Infof()。

建议:
方案 A:提供包级函数 log.Infof() / log.Errorf() 等,减少调用层级
方案 B:使用更长的别名如 mlog 或 logger

  1. lumberjack 版本使用了不稳定标记
    plaintext
    github.com/natefinch/lumberjack v2.0.0+incompatible
    问题:+incompatible 表示该模块未正确支持 Go modules。lumberjack 官方的 module path 是 gopkg.in/natefinch/lumberjack.v2。建议:使用 gopkg.in/natefinch/lumberjack.v2 替代,这是维护更活跃、Go modules 兼容的版本。

总结

优先级 问题 风险
P0 bufferedWriteSyncer 无定时刷盘 异常退出丢日志
P0 Panic value 类型不一致 上层 recover 可能二次 panic
P1 Criticalf 使用 DPanicLevel 潜在 panic 风险
P1 lumberjack +incompatible 版本 依赖稳定性
P2 目录创建逻辑重复 代码质量
P2 默认值处理分散 维护成本
P2 write() 绕过 zap 正常链路 未来扩展性
P3 导入别名可读性 代码风格
P3 测试盲区 回归风险

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