Make the zero value useful

[wojas]

While updating go-tlsconfig to logr v1, I encountered an issue with how zero values of Logger are handled. These currently cause a panic when a method is called on them.

In go-tlsconfig a logr.Logger is an option that could be passed like this (pre-v1 code):

type Options struct {
        ...
	Logr     logr.Logger
}

func New(ctx context.Context, opt Options) (*Foo, error) {
        if log == nil {
		log = SomeDefaultLogger()
	}
        ...
}

The caller can omit the Logr option if it is not interested in logging or has no preference for a logger, depending on the component.

Since v1 made the Logger a concrete struct instead of an interface, it is no longer possible to set or compare it to nil. If the caller omits the Logr, any log call will cause a panic.

The component also has no clean way to check if it received a zero value. It is possible to check if log.GetSink() == nil, but this is a bit messy and easy to forget.

Proposed solution

These problems can be avoided by making the zero value of a Logger useful, which is considered a good practise for Go values anyway. This makes the Logger safer and more convenient to use. Calling methods on a zero value would be equivalent to calling them on a Discard logger.

Additionally, a new IsZero() function would allow testing for the zero value so that a default logger can be assigned, if desired. This is similar to Time.IsZero() in the standard library.

The Options struct in the example above would remain the same and the New function would become:

func New(ctx context.Context, opt Options) (*Foo, error) {
        if log.IsZero() {
		log = SomeDefaultLogger()
	}
        ...
}

If the component is not interested in logging when a zero Logger is provided, it can still safely use that zero value and no longer needs to explicitly assign a Discard logger.

The Discard logger would remain as is and not be converted to a zero value. This allows a caller to explicitly disable logging without it would being overridden with some component-specific default logger.

I posted a PR with this change (#153).

Alternatives

Changing the option to *logr.Logger is also undesirable, because this would require:

• the caller to explicitly take the address of a logger like &log to pass it as an option.
• the component to dereference the pointer like *log to use and pass it as a value.

[pohly]

We discussed that in the past and the conclusion then was to establish the convention that a non-pointer Logger always must be usable (non-null). Code that wants to indicate that a logger is optional should use a pointer, but the need for that should be rare. In functional options, the option internally can use a field with a pointer, but then expose only Logger in the API.

[wojas]

In general it is a good practise to make non-pointer zero values usable and in this case it can be done with little effort. It would not matter much for some internal component, but this is something that is widely used by different projects.

While a pointer for optional values is occasionally used, it makes values less convenient to work with. In most cases where a zero value (e.g. an empty string) is sufficient to distinguish it from explicitly set values, the zero value is used instead. For example, in a config struct that is read from YAML, I would only use a pointer if I really need to distinguish between the zero value and an unset value.

The advantages for this specific case would be:

• Recommended Go practise
• No risk of unintentional panics due to uninitialized values
• A logger option can be easily added to components without resorting to pointers or breaking existing callers
• No pointer dereferencing in the code just to work around the current limitation

Downsides:

• Needs to be taken into account when adding new methods to the Logger
• Potentially a tiny performance impact (but I am not sure this is even measurable)

[wojas]

Performance comparison to make it more concrete:

name                        time/op
LogDiscard-20               3.95ns ± 2%
LogDiscard_no_nil_check-20  3.64ns ± 1%

So the overhead of this extra check is 0.3 nanosecond, which is basically one CPU clock cycle.

Benchmark code:

func (l Logger) benchNoNilCheckEnabled() bool {
	return l.sink.Enabled(l.level) // nil check omitted for benchmark
}

func (l Logger) benchNoNilCheckInfo(msg string, keysAndValues ...interface{}) {
	if l.benchNoNilCheckEnabled() {
		if withHelper, ok := l.sink.(CallStackHelperLogSink); ok {
			withHelper.GetCallStackHelper()()
		}
		l.sink.Info(l.level, msg, keysAndValues...)
	}
}

func BenchmarkLogDiscard(b *testing.B) {
	var l Logger = Discard()
	for i := 0; i < b.N; i++ {
		l.Info("foo")
	}
}

func BenchmarkLogDiscard_no_nil_check(b *testing.B) {
	var l Logger = Discard()
	for i := 0; i < b.N; i++ {
		l.benchNoNilCheckInfo("foo")
	}
}