Add zap logger support #93
Conversation
1 similar comment
1 similar comment
| @@ -203,15 +204,15 @@ func TestTracerOptions(t *testing.T) { | |||
| openTracer, closer := NewTracer("DOOP", // respect the classics, man! | |||
| NewConstSampler(true), | |||
| NewNullReporter(), | |||
| TracerOptions.Logger(StdLogger), | |||
| TracerOptions.Logger(log.StdLogger), | |||
There was a problem hiding this comment.
can we have a test that shows backwards compatibility by passing jaeger.Logger as log.Logger?
|
Other than Yuri's comment it LGTM |
1 similar comment
There was a problem hiding this comment.
@black-adder Left some comments here - I think that this will be faster and easier to use if you change a few small things.
|
|
||
| // Infof logs a message at info priority | ||
| func (l *Logger) Infof(msg string, args ...interface{}) { | ||
| l.logger.Info(fmt.Sprintf(msg, args)) |
There was a problem hiding this comment.
Please first check if Info is enabled, since we don't always want to incur the overhead of string formatting:
if l.logger.Facility().Enabled(zap.InfoLevel) {
l.logger.Info(fmt.Sprintf(msg, args))
}In general, if you're going to do formatted logging, you're better off wrapping *zap.SugaredLogger, which already has an efficient version of this API. Accepting that type also makes it clear to users that you're planning to do printf-style logging. Every *zap.Logger can easily and cheaply be converted to a *zap.SugaredLogger.
| } | ||
|
|
||
| // NewLogger creates a new Logger. | ||
| func NewLogger(logger zap.Logger) *Logger { |
There was a problem hiding this comment.
The exported function should take a *zap.Logger, since that's what all of zap's public APIs return.
| ) | ||
|
|
||
| func TestLogger(t *testing.T) { | ||
| logger := NewLogger(*zap.New(zapcore.NewNopCore())) |
There was a problem hiding this comment.
(Or, after uber-go/zap#326 lands, NewNop.)
This is a breaking change, we should maybe consider keeping the original log as is and add the zap-log directory, or rererelease 2.0.0 again