Skip to content
Merged
Changes from 3 commits
Commits
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
105 changes: 105 additions & 0 deletions zapgrpc/zapgrpc.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
// Copyright (c) 2016 Uber Technologies, Inc.
//
// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to deal
// in the Software without restriction, including without limitation the rights
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
// copies of the Software, and to permit persons to whom the Software is
// furnished to do so, subject to the following conditions:
//
// The above copyright notice and this permission notice shall be included in
// all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.

// Package zapgrpc provides a logger that is compatible with grpclog.
package zapgrpc // import "go.uber.org/zap/zapgrpc"

import "go.uber.org/zap"

// LoggerOption is an option for a new Logger.
type LoggerOption func(*loggerOptions)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See comment on Logger - if possible, it'd be nice to avoid having exported types reference unexported types.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated to take a *Logger instead


// WithDebug says to print the debug level instead of the default info level.
func WithDebug() LoggerOption {
return func(loggerOptions *loggerOptions) {
loggerOptions.printFunc = (*zap.SugaredLogger).Debug
loggerOptions.printfFunc = (*zap.SugaredLogger).Debugf
}
}

// NewLogger returns a new Logger.
func NewLogger(l *zap.Logger, options ...LoggerOption) *Logger {
loggerOptions := newLoggerOptions(options...)
return &Logger{
l.Sugar(),
(*zap.SugaredLogger).Fatal,
(*zap.SugaredLogger).Fatalf,
loggerOptions.printFunc,
loggerOptions.printfFunc,
}
}

// Logger is a logger that is compatible with the grpclog Logger interface.
type Logger struct {
sugaredLogger *zap.SugaredLogger
fatalFunc func(*zap.SugaredLogger, ...interface{})
fatalfFunc func(*zap.SugaredLogger, string, ...interface{})
printFunc func(*zap.SugaredLogger, ...interface{})
printfFunc func(*zap.SugaredLogger, string, ...interface{})
}
Copy link
Copy Markdown
Contributor

@akshayjshah akshayjshah Apr 12, 2017

Choose a reason for hiding this comment

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

What's the benefit of keeping these four function pointers instead of

type Logger struct {
  sugar *zap.SugaredLogger
  development bool
}

and checking development in Print and friends? I don't think that the performance impact of a single boolean check will even be measurable relative to the cost of string formatting.

This also lets us get rid of the the loggerOptions type in favor of type Option func(*Logger) (or even type Option interface { apply(*Logger) }).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Talked a little offline, I still prefer this, but I can change to something else

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I could also do debug bool for now, if that's better for you - it accomplishes the same thing. My biggest issue would be doing WithDevelopment - I want this to be more specific.

Copy link
Copy Markdown
Contributor

@akshayjshah akshayjshah Apr 12, 2017

Choose a reason for hiding this comment

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

Mind explaining what's attractive about this approach? I'm not necessarily opposed, I'm just not seeing the upside.

There's no reason I can see that changing the internal structure of Logger requires any changes to the public API - WithDevelopment can still do exactly what it does now.

Copy link
Copy Markdown
Contributor Author

@bufdev bufdev Apr 13, 2017

Choose a reason for hiding this comment

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

Maybe there's a misunderstanding - note this is controlling what happens when you call a grpclog function such as grpclog.Printf, not controlling the actual *zap.Logger given to zapgrpc.SetLogger, so for example:

grpclog.Print("hello") // if WithDebug was not set, this will call zap.Info
grpclog.Print("hello") // if WithDebug was set, this will call zap.Debug

By setting WithDebug, if the level of the *zap.Logger is not debug, this will suppress grpc logging, which might be what someone wants.

Doing this by setting printFunc, printfFunc versus having a debug bool to me is cleaner, and the difference is minor, something around a few lines. I think the function style is nicer, and also note how it lets me test grpclog.Fatal* functions easily - without being able to also set fatalFunc, fatalfFunc, I wouldn't be really able to test grpclog.Fatal* functions because the program would exit. Without adding any special boolean to the actual implementation, I am able to add a withWarn LoggerOption in the tests that sets the fatal functions to redirect to the warn level, in the same style as the WithDebug option. This alone might be a good tie breaker.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fair enough.


// Fatal implements grpclog.Logger#Fatal.
func (l *Logger) Fatal(args ...interface{}) {
l.fatalFunc(l.sugaredLogger, args...)
}

// Fatalf implements grpclog.Logger#Fatalf.
func (l *Logger) Fatalf(format string, args ...interface{}) {
l.fatalfFunc(l.sugaredLogger, format, args...)
}

// Fatalln implements grpclog.Logger#Fatalln.
func (l *Logger) Fatalln(args ...interface{}) {
l.fatalFunc(l.sugaredLogger, args...)
}

// Print implements grpclog.Logger#Print.
func (l *Logger) Print(args ...interface{}) {
l.printFunc(l.sugaredLogger, args...)
}

// Printf implements grpclog.Logger#Printf.
func (l *Logger) Printf(format string, args ...interface{}) {
l.printfFunc(l.sugaredLogger, format, args...)
}

// Println implements grpclog.Logger#Println.
func (l *Logger) Println(args ...interface{}) {
l.printFunc(l.sugaredLogger, args...)
}

type loggerOptions struct {
printFunc func(*zap.SugaredLogger, ...interface{})
printfFunc func(*zap.SugaredLogger, string, ...interface{})
}

func newLoggerOptions(options ...LoggerOption) *loggerOptions {
loggerOptions := &loggerOptions{}
for _, option := range options {
option(loggerOptions)
}
if loggerOptions.printFunc == nil {
loggerOptions.printFunc = (*zap.SugaredLogger).Info
}
if loggerOptions.printfFunc == nil {
loggerOptions.printfFunc = (*zap.SugaredLogger).Infof
}
return loggerOptions
}