Add zapgrpc package

[bufdev]

I'm not sure if we want this, so feel free to close this otherwise, but I was adding it in yarpc/yarpc-go#863 and thought it might make more sense here.

https://github.com/grpc/grpc-go has a global internal logger it uses, and https://github.com/grpc/grpc-go/tree/master/grpclog defines what logger is used. If a logger is not set, the default golang logger is used, which could mean lost logs for applications that use zap and grpc. This package defines a simple wrapper for a *zap.Logger that is compatible with grpclog.Logger. To use it, one would do:

grpclog.SetLogger(zapgrpc.NewLogger(zap.L())

[akshayjshah]

Generally love this idea. I'm out for a week and won't be able to look until I'm back; try pinging @prashantv or we can pick this up on Wednesday.

In the meantime, no harm in integrating it into YARPC as an internal package.

[bufdev]

I updated this to have a WithDebug function, and to use a struct instead of an interface. @prashantv if you want to review that's great, but no rush on this.

[akshayjshah]

Love the idea of bundling this package with zap, since it doesn't introduce any new dependencies.

Needs tests, but the general approach lgtm.

[akshayjshah]

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) }).

[bufdev]

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

[bufdev]

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.

[akshayjshah]

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.

[bufdev]

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.

[akshayjshah]

Fair enough.

[akshayjshah]

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

[bufdev]

Updated to take a *Logger instead

[bufdev]

I've added tests that give 100% coverage on zapgrpc

[flisky]

Sorry for a late chiming in, but Iet me introduce another way.

grpclog.Logger interface says:
Logger mimics golang's standard Logger as an interface.

So you can do this -

stdLogger := zap.NewStdLog(logger)
grpclog.SetLogger(stdLogger)

[akshayjshah]

@flisky you can definitely do that, but it sacrifices quite a bit of performance in some common situations. Since NewStdLog needs to return a *log.Logger, we have to implement io.Writer - and Write is only called after all string formatting is done. When info-level logging is disabled, this incurs a bunch of string-formatting expense only to drop the final output.

By wrapping the SugaredLogger and directly implementing the gRPC interface, we can avoid all that overhead.

[flisky]

Thank you for your elaboration, and I see the point.

I'm just not sure whether it should be maintained under zap.

• The grpclog is overhauling. How would we keep sync with it?
• How about other projects if we maintain zapgrpc? Are we encourage the community to contribute more bindings into zap?

Just my 2 cents.