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

Using zapcore.Level less than -1 and sampling causes a panic #713

Closed
shawn-hurley opened this issue May 15, 2019 · 4 comments
Closed

Using zapcore.Level less than -1 and sampling causes a panic #713

shawn-hurley opened this issue May 15, 2019 · 4 comments

Comments

@shawn-hurley
Copy link

When someone sets the zapcore.Level(-2) with sampling set to true, there is a panic.

github.com/presslabs/mysql-operator/vendor/go.uber.org/zap/zapcore.(*counters).get(0xc0001d2000, 0xc000151cfe, 0xbb762a, 0xc, 0xc000151be0)
	/home/x/go/src/github.com/presslabs/mysql-operator/vendor/go.uber.org/zap/zapcore/sampler.go:48 +0x79
github.com/presslabs/mysql-operator/vendor/go.uber.org/zap/zapcore.(*sampler).Check(0xc00018f560, 0xfe, 0xbee5907d06b7cfd4, 0x2a36d55, 0x11b62a0, 0xbb4baf, 0x7, 0xbb762a, 0xc, 0x0, ...)
	/home/x/go/src/github.com/presslabs/mysql-operator/vendor/go.uber.org/zap/zapcore/sampler.go:128 +0x9d
github.com/presslabs/mysql-operator/vendor/go.uber.org/zap.(*Logger).check(0xc0000a91a0, 0x40bffe, 0xbb762a, 0xc, 0xc00010e501)
	/home/x/go/src/github.com/presslabs/mysql-operator/vendor/go.uber.org/zap/logger.go:269 +0x137
github.com/presslabs/mysql-operator/vendor/go.uber.org/zap.(*Logger).Check(0xc0000a91a0, 0xb2a0fe, 0xbb762a, 0xc, 0xfffffffffffffffe)
	/home/x/go/src/github.com/presslabs/mysql-operator/vendor/go.uber.org/zap/logger.go:172 +0x48
github.com/presslabs/mysql-operator/vendor/github.com/go-logr/zapr.(*infoLogger).Info(0xc0000bba40, 0xbb762a, 0xc, 0x0, 0x0, 0x0)
	/home/x/go/src/github.com/presslabs/mysql-operator/vendor/github.com/go-logr/zapr/zapr.go:69 +0x4e

I was wondering if it makes sense to allow for this configuration by disabling sampling for any logs that are below the set debug level.

Original Issue:
go-logr/zapr#2

/cc @DirectXMan12

@prashantv
Copy link
Collaborator

Zap only supports the levels defined here. I'm surprised if it works at all with levels that aren't defined there, but it's not something we support.

Supporting custom log levels (#680) would likely help with your issue but I don't think we'd do it in 1.0.

@shawn-hurley
Copy link
Author

Would you be interested in others implementing it?

@prashantv
Copy link
Collaborator

I don't think we want to add any more levels to zap, I've found that users aren't even sure when to use the current set of levels. I think adding more levels increases complexity, so we'd need to be convinced of the benefit before adding additional levels (or allowing custom levels).

@shawn-hurley
Copy link
Author

As there has been little movement on this issue, I am going to close it. If someone would like to drive it we can either re-open or create a duplicate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants