Skip to content

Allow custom implementation of toZapLevel function #86

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Allow custom implementation of toZapLevel function #86

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 15 additions & 3 deletions zapr.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,9 @@ type zapLogger struct {
// that explain why a call was invalid (for example,
// non-string key). This is enabled by default.
panicMessages bool

// toZapLevel maps logr numeric level to appropriate zap level.
toZapLevel func(lvl int) zapcore.Level
}

const (
Expand Down Expand Up @@ -184,7 +187,7 @@ func (zl *zapLogger) Init(ri logr.RuntimeInfo) {

// Zap levels are int8 - make sure we stay in bounds. logr itself should
// ensure we never get negative values.
func toZapLevel(lvl int) zapcore.Level {
func defaultToZapLevel(lvl int) zapcore.Level {
if lvl > 127 {
lvl = 127
}
Expand All @@ -193,11 +196,11 @@ func toZapLevel(lvl int) zapcore.Level {
}

func (zl zapLogger) Enabled(lvl int) bool {
return zl.l.Core().Enabled(toZapLevel(lvl))
return zl.l.Core().Enabled(zl.toZapLevel(lvl))
}

func (zl *zapLogger) Info(lvl int, msg string, keysAndVals ...interface{}) {
if checkedEntry := zl.l.Check(toZapLevel(lvl), msg); checkedEntry != nil {
if checkedEntry := zl.l.Check(zl.toZapLevel(lvl), msg); checkedEntry != nil {
checkedEntry.Write(zl.handleFields(lvl, keysAndVals)...)
}
}
Expand Down Expand Up @@ -253,6 +256,7 @@ func NewLoggerWithOptions(l *zap.Logger, opts ...Option) logr.Logger {
}
zl.errorKey = "error"
zl.panicMessages = true
zl.toZapLevel = defaultToZapLevel
for _, option := range opts {
option(zl)
}
Expand Down Expand Up @@ -303,5 +307,13 @@ func DPanicOnBugs(enabled bool) Option {
}
}

// ToZapLevel overrides the default function mapping logr's numeric level
// to corresponding zap's level.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add an example of some real-world usage of this new feature?

Copy link
Author

Choose a reason for hiding this comment

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

I have described the real world example in #84 (comment)

Copy link
Author

Choose a reason for hiding this comment

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

Also see even more detailed example below: #86 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

The key point that needs to be called out is that there will be several different zapr instances with the same zap backend in the same program: one for each package or set of packages with its own conventions regarding the logr level. If this is possible, then this option makes sense.

Where it breaks down is when different packages use the same logr instance (for example, by getting it from the context) and then use inconsistent log levels. That is what I was wondering about in #86 (comment).

Copy link
Author

@nilebox nilebox Oct 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Where it breaks down is when different packages use the same logr instance (for example, by getting it from the context) and then use inconsistent log levels.

Agreed, this can only be fixed by addressing the inconsistency between the original libraries.
Even then, I believe the customisable toZapLevel function is a useful improvement:

  • The default behaviour is preserved as is, no hidden or breaking changes
  • It allows to fix the mapping for klog (I believe the current default implementation is still wrong to consider V(0) the only equivalent of Info and V(1) to be the only equivalent of Debug and V(2) and above not even captured by standard zap level at all), i.e. allows
	klogToZapLevel := func(lvl int) zapcore.Level {
		if lvl >= 4 {
			return zapcore.DebugLevel
		}
		if lvl > 0 {
			return zapcore.InfoLevel
		}
		return zapcore.WarnLevel
	}

to be used globally instead of the default if preferred.

I also think it's better to recommend defining custom toZapLevel (i.e. make zapr work for the standard zap config) over zap.NewAtomicLevelAt(zapcore.Level(-2)) suggested in https://github.com/go-logr/zapr?tab=readme-ov-file#increasing-verbosity (i.e. having to change the zap config itself), but it is up to you.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the customisable toZapLevel function is a useful improvement

Yes, let's merge this. We just need to be sure that developers understand when and how to use it.

I also think it's better to recommend defining custom toZapLevel (i.e. make zapr work for the standard zap config) over zap.NewAtomicLevelAt(zapcore.Level(-2)) suggested in https://github.com/go-logr/zapr?

That depends a bit on how the zap backend gets configured. In component-base/logs we map the program's -v parameter to the zapcore.Level and the current mapping works as intended.

Can you update the documentation as part of your PR?

func ToZapLevel(toZapLevel func(lvl int) zapcore.Level) Option {
return func(zl *zapLogger) {
zl.toZapLevel = toZapLevel
}
}

var _ logr.LogSink = &zapLogger{}
var _ logr.CallDepthLogSink = &zapLogger{}
113 changes: 113 additions & 0 deletions zapr_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,119 @@ func TestLogNumeric(t *testing.T) {
}
}

// TestCustomToZapLevel tests support for custom toZapLevel implementation.
func TestCustomToZapLevel(t *testing.T) {
type testCase struct {
name string
level zapcore.Level
toZapLevel func(lvl int) zapcore.Level
expectedLogs []string
}
customToZapLevel := func(lvl int) zapcore.Level {
if lvl > 4 {
return zapcore.DebugLevel
}
if lvl > 0 {
return zapcore.InfoLevel
}
return zapcore.WarnLevel
}

var testCases = []testCase{
{
name: "default, log everything",
level: zapcore.Level(-100), // Log everything
toZapLevel: nil, // default
expectedLogs: []string{
`{"level":"info","msg":"test 0","v":0}`,
`{"level":"debug","msg":"test 1","v":1}`,
// No matching zap level for verbosity 2 and above
`{"level":"Level(-2)","msg":"test 2","v":2}`,
`{"level":"Level(-3)","msg":"test 3","v":3}`,
`{"level":"Level(-4)","msg":"test 4","v":4}`,
`{"level":"Level(-5)","msg":"test 5","v":5}`,
`{"level":"Level(-6)","msg":"test 6","v":6}`,
`{"level":"Level(-7)","msg":"test 7","v":7}`,
},
},
{
name: "default, log debug",
level: zapcore.DebugLevel,
toZapLevel: nil, // default
expectedLogs: []string{
`{"level":"info","msg":"test 0","v":0}`,
`{"level":"debug","msg":"test 1","v":1}`,
// Logs at verbosity 2 and above are dropped
Copy link
Author

@nilebox nilebox Oct 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This test case illustrates the main problem I'm running into, which a custom toZapLevel solves in test cases below.

},
},
{
name: "custom, log everything",
level: zapcore.Level(-100), // Log everything
toZapLevel: customToZapLevel,
expectedLogs: []string{
`{"level":"warn","msg":"test 0","v":0}`,
`{"level":"info","msg":"test 1","v":1}`,
`{"level":"info","msg":"test 2","v":2}`,
`{"level":"info","msg":"test 3","v":3}`,
`{"level":"info","msg":"test 4","v":4}`,
`{"level":"debug","msg":"test 5","v":5}`,
`{"level":"debug","msg":"test 6","v":6}`,
`{"level":"debug","msg":"test 7","v":7}`,
},
},
{
name: "custom, log info",
level: zapcore.InfoLevel,
toZapLevel: customToZapLevel,
expectedLogs: []string{
`{"level":"warn","msg":"test 0","v":0}`,
`{"level":"info","msg":"test 1","v":1}`,
`{"level":"info","msg":"test 2","v":2}`,
`{"level":"info","msg":"test 3","v":3}`,
`{"level":"info","msg":"test 4","v":4}`,
// Logs at verbosity 5 and above are mapped to Debug and therefore dropped
},
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
var buffer bytes.Buffer
writer := bufio.NewWriter(&buffer)
var sampleInfoLogger logr.Logger
encoder := zapcore.NewJSONEncoder(zapcore.EncoderConfig{
MessageKey: "msg",
LevelKey: "level",
EncodeLevel: zapcore.LowercaseLevelEncoder,
})
core := zapcore.NewCore(encoder, zapcore.AddSync(writer), tc.level)
zl := zap.New(core)
opts := []zapr.Option{zapr.LogInfoLevel("v")}
if tc.toZapLevel != nil {
opts = append(opts, zapr.ToZapLevel(tc.toZapLevel))
}
sampleInfoLogger = zapr.NewLoggerWithOptions(zl, opts...)
for i := 0; i < 8; i++ {
sampleInfoLogger.V(i).Info(fmt.Sprintf("test %d", i))
}
if err := writer.Flush(); err != nil {
t.Fatalf("unexpected error from Flush: %v", err)
}
logOutput := buffer.String()
var lines []string
sc := bufio.NewScanner(strings.NewReader(logOutput))
for sc.Scan() {
lines = append(lines, sc.Text())
}
require.Equal(t, len(tc.expectedLogs), len(lines))
for i, expected := range tc.expectedLogs {
actual := lines[i]
require.JSONEq(t, expected, actual)
}
})
}
}

// TestError tests Logger.Error.
func TestError(t *testing.T) {
for _, logError := range []string{"err", "error"} {
Expand Down
Loading