log.Fatal() and log.Panic() are not guaranteed to exit

[a-canya]

In my app I initialize the global log level with:

level, err := zerolog.ParseLevel(logLevel)
if err != nil { ... }
zerolog.SetGlobalLevel(level)

and then I use the global logger to do all kinds of stuff. For instance:

log.Fatal().Msg("something happened and app needs to end")

However, if the logLevel was something such as "panic" or "disabled", the log.Fatal() will not end the program. What's even more annoying is that if logLevel is an empty string, then level will be NoLevel and nothing will be logged (and fatals and panics will not cause the program to exit). (playground example)

The workaround is quite obvious and easy if you are aware of this problem. But taking into account that the native log package has equivalent functions which are guaranted to exit, this seems confusing to me.

Also, while coding, if you really want the program to end, you may consider calling sys.Exit() rather than calling log.Fatal().

• It would be less confusing to me if log.Fatal() and log.Panic() would always call sys.Exit() / panic(), even if their logging level is disabled and nothing is logged, so that program flow does not depend on log level. Is the current behaviour a design choice? What do you think about changing this?

• If this is not changed, I suggest at least adding a warning in the docs, at the log.Fatal log.Panic, Logger.Fatal and Logger.Panic functions and methods.

[rs]

You are right, in newEvent, done should be called before returning when enabled is false. Would you send a PR?

[a-canya]

Hi @rs , thanks for the quick reply. I am not very familiar with the codebase but I'm taking a look.

If I understand your proposal and the code correctly, this would have weird consequences:

zerolog.SetGlobalLevel("info")
log.Fatal() // does not exit because Msg() is not called (current behaviour)

zerolog.SetGlobalLevel("disabled")
log.Fatal() // exits because we call done at newEvent()

[rs]

That’s correct

[a-canya]

I've been looking for a way to improve what I pointed out in the previous comment but it would require many changes. Another issue with this solution is that the message is lost in the case of panic.

Anyway, I guess this is a rather unusual corner case and keeping the os.Exit() / panic() call is an improvement.