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.

Sign up for GitHub

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
+145 −0
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f7caf7b
feat(log): add slog-backed Logger type
mark-rushakoff Oct 23, 2024
cf20016
chore: update changelog
mark-rushakoff Oct 23, 2024
a078c39
test: use standard library testing package
mark-rushakoff Oct 23, 2024
c3b59df
Merge remote-tracking branch 'origin/main' into mr/log-slog
mark-rushakoff Oct 24, 2024
8f1f86e
chore: rename FromSlog to NewCustomLogger
mark-rushakoff Oct 24, 2024
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
View file
Open in desktop
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
View file
Open in desktop
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`.
mark-rushakoff marked this conversation as resolved.
Show resolved Hide resolved
4 changes: 4 additions & 0 deletions log/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,20 @@ require (
github.com/bytedance/sonic v1.12.3
github.com/pkg/errors v0.9.1
github.com/rs/zerolog v1.33.0
github.com/stretchr/testify v1.8.1
mark-rushakoff marked this conversation as resolved.
Show resolved Hide resolved
)

require (
github.com/bytedance/sonic/loader v0.2.0 // indirect
github.com/cloudwego/base64x v0.1.4 // indirect
github.com/cloudwego/iasm v0.2.0 // indirect
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/klauspost/cpuid/v2 v2.0.9 // indirect
github.com/mattn/go-colorable v0.1.13 // indirect
github.com/mattn/go-isatty v0.0.20 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/twitchyliquid64/golang-asm v0.15.1 // indirect
golang.org/x/arch v0.0.0-20210923205945-b76863e36670 // indirect
golang.org/x/sys v0.22.0 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
)
1 change: 1 addition & 0 deletions log/go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.12.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.22.0 h1:RI27ohtqKCnwULzJLqkv897zojh5/DwS/ENaMzUOaWI=
golang.org/x/sys v0.22.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
Expand Down
47 changes: 47 additions & 0 deletions log/slog/logger.go
View file
Open in desktop
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.
Show resolved Hide resolved
func FromSlog(log *slog.Logger) Logger {
return Logger{log: log}
}
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
// 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}
}

Copy link
Member

@julienrbrt julienrbrt Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

+1 cc @mark-rushakoff

Copy link
Contributor

Choose a reason for hiding this comment

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

@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
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
}
62 changes: 62 additions & 0 deletions log/slog/logger_test.go
View file
Open in desktop
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.
Show resolved Hide 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)
}
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

Enhance test robustness with additional test cases.

While the current tests cover basic logging functionality, consider the following improvements:

  1. Convert to table-driven tests to reduce code duplication
  2. Add error cases (e.g., invalid log levels)
  3. Test WithLevel functionality
  4. 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)
        })
    }
}

julienrbrt marked this conversation as resolved.
Show resolved Hide resolved
Loading