feat(log): add slog-backed Logger type #22347
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,5 @@ | ||
| # Log | ||
|
|
||
| The `cosmossdk.io/log` provides a zerolog logging implementation for the Cosmos SDK and Cosmos SDK modules. | ||
|
|
||
| To use a logger backed by an instance of the standard library's `log/slog` package, use `cosmossdk.io/log/slog`. | ||
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
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
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
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,47 @@ | ||||||||||||||||||||||||
| // Package slog contains a Logger type that satisfies [cosmossdk.io/log.Logger], | ||||||||||||||||||||||||
| // backed by a standard library [*log/slog.Logger]. | ||||||||||||||||||||||||
| package slog | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||
| "log/slog" | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| "cosmossdk.io/log" | ||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| var _ log.Logger = Logger{} | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Logger satisfies [log.Logger] with logging backed by | ||||||||||||||||||||||||
| // an instance of [*slog.Logger]. | ||||||||||||||||||||||||
| type Logger struct { | ||||||||||||||||||||||||
| log *slog.Logger | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // FromSlog returns a Logger backed by an existing slog.Logger instance. | ||||||||||||||||||||||||
|
mark-rushakoff marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||
| func FromSlog(log *slog.Logger) Logger { | ||||||||||||||||||||||||
| return Logger{log: log} | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
Contributor
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
Suggested change
Contributor
There was a problem hiding this comment. @julienrbrt, glad you agree! (_/) |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| 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...) | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
Comment on lines
+27
to
+41
Contributor
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...)
} |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| func (l Logger) With(keyVals ...any) log.Logger { | ||||||||||||||||||||||||
| return Logger{log: l.log.With(keyVals...)} | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
Comment on lines
+43
to
+45
Contributor
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...)}
}
|
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Impl returns l's underlying [*slog.Logger]. | ||||||||||||||||||||||||
| func (l Logger) Impl() any { | ||||||||||||||||||||||||
| return l.log | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| package slog_test | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "encoding/json" | ||
| stdslog "log/slog" | ||
| "testing" | ||
|
|
||
| "cosmossdk.io/log/slog" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestSlog(t *testing.T) { | ||
| var buf bytes.Buffer | ||
| h := stdslog.NewJSONHandler(&buf, &stdslog.HandlerOptions{ | ||
| Level: stdslog.LevelDebug, | ||
| }) | ||
| logger := slog.FromSlog(stdslog.New(h)) | ||
|
|
||
| type logLine struct { | ||
| Level string `json:"level"` | ||
| Msg string `json:"msg"` | ||
| Num int `json:"num"` | ||
| } | ||
|
julienrbrt marked this conversation as resolved.
|
||
|
|
||
| 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) | ||
| } | ||
|
Comment on lines
+26
to
+92
Contributor
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:
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)
})
}
}
julienrbrt marked this conversation as resolved.
Comment on lines
+25
to
+92
Contributor
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:
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)
} |
||
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.
Uh oh!
There was an error while loading. Please reload this page.