Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Dec 15, 2025

Closes goravel/goravel#535

Changes Made

Contracts (contracts/log/)

  • level.go: Custom levels (LevelPanic, LevelFatal) defined relative to slog.Level, implements slog.Leveler interface
  • log.go: Removed Hook interface; Logger.Handle() now returns log.Handler; added Hook to Handler adapter for backward compatibility

Implementation (log/)

  • application.go: Uses slog-multi.Fanout() for multi-handler support
  • writer.go: New slog-based writer (replaces logrus_writer.go)
  • logger/handler.go: IOHandler and ConsoleHandler implementing log.Handler
    • Format timestamp using carbon (ToDateTimeMilliString)
    • Only print [With] and [Tags] sections when they have entries
    • ConsoleHandler accepts level parameter from configuration
  • logger/single.go, daily.go: Return log.Handler instead of logrus.Hook; export GetLevelFromString

Tests

  • Comprehensive unit tests for log/logger/handler.go, single.go, daily.go
  • Unit tests for log/entry.go and log/utils.go using testify suite
  • Table-driven test cases for telemetry handler tests
  • Entry field tests with specific content assertions for all fields (Code, Context, Domain, Hint, Owner, Request, Response, Tags, User, With)

Removed

  • log/hook.go, log/formatter/general.go
  • mocks/log/Hook.go

Dependencies

  • Added: github.com/samber/slog-multi
  • Removed: github.com/sirupsen/logrus, github.com/rifflock/lfshook

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI changed the title [WIP] Refactor log module to replace logrus with log/slog Refactor log module from logrus to log/slog with multi-handler architecture Dec 15, 2025
Copilot AI requested a review from devhaozi December 15, 2025 10:54
@devhaozi
Copy link
Member

@copilot 然后这些功能现在是不是用不了了?
写入特定通道
有时,你可能希望将消息记录到应用程序默认频道以外的频道:

facades.Log().Channel("single").Info(message)
如果你想同时写入多个通道,可以使用 Stack 方法:

facades.Log().Stack([]string{"single", "slack"}).Info(message)

Goravel 提供有便捷的链式方式,方便在日志中插入更多有用信息:

facades.Log().User("John").Debug(message)
Method Action
Code 设置日志代码。
Hint 设置提示,方便调试。
In 设置日志相关的分类。
Owner 设置日志归属人。
Request 记录 Request。
Response 记录 Response。
Tags 为日志添加标签。
User 记录触发日志的用户。
With 为日志附加数据。
WithTrace 为日志附加堆栈信息。

用不了的话得加回去

Copy link
Contributor Author

Copilot AI commented Dec 15, 2025

@copilot 然后这些功能现在是不是用不了了?
写入特定通道
有时,你可能希望将消息记录到应用程序默认频道以外的频道:...

这些功能都还能用:

  1. Channel() - 在 application.go 第 58-71 行实现
  2. Stack() - 在 application.go 第 73-94 行实现
  3. 所有链式方法 (Code, Hint, In, Owner, Request, Response, Tags, User, With, WithTrace) - 全部在 writer.go 第 120-177 行实现

测试都通过了,包括专门针对这些方法的测试用例(如 TestWriter/Code, TestWriter/Hint 等)。

@devhaozi
Copy link
Member

@copilot 还有一点需要重构的:CustomDriver之类的都重构为DriverCustom,DebugLevel之类的都重构为LevelDebug

@devhaozi
Copy link
Member

@copilot 改完不用测试了,提交更改吧

@codecov
Copy link

codecov bot commented Dec 15, 2025

Codecov Report

❌ Patch coverage is 89.40678% with 50 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.99%. Comparing base (c4009cf) to head (64aa876).
⚠️ Report is 29 commits behind head on master.

Files with missing lines Patch % Lines
log/application.go 70.83% 16 Missing and 5 partials ⚠️
log/logger/handler.go 74.32% 18 Missing and 1 partial ⚠️
log/logger/single.go 71.42% 2 Missing and 2 partials ⚠️
log/logger/utils.go 92.30% 1 Missing and 1 partial ⚠️
log/writer.go 98.03% 1 Missing and 1 partial ⚠️
log/logger/daily.go 90.90% 1 Missing ⚠️
telemetry/instrumentation/log/handler.go 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1302      +/-   ##
==========================================
+ Coverage   69.41%   69.99%   +0.58%     
==========================================
  Files         282      282              
  Lines       16630    16659      +29     
==========================================
+ Hits        11543    11660     +117     
+ Misses       4596     4511      -85     
+ Partials      491      488       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@devhaozi devhaozi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

… level from config, add Entry field tests

Co-authored-by: devhaozi <[email protected]>
Copy link
Contributor Author

Copilot AI commented Dec 23, 2025

@copilot Please consider the comments above and solve them.

Addressed in 04da2a6:

  • Format timestamp using carbon.FromStdTime().ToDateTimeMilliString() instead of UnixMilli
  • Fix empty [With] map[] and [Tags] [] by checking length before printing
  • ConsoleHandler now accepts level parameter from configuration
  • Add comprehensive Entry field tests (TestHandleWithAllFields, TestHandleEmptyOptionalFields)
  • Replace Maybe() with specific Times() or Once() expectations in application_test.go

@devhaozi
Copy link
Member

@hwbrzzl PTAL

Copy link
Contributor

@hwbrzzl hwbrzzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, only two nitpicks.

Comment on lines 8 to 16
// getContextValues gets all key-value pairs from the context
// TODO: This method has a high performance overhead; is it necessary to retain it?
func getContextValues(ctx any, values map[any]any) {
contextValues := reflect.Indirect(reflect.ValueOf(ctx))
contextKeys := reflect.TypeOf(ctx)
if contextKeys.Kind() == reflect.Ptr {
contextKeys = contextKeys.Elem()
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but it's required I think.

auto-merge was automatically disabled December 24, 2025 08:02

Head branch was pushed to by a user without write access

Copy link
Contributor

@hwbrzzl hwbrzzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, LGTM 💯

@devhaozi devhaozi merged commit 383159b into master Dec 24, 2025
15 checks passed
@devhaozi devhaozi deleted the copilot/refactor-logrus-to-slog-again branch December 24, 2025 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

✨ [Feature] Replace github.com/sirupsen/logrus with log/slog

4 participants