feat(log): add slog-backed Logger type #22347
Conversation
The standard library's log/slog package was released after the SDK added its standardized Logger interface. It is reasonable to expect new Go projects to depend on log/slog rather than zerolog or any other third party logging implementations. The existing Logger interface maps cleanly to the log/slog API. This change adds a new cosmossdk.io/log/slog package that exports a single Logger type, backed by an *slog.Logger. Currently, we expect the caller to provide a fully configured *log/slog.Logger and pass it to cosmossdk.io/log/slog.FromSlog. There is no reason we can't have a New function to produce a wrapped slog logger, but that seems out of scope of the initial implementation.
mark-rushakoff
requested review from
a team,
julienrbrt and
sontrinh16
as code owners
This comment has been minimized.
This comment has been minimized.
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve updates to the logging system within the Cosmos SDK. A new logger implementation using the standard library's Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (3)
log/go.mod (2)
Line range hint
3-3: Go version must be updated to 1.21 or higherThe PR introduces slog-backed Logger, but slog was introduced in Go 1.21. The current Go version (1.20) doesn't support slog and will cause compilation errors.
Apply this diff:
-go 1.20 +go 1.21
9-9: Consider updating testify to v1.8.4The current version (v1.8.1) is a bit outdated. Version 1.8.4 includes several bug fixes and improvements.
Apply this diff:
- github.com/stretchr/testify v1.8.1 + github.com/stretchr/testify v1.8.4log/slog/logger.go (1)
13-17: Consider enhancing type documentation.While the documentation is good, it could be more helpful to add an example usage.
Add a documentation example:
// Logger satisfies [log.Logger] with logging backed by // an instance of [*slog.Logger]. +// +// Example: +// +// slogger := slog.New(slog.NewTextHandler(os.Stdout, nil)) +// logger := slog.FromSlog(slogger) +// logger.Info("message", "key", "value") type Logger struct {
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
log/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
- log/CHANGELOG.md (1 hunks)
- log/README.md (1 hunks)
- log/go.mod (1 hunks)
- log/slog/logger.go (1 hunks)
- log/slog/logger_test.go (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- log/README.md
🧰 Additional context used
📓 Path-based instructions (3)
log/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"log/slog/logger.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.log/slog/logger_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (5)
log/slog/logger.go (3)
1-9: LGTM! Well-documented package with clean imports.The package documentation is clear and follows Go standards with proper documentation references.
11-11: LGTM! Proper interface compliance check.Good use of compile-time interface compliance check.
44-47: LGTM! Well-documented implementation accessor.The Impl method provides clear access to the underlying logger with proper documentation.
log/slog/logger_test.go (1)
1-11: LGTM! Well-structured package and imports.The package name and import organization follow Go best practices. Good use of aliasing for the standard
slogpackage to avoid naming conflicts.log/CHANGELOG.md (1)
28-28: LGTM! The changelog entry is well-formatted and accurate.The new entry follows the changelog guidelines, is placed in the appropriate section, and accurately describes the addition of the slog-backed logger feature.
| func (l Logger) Info(msg string, keyVals ...any) { | ||
| l.log.Info(msg, keyVals...) | ||
| } | ||
|
|
||
| func (l Logger) Warn(msg string, keyVals ...any) { | ||
| l.log.Warn(msg, keyVals...) | ||
| } | ||
|
|
||
| func (l Logger) Error(msg string, keyVals ...any) { | ||
| l.log.Error(msg, keyVals...) | ||
| } | ||
|
|
||
| func (l Logger) Debug(msg string, keyVals ...any) { | ||
| l.log.Debug(msg, keyVals...) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider validating key-value pairs in logging methods.
The logging methods accept variadic key-value pairs but don't validate that they come in pairs.
Add a helper function for validation:
func validateKeyVals(keyVals ...any) {
if len(keyVals)%2 != 0 {
panic("odd number of key-value pairs")
}
}Then use it in each method, for example:
func (l Logger) Info(msg string, keyVals ...any) {
+ validateKeyVals(keyVals...)
l.log.Info(msg, keyVals...)
}
log/slog/logger.go
Outdated
| // FromSlog returns a Logger backed by an existing slog.Logger instance. | ||
| func FromSlog(log *slog.Logger) Logger { | ||
| return Logger{log: log} | ||
| } |
There was a problem hiding this comment.
Add parameter validation in constructor.
The constructor should validate that the input logger is not nil to prevent potential panics.
func FromSlog(log *slog.Logger) Logger {
+ if log == nil {
+ panic("nil slog.Logger")
+ }
return Logger{log: log}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // FromSlog returns a Logger backed by an existing slog.Logger instance. | |
| func FromSlog(log *slog.Logger) Logger { | |
| return Logger{log: log} | |
| } | |
| // FromSlog returns a Logger backed by an existing slog.Logger instance. | |
| func FromSlog(log *slog.Logger) Logger { | |
| if log == nil { | |
| panic("nil slog.Logger") | |
| } | |
| return Logger{log: log} | |
| } |
There was a problem hiding this comment.
@julienrbrt, glad you agree!
(_/)
(•ᴗ•)
(")_(")
| func (l Logger) With(keyVals ...any) log.Logger { | ||
| return Logger{log: l.log.With(keyVals...)} | ||
| } |
There was a problem hiding this comment.
Add validation in With method.
The With method should also validate key-value pairs.
func (l Logger) With(keyVals ...any) log.Logger {
+ validateKeyVals(keyVals...)
return Logger{log: l.log.With(keyVals...)}
}Committable suggestion was skipped due to low confidence.
log/slog/logger_test.go
Outdated
| var line logLine | ||
|
|
||
| logger.Debug("Message one", "num", 1) | ||
| require.NoError(t, json.Unmarshal(buf.Bytes(), &line)) | ||
| require.Equal(t, logLine{ | ||
| Level: stdslog.LevelDebug.String(), | ||
| Msg: "Message one", | ||
| Num: 1, | ||
| }, line) | ||
|
|
||
| buf.Reset() | ||
| logger.Info("Message two", "num", 2) | ||
| require.NoError(t, json.Unmarshal(buf.Bytes(), &line)) | ||
| require.Equal(t, logLine{ | ||
| Level: stdslog.LevelInfo.String(), | ||
| Msg: "Message two", | ||
| Num: 2, | ||
| }, line) | ||
|
|
||
| buf.Reset() | ||
| logger.Warn("Message three", "num", 3) | ||
| require.NoError(t, json.Unmarshal(buf.Bytes(), &line)) | ||
| require.Equal(t, logLine{ | ||
| Level: stdslog.LevelWarn.String(), | ||
| Msg: "Message three", | ||
| Num: 3, | ||
| }, line) | ||
|
|
||
| buf.Reset() | ||
| logger.Error("Message four", "num", 4) | ||
| require.NoError(t, json.Unmarshal(buf.Bytes(), &line)) | ||
| require.Equal(t, logLine{ | ||
| Level: stdslog.LevelError.String(), | ||
| Msg: "Message four", | ||
| Num: 4, | ||
| }, line) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test robustness with additional test cases.
While the current tests cover basic logging functionality, consider the following improvements:
- Convert to table-driven tests to reduce code duplication
- Add error cases (e.g., invalid log levels)
- Test WithLevel functionality
- Validate behavior with nil/empty attributes
Here's a suggested table-driven test approach:
func TestSlog(t *testing.T) {
tests := []struct {
name string
level slog.Level
msg string
num int
expected logLine
}{
{
name: "debug message",
level: slog.LevelDebug,
msg: "Message one",
num: 1,
expected: logLine{
Level: slog.LevelDebug.String(),
Msg: "Message one",
Num: 1,
},
},
// Add more test cases
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
var buf bytes.Buffer
logger := setupLogger(&buf)
switch tt.level {
case slog.LevelDebug:
logger.Debug(tt.msg, "num", tt.num)
// Add other cases
}
var line logLine
require.NoError(t, json.Unmarshal(buf.Bytes(), &line))
require.Equal(t, tt.expected, line)
})
}
}
|
hmmm maybe adding a new function to wrapped the slog is fine |
|
or you going to do it in a follow up PR right ? @mark-rushakoff |
Yes, I think there will be one followup PR to be able to use this with the server/v2 code. Although that may overlap with the work in #22267. |
This is more in line with the existing zerolog-focused API. Also update a couple more comments, and add a test for the With method.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
- log/README.md (1 hunks)
- log/slog/logger.go (1 hunks)
- log/slog/logger_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- log/README.md
🧰 Additional context used
📓 Path-based instructions (2)
log/slog/logger.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.log/slog/logger_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (7)
log/slog/logger.go (5)
1-9: LGTM! Clear package documentation and clean imports.The package documentation clearly describes the purpose and the imports are minimal and necessary.
11-17: LGTM! Clean type definition with interface compliance check.Good practice using var assertion to verify interface compliance at compile time.
19-25: LGTM! Constructor follows existing naming patterns.The name
NewCustomLoggeraligns with existing patterns in the codebase, and the documentation clearly states the caller's responsibilities.
27-41: LGTM! Consistent implementation across logging methods.The logging methods provide a clean pass-through to the underlying slog.Logger with consistent implementation patterns.
47-50: LGTM! Well-documented implementation accessor.The
Implmethod follows Go conventions for providing access to the underlying implementation.log/slog/logger_test.go (2)
1-10: LGTM! Well-organized imports and package declaration.The package is correctly declared as
slog_testfor external testing, and imports are properly organized with appropriate aliasing.
19-23: Previous comment about expanding logLine struct is still applicable.
| func TestSlog(t *testing.T) { | ||
| var buf bytes.Buffer | ||
| h := stdslog.NewJSONHandler(&buf, &stdslog.HandlerOptions{ | ||
| Level: stdslog.LevelDebug, | ||
| }) | ||
| logger := slog.NewCustomLogger(stdslog.New(h)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify interface compliance and enhance test setup.
Consider adding interface compliance verification and extracting the logger setup into a helper function:
func TestLoggerImplementsInterface(t *testing.T) {
var _ log.Logger = (*slog.Logger)(nil)
}
func setupTestLogger() (*bytes.Buffer, *slog.Logger) {
buf := &bytes.Buffer{}
h := stdslog.NewJSONHandler(buf, &stdslog.HandlerOptions{
Level: stdslog.LevelDebug,
})
return buf, slog.NewCustomLogger(stdslog.New(h))
}| var line logLine | ||
|
|
||
| logger.Debug("Message one", "num", 1) | ||
| if err := json.Unmarshal(buf.Bytes(), &line); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if want := (logLine{ | ||
| Level: stdslog.LevelDebug.String(), | ||
| Msg: "Message one", | ||
| Num: 1, | ||
| }); want != line { | ||
| t.Fatalf("unexpected log record: want %v, got %v", want, line) | ||
| } | ||
|
|
||
| buf.Reset() | ||
| logger.Info("Message two", "num", 2) | ||
| if err := json.Unmarshal(buf.Bytes(), &line); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if want := (logLine{ | ||
| Level: stdslog.LevelInfo.String(), | ||
| Msg: "Message two", | ||
| Num: 2, | ||
| }); want != line { | ||
| t.Fatalf("unexpected log record: want %v, got %v", want, line) | ||
| } | ||
|
|
||
| buf.Reset() | ||
| logger.Warn("Message three", "num", 3) | ||
| if err := json.Unmarshal(buf.Bytes(), &line); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if want := (logLine{ | ||
| Level: stdslog.LevelWarn.String(), | ||
| Msg: "Message three", | ||
| Num: 3, | ||
| }); want != line { | ||
| t.Fatalf("unexpected log record: want %v, got %v", want, line) | ||
| } | ||
|
|
||
| buf.Reset() | ||
| logger.Error("Message four", "num", 4) | ||
| if err := json.Unmarshal(buf.Bytes(), &line); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if want := (logLine{ | ||
| Level: stdslog.LevelError.String(), | ||
| Msg: "Message four", | ||
| Num: 4, | ||
| }); want != line { | ||
| t.Fatalf("unexpected log record: want %v, got %v", want, line) | ||
| } | ||
|
|
||
| wLogger := logger.With("num", 5) | ||
| buf.Reset() | ||
| wLogger.Info("Using .With") | ||
|
|
||
| if err := json.Unmarshal(buf.Bytes(), &line); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if want := (logLine{ | ||
| Level: stdslog.LevelInfo.String(), | ||
| Msg: "Using .With", | ||
| Num: 5, | ||
| }); want != line { | ||
| t.Fatalf("unexpected log record: want %v, got %v", want, line) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test cases for error conditions and edge cases.
While basic logging functionality is covered, consider adding tests for:
- Invalid key-value pairs (odd number of arguments)
- Empty or nil messages
- Special characters in messages
- Timestamp presence and format validation
- Concurrent logging safety
Example test case for invalid arguments:
func TestLoggerInvalidInput(t *testing.T) {
buf := &bytes.Buffer{}
logger := setupTestLogger(buf)
// Should not panic with odd number of kvs
logger.Info("test", "key") // missing value
// Should handle nil/empty values
logger.Info("", nil, "key", nil)
}
* main: (157 commits) feat(core): add ConfigMap type (#22361) test: migrate e2e/genutil to systemtest (#22325) feat(accounts): re-introduce bundler (#21562) feat(log): add slog-backed Logger type (#22347) fix(x/tx): add feePayer as signer (#22311) test: migrate e2e/baseapp to integration tests (#22357) test: add x/circuit system tests (#22331) test: migrate e2e/tx tests to systemtest (#22152) refactor(simdv2): allow non-comet server components (#22351) build(deps): Bump rtCamp/action-slack-notify from 2.3.1 to 2.3.2 (#22352) fix(server/v2): respect context cancellation in start command (#22346) chore(simdv2): allow overriding the --home flag (#22348) feat(server/v2): add SimulateWithState to AppManager (#22335) docs(x/accounts): improve comments (#22339) chore: remove unused local variables (#22340) test: x/accounts systemtests (#22320) chore(client): use command's configured output (#22334) perf(log): use sonic json lib (#22233) build(deps): bump x/tx (#22327) fix(x/accounts/lockup): fix proto path (#22319) ...
The standard library's log/slog package was released after the SDK added its standardized Logger interface. It is reasonable to expect new Go projects to depend on log/slog rather than zerolog or any other third party logging implementations. The existing Logger interface maps cleanly to the log/slog API.
This change adds a new cosmossdk.io/log/slog package that exports a single Logger type, backed by an *slog.Logger. Currently, we expect the caller to provide a fully configured *log/slog.Logger and pass it to cosmossdk.io/log/slog.FromSlog. There is no reason we can't have a New function to produce a wrapped slog logger, but that seems out of scope of the initial implementation.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests