ctxzap: add convenience functions

[CAFxX]

In our codebases some very common patterns are emerging.

Either:

func foo(ctx context.Context, ...) (...) {
	logger := ctxzap.Extract(ctx)

	// later on

	if err := bar(...); err != nil {
		logger.Error("bar failed", zap.Error(err))
	}

	// other similar uses of logger follow...
}

or, especially if there is a single use of logger:

func foo(ctx context.Context, ...) (...) {
	// later on

	if err := bar(...); err != nil {
		ctxzap.Extract(ctx).Error("bar failed", zap.Error(err))
	}
}

The latter is not terrible, but it's repetitive and noisy. The former has some non-obvious interactions with other related functions (see below for details). And the existence of both ways to do the same thing leads to overhead (and sometimes bikeshedding) during code reviews. 😁

I would like to suggest, in the same spirit as fmt, math/rand, net, net/http, and other packages in the standard library, to define convenience short-form versions of the most common zap.Logger methods (Debug, Info, Warn, Error), similar to:

// Error is equivalent to calling Error on the zap.Logger in the context.
func Error(ctx context.Context, msg string, fields ...zap.Field) {
	Extract(ctx).Error(msg, fields...)
}

this would help unify the logging idiom (in most cases) to:

func foo(ctx context.Context, ...) (...) {
	// later on

	if err := bar(...); err != nil {
		ctxzap.Error(ctx, "bar failed", zap.Error(err))
	}

	// other similar uses follow...
}

This may superficially look like a small improvement, but it compounds quickly for large/complex services. This is especially true for time spent in style discussions during code review, that in the past have also overlooked interactions between the "single Extract call" style (the first one at the top) and its non-obvious interactions with ctxzap.AddFields, ctxzap.ToContext, and other calls to ctxzap.Extract.

[johanbrandhorst]

Thanks for your issue! I think this sounds reasonable, though it's effectively the same as just replacing all your logger.XXX call sites with ctxzap.Extract(ctx).XXX, right?

[CAFxX]

Correct. The main benefit comes from not having to argue about how this should be done over PRs, as well as indicating to users the golden path that prevents them from shooting their own foot (see the bit about interactions with other functions).

Reducing visual noise is the icing on the cake.

[johanbrandhorst]

I'm not familiar enough with the package so say, but do we risk introducing confusing user behaviour? What happens if there's no logger in the context if you call ctxzap.Error?

[CAFxX]

Extract already takes care of that by returning a nop Logger @johanbrandhorst

go-grpc-middleware/logging/zap/ctxzap/context.go

Lines 36 to 39 in a77ba4d

	l, ok := ctx.Value(ctxMarkerKey).(*ctxLogger)
	if !ok || l == nil {
	return nullLogger
	}

go-grpc-middleware/logging/zap/ctxzap/context.go

Line 20 in a77ba4d

nullLogger = zap.NewNop()

[johanbrandhorst]

That's not terribly comforting but I guess we're not making things worse. Would you like to make this contribution?

[CAFxX]

@johanbrandhorst opened #408