Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(log): add slog-backed Logger type #22347

Merged
merged 5 commits into from
Oct 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions log/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ Each entry must include the Github issue reference in the following format:
### Improvements

* [#22233](https://github.com/cosmos/cosmos-sdk/pull/22233) Use sonic json library for faster json handling
* [#22347](https://github.com/cosmos/cosmos-sdk/pull/22347) Add cosmossdk.io/log/slog to allow using a standard library log/slog-backed logger.

## [v1.4.1](https://github.com/cosmos/cosmos-sdk/releases/tag/log/v1.4.1) - 2024-08-16

Expand Down
2 changes: 2 additions & 0 deletions log/README.md
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 wrapping an instance of the standard library's `log/slog` package, use `cosmossdk.io/log/slog`.
50 changes: 50 additions & 0 deletions log/slog/logger.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
// 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
}

// NewCustomLogger returns a Logger backed by an existing slog.Logger instance.
// All logging methods are called directly on the *slog.Logger;
// therefore it is the caller's responsibility to configure message filtering,
// level filtering, output format, and so on.
func NewCustomLogger(log *slog.Logger) Logger {
return Logger{log: log}
}

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
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ 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
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.


// Impl returns l's underlying [*slog.Logger].
func (l Logger) Impl() any {
return l.log
}
92 changes: 92 additions & 0 deletions log/slog/logger_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
package slog_test

import (
"bytes"
"encoding/json"
stdslog "log/slog"
"testing"

"cosmossdk.io/log/slog"
)

func TestSlog(t *testing.T) {
var buf bytes.Buffer
h := stdslog.NewJSONHandler(&buf, &stdslog.HandlerOptions{
Level: stdslog.LevelDebug,
})
logger := slog.NewCustomLogger(stdslog.New(h))
Comment on lines +12 to +17
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ 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))
}


type logLine struct {
Level string `json:"level"`
Msg string `json:"msg"`
Num int `json:"num"`
}

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)
}
}
Comment on lines +25 to +92
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add test cases for error conditions and edge cases.

While basic logging functionality is covered, consider adding tests for:

  1. Invalid key-value pairs (odd number of arguments)
  2. Empty or nil messages
  3. Special characters in messages
  4. Timestamp presence and format validation
  5. 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)
}

Loading