Should be able to call Fatal without exiting

[nathanperkins]

Describe the bug

When using logger.Fatal, it always exits with code 1. We want to be able to call Fatal and exit with different codes that indicate the underlying reason for the failure. We want FATAL to represent logs which caused the component to exit, but we want the exit to happen on our own terms, with defers and the correct exit code.

I tried using logger.WithOptions(zap.WithFatalHook(zapcore.WriteThenNoop)), but this option is not supported and just exits with code 1, without any indication that the option was ignored.

With the supported FatalHook options, it is difficult to accomplish the following:

1. Write log at FATAL severity.
2. Run defers.
3. Exit with a specific code.

Suggestions

First, if the WriteThenNoop option is not supported by Fatal, it could be documented more clearly and it should give a warning or error. This caused a bug in our application since we were calling os.Exit(run()) in main, expecting it to exit with a returned exit code and it was always exiting with code 1 instead.

Second, the logger should have flexibility in the options to call whatever severity we want without imposing additional behaviors. We should be able to continue processing. We don't want a logger to influence the control flow; we just want it to log.

Another option (and maybe the best one) would be to provide another method like FatalNoop and PanicNoop so that code can explicitly write those logs without modifying the control flow.

Ultimately, that is what we are trying to do in our code. We wrote our own logger wrapper which provides a FatalNoExit method which wasn't working properly because it's using zapcore.WriteThenNoop

To Reproduce

Here is a reproduction and demonstration of a use case that is relatively difficult to achieve currently:

package main

import (
	"cmp"
	"errors"
	"os"

	"go.uber.org/zap"
	"go.uber.org/zap/zapcore"
)

const (
	setup1Code  = 4
	setup2Code  = 5
	cleanupCode = 6

)


func setup1() error {
	// return errors.New("setup1 error")
	return nil
}

func setup2() error {
	// return errors.New("setup2 error")
	return nil
}

func cleanup() error {
	return errors.New("cleanup error")
	// return nil
}

func main() {
	os.Exit(run())
}

func run() (code int) {
	logger, err := zap.NewProductionConfig().Build()
	if err != nil {
		panic(err)
	}
	loggerNoExit := logger.WithOptions(zap.WithFatalHook(zapcore.WriteThenNoop))

	if err := setup1(); err != nil {
		loggerNoExit.Fatal("Setup1 failed", zap.Error(err))
		return setup1Code
	}

	defer func() {
		if err := cleanup(); err != nil {
			loggerNoExit.Fatal("Cleanup failed", zap.Error(err))
			code = cmp.Or(code, cleanupCode)
		}
	}()

	if err := setup2(); err != nil {
		loggerNoExit.Fatal("Setup2 failed", zap.Error(err))
		return setup2Code
	}

	return 0
}

Note it is possible here to move the Fatal log into main and write a custom FatalHook that exits with a specific code, but once you have more fatal logs each with different messages and different codes, it becomes more complicated. You could return the code, message and zap fields, but then you lose any zap fields which were added to the logger along the way.

Expected behavior

Program should exit with the code specified by the returned exit code.

[nathanperkins]

The reasoning for this behavior is documented here:

func terminalHookOverride(defaultHook, override zapcore.CheckWriteHook) zapcore.CheckWriteHook {
	// A nil or WriteThenNoop hook will lead to continued execution after
	// a Panic or Fatal log entry, which is unexpected. For example,
	//
	//   f, err := os.Open(..)
	//   if err != nil {
	//     log.Fatal("cannot open", zap.Error(err))
	//   }
	//   fmt.Println(f.Name())
	//
	// The f.Name() will panic if we continue execution after the log.Fatal.
	if override == nil || override == zapcore.WriteThenNoop {
		return defaultHook
	}
	return override
}

I understand the logic here, that if we were to add this override to the panic or fatal hook and pass that logger to other go dependencies, their code would not be expecting logger.Fatal to continue.

But then the terminal hook override should print a warning that this option is unsupported rather than silently overriding it.

Given that there is some good justification not to modify the behavior of logger.Fatal, I added an option to my suggestions where we add a logger.FatalNoop method which is explicit in not modifying the control flow.

I added this to our logger wrapper and it achieves the behavior we want:

type noopAction struct{}

func (a noopAction) OnWrite(ce *zapcore.CheckedEntry, _ []zapcore.Field) {}

// FatalNoop calls logger.Fatal without the normal os.Exit(1) behavior.
func (logger *Logger) FatalNoop(msg string, fields ...zap.Field) {
	zLogger := logger.WithOptions(zap.WithFatalHook(noopAction{}))
	zLogger.Fatal(msg, fields...)
}