db: by default don't log to standard output #2928
Conversation
Previously, by default an Options struct without a set Logger would default to logging to stdout using Go's standard library logging. This made unit tests particularly verbose. This is a backwards incompatible change, and to restore previous behavior clients that leave Logger unset should explicitly set it to pebble.DefaultLogger.
|
This change is |
|
Thoughts on this? I don't think it's too burdensome of a change in defaults and makes our lives easier. |
There was a problem hiding this comment.
Don't we want these logs for unit tests? They should only be visible when using -test.v no?
Reviewable status: 0 of 4 files reviewed, all discussions resolved
There was a problem hiding this comment.
Don't we want these logs for unit tests? They should only be visible when using
-test.vno?
Running a single package's tests will show all standard output. But go test ./... will silence it. I think tests can (and ideally should) use testLogger which will log to (*testing.T).Log, so that it's only visible when using -test.v or during failures.
Reviewable status: 0 of 4 files reviewed, all discussions resolved
|
Would it be possible to also expose a NoopLogger / NoopLoggerAndTracer to allow setting those? They are defined in EDIT: Or doh, just merge this :) That also works, then I don't have to define anything. |
Doesn't that mean that if we have a CI failure, we won't see the logs? |
There was a problem hiding this comment.
The Pebble EventListener logs, yes, unless the test uses a testLogger to direct log output to the *testing.T. I don't think the Pebble logs are very useful for debugging unit tests, with the exception being Fatalfs which will still be visible (since they panic). In my experience, the intertwined logs (especially since they recently became more verbose) just make it harder to find the actual test failure.
Reviewable status: 0 of 4 files reviewed, all discussions resolved
|
Sounds good. I'm still confused how there is a difference in behavior depending what packages we |
Previously, by default an Options struct without a set Logger would default to logging to stdout using Go's standard library logging. This made unit tests particularly verbose. This is a backwards incompatible change, and to restore previous behavior clients that leave Logger unset should explicitly set it to pebble.DefaultLogger.