Support Panic-level logging that doesn't panic

[lhecker]

Jump down to the last paragraph for the actual issue.

I'm currently facing the issue that I got a panic-recovery middleware in my server and thus would like errors catched by it to be logged at the Panic level. I can't do that right now though since log.Panic() also panics in turn which I don't want of course.

This problem could be solved on my end by copying the most essential parts of Logger.check() like this:

ce := log.Core().Check(zapcore.Entry{
    LoggerName: log.Name(),
    Time:       time.Now(),
    Level:      zapcore.PanicLevel,
    Message:    "panic recover",
}, nil)

if ce != nil {
    ce.Write(zap.Stack("stack"), zap.Error(err))
}

Unfortunately Logger does not have a Name() method which would make it easily possible to set the LoggerName above. Would it be possible to add such a method as well as for the other private Logger fields (like errorOutput) before v1.0.0 is released?

[akshayjshah]

Sure, I'm open to this. We can even do this after 1.0, since loggers are concrete types - adding methods isn't a breaking change.

If you open a PR, I'm happy to review it.

[akshayjshah]

This is currently possible, though ugly:

if ce := logger.Check(zap.PanicLevel, "message"); ce != nil {
  logger.Core().Write(ce.Entry, []zapcore.Field{...})
}

If this is truly common, we can revisit expanding the logger type's API surface.

[bufdev]

I don't think most logging libraries would log at the "panic" level without panicking - this gets into a discussion I had with @prashantv where the claim was that log.Fatal is "known" to call os.Exit(...). I rather there be more levels of granularity than have unexpected behavior.

[lhecker]

@peter-edge Well my original intention in this issue wasn't to have a logger.Panic() which does not panic. I wanted something to be able to generate {"level":"panic", ...} log lines at will.
It can basically be solved with @akshayjshah's code snippet above, but currently it's impossible to replicate this piece of code outside of zap.

[bufdev]

My concern with adding something to Zap (more than is there right now) to do this is that its not what will be expected/is inconsistent with other logging libraries - Panic and Fatal mean specific things in golang, and we should probably keep it like that.

[ansel1]

2cents: agree with @peter-edge on the established convention. also agree generally with this blog post: https://dave.cheney.net/2015/11/05/lets-talk-about-logging

I came around this opinion the long way, being used to lots of log levels in other languages. but after panicing, os.exiting, and trying to pick exactly the right log level from 15 choices at each log site, we've settled on the philosophy that your logging library has no business panicing or exiting your program (you're welcome to do that on the next line of code, if appropriate), and that we really only need debug (with granular named loggers for controlling output), info, and error.

[bufdev]

I agree with this sentiment, for zap it's been decided to keep them in there, which does make some sense: #207

[akshayjshah]

Yep - we've had this discussion many times, in many different venues. #207 captures my feelings about it.

[lhecker]

So what you guys are saying is that you left the possibility in for users of zap to write panic-level log lines at will, but if e.g. a stdlib function panics you may only log to up to the error-level? Or: A panic caused by zap is panic-level but a panic caused by the stdlib is error-level? Every time I write strings.Repeat("", -1) it's an error-, but every a invariant of my zap-infused code is violated and it logger.Panic("")s it's a panic-log-line?
Am I the only person seeing the discrepancy here? This is a plain flaw in zap's Logger API of not being able to convert foreign panics to zap-panics.

P.S.: That you panic in zapcore with a plain string instead of a discrete error type is another flaw btw. Makes it impossible to recover from and detect panics caused by logger.Panic() and thus prevents you from printing non-zap-panics at panic-level without possibly printing the same message twice.

[akshayjshah]

@lhecker - I see the asymmetry you're pointing out, but I just don't see it as a significant issue. This is, from what I see, the overwhelming consensus among logging libraries that provide levels. It's also the API that the authors of this library prefer.

That said, we wrote zap knowing that our preferences won't please everyone - that's why there are dozens of logging libraries, each with slightly different APIs. The zapcore package and the Core type are relatively unopinionated, and I encourage you to build the API you'd prefer around it. Zap's own Logger is just a single, relatively small file - it's basically middleware around a Core. Use the Core, get all the performance benefits of zap, and get exactly the API you'd prefer.