search

rs / zerolog

Zero Allocation JSON Logger
MIT License
10.62k stars 572 forks source link

Fatal() does os.Exit(1) when Nop() log is disabled #473

Open lbergesio opened 2 years ago

lbergesio commented 2 years ago

I think this is an error, It was discovered after upgrading from 1.26.1 to 1.28.0:

Fatal() here https://github.com/rs/zerolog/blob/d894f123bc5c2a887c95e90218b9410563141d67/log.go#L367

calls newEvent https://github.com/rs/zerolog/blob/d894f123bc5c2a887c95e90218b9410563141d67/log.go#L444

with the done paremeter being os.Exit(1) so here even the log is disabled, done is executed which was not like this before:

https://github.com/rs/zerolog/blob/d894f123bc5c2a887c95e90218b9410563141d67/log.go#L448

lbergesio commented 2 years ago

This has been introduced in 1.27.0, is this the new expected behaviour?

rs commented 2 years ago

This is the expected behavior though. Disabling logging should only disable the logging and not change the flow of you code.

lbergesio commented 2 years ago

The problem to me is that the logger I am using is Nop() and it reads: Nop returns a disabled logger for which all operation are no-op. So I would expect that even Fatal() is not op for it, meaning not exiting my program. Does it make sense?

rs commented 2 years ago

If you call Fatal on a logger in your code, you are not supposed to know the logger is disabled, and the code logic is not supposed to pass the Fatal statement.

lbergesio commented 2 years ago

Agree. My point is more in the semantics of "no op" not if in the log is disabled or not. But if according to you a nop logger should be exiting ok, i will just modify my code that was using 1.26.1. this can be closed in such case. Thanks