Reconsider number of logging levels
nhooyr opened this issue · 9 comments
See @peterbourgon's criticism at https://lobste.rs/s/tofgzk/cdr_slog_minimal_structured_logging#c_beo2sc
I fully agree regarding the logging levels. I do want to minimize the number of logs.
Will require some thought.
@cdr/engineering
Info and Error are pretty much required.
Info being for regular information and Error for when an error occurs. On the monitoring front, if enough errors occur, someone is notified.
I can't really think of a good use case for Debug/Warn since most logs with those levels completely ignored. For Critical, I don't think it's a good practice as it would cause noise in monitoring with constant notifications.
As for Fatal, I'm not sure. Its nice to have the logger synced before os.Exit(1) which is something you'll have to remember to do.
(also need to remove LevelFatal anyway since it's == LevelCritical nvm I like it being separate so its clear the process is exiting)
The truth is that logging systems such as Stackdriver use many levels and consumers of this package will want to target them. I think the set of levels is fine.
As far as Fatal goes, it is extremely useful while bootstrapping a programming. It's also obvious that Fatal would exit the program. Stdlib log provides log.Fatal, too.
As far as Fatal goes, it is extremely useful while bootstrapping a programming. It's also obvious that Fatal would exit the program. Stdlib log provides log.Fatal, too.
I was thinking we stop using main() directly and use a structure like:
func main() {
err := run()
if err != nil {
// fatal with any error from run()
}
}But I don't think that'll be easy to introduce and you still might have errors even initializing the logger which would add duplication in syncing the logger and exiting.
@ammario What about Critical? I think we can remove that at least. @sreya92 is on board too.
As far as Fatal goes, it is extremely useful while bootstrapping a programming. It's also obvious that Fatal would exit the program. Stdlib log provides log.Fatal, too.
Only func main has the right to terminate the program. log.Fatal should never be used outside of func main in production applications.
edit: I guess it wasn't clear: I consider log.Fatal a design error, loggers shouldn't have the ability to terminate the process, new logger packages shouldn't include it.
Only func main has the right to terminate the program. log.Fatal should never be used outside of func main in production applications.
That's pretty much all we use it for anyway. We can document this as a best practice.
@nhooyr Critical seems like a standard level on many logging platforms.
@peterbourgon that rule has nice properties, but I don't think it's a big deal if disregarded during initialization. As @nhooyr pointed out, it's easy to forget syncing the logger before exiting.
Syncing should not be a property of the logger, either — it's a property of the underlying io.Writer.
Syncing should not be a property of the logger, either — it's a property of the underlying io.Writer.
That's only true if you remove the fatal level but it's natural to have them together instead of always remembering to sync yourself on exit.
Discussed with the team and they really like the levels as is. I don't think its a big deal either way and its nice to have separate levels when scanning.
It also lets us style fatals appropriately so that they are immediately noticed when in the console.