New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add WithFatalHook/CheckedEntry.After to replace OnFatal/CheckedEntry.Should #1088
Changes from 4 commits
5fe5ca9
e6465c2
39c2ed3
68e4105
48f79d3
d247606
7481e89
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -133,9 +133,15 @@ func IncreaseLevel(lvl zapcore.LevelEnabler) Option { | |
} | ||
|
||
// OnFatal sets the action to take on fatal logs. | ||
// Deprecated: Use WithFatalHook instead. | ||
func OnFatal(action zapcore.CheckWriteAction) Option { | ||
return WithFatalHook(action) | ||
} | ||
|
||
// WithFatalHook sets a CheckWriteHook to run on fatal logs. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The documentation should probably call out that users of the logging package expect execution to end at
It's pretty common to see code like: resp, err = [...]
if err != nil {
logger.Fatal(err)
}
data, err := ioutil.ReadAll(resp.Body) This code would panic, but could be other cases where data ends up getting corrupt etc. It's similar to catching panics, but a little riskier since it doesn't even half execution of the specific goroutine that calls Should we make it more difficult to allow execution to continue by disallowing One other option is to always There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's fair. Taking a second look, I may have misunderstood the WriteThenNoop hook to mean that it actually allows no-op-ing the Fatal log. But after testing it out, it seems that WriteThenNoop turns into a "os.Exit" as the logic suggests. |
||
func WithFatalHook(hook zapcore.CheckWriteHook) Option { | ||
return optionFunc(func(log *Logger) { | ||
log.onFatal = action | ||
log.onFatal = hook | ||
}) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,10 +27,9 @@ import ( | |
"sync" | ||
"time" | ||
|
||
"go.uber.org/multierr" | ||
"go.uber.org/zap/internal/bufferpool" | ||
"go.uber.org/zap/internal/exit" | ||
|
||
"go.uber.org/multierr" | ||
) | ||
|
||
var ( | ||
|
@@ -152,6 +151,13 @@ type Entry struct { | |
Stack string | ||
} | ||
|
||
// CheckWriteHook allows to customize the action to take after a Fatal log entry | ||
// is processed. | ||
type CheckWriteHook interface { | ||
// OnWrite gets invoked when an entry is written | ||
OnWrite(*CheckedEntry, []Field) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It may be useful to document that the fields here are not all fields on the logger, but just the fields specific to the message. |
||
} | ||
|
||
// CheckWriteAction indicates what action to take after a log entry is | ||
// processed. Actions are ordered in increasing severity. | ||
type CheckWriteAction uint8 | ||
|
@@ -164,10 +170,28 @@ const ( | |
WriteThenGoexit | ||
// WriteThenPanic causes a panic after Write. | ||
WriteThenPanic | ||
// WriteThenFatal causes a fatal os.Exit after Write. | ||
// WriteThenFatal causes an os.Exit(1) after Write. | ||
WriteThenFatal | ||
) | ||
|
||
// OnWrite implements the OnWrite method to keep CheckWriteAction compatible | ||
// with the new CheckWriteHook interface which deprecates CheckWriteAction. | ||
func (a CheckWriteAction) OnWrite(ce *CheckedEntry, _ []Field) { | ||
switch a { | ||
case WriteThenGoexit: | ||
runtime.Goexit() | ||
case WriteThenPanic: | ||
panic(ce.Message) | ||
case WriteThenFatal: | ||
exit.Exit() | ||
} | ||
} | ||
|
||
var _ CheckWriteHook = WriteThenNoop | ||
var _ CheckWriteHook = WriteThenGoexit | ||
var _ CheckWriteHook = WriteThenPanic | ||
var _ CheckWriteHook = WriteThenFatal | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The check is type-level, not value-level so we can just verify that CheckWriteAction implements the interface. |
||
|
||
// CheckedEntry is an Entry together with a collection of Cores that have | ||
// already agreed to log it. | ||
// | ||
|
@@ -178,15 +202,15 @@ type CheckedEntry struct { | |
Entry | ||
ErrorOutput WriteSyncer | ||
dirty bool // best-effort detection of pool misuse | ||
should CheckWriteAction | ||
after CheckWriteHook | ||
cores []Core | ||
} | ||
|
||
func (ce *CheckedEntry) reset() { | ||
ce.Entry = Entry{} | ||
ce.ErrorOutput = nil | ||
ce.dirty = false | ||
ce.should = WriteThenNoop | ||
ce.after = nil | ||
for i := range ce.cores { | ||
// don't keep references to cores | ||
ce.cores[i] = nil | ||
|
@@ -224,17 +248,11 @@ func (ce *CheckedEntry) Write(fields ...Field) { | |
ce.ErrorOutput.Sync() | ||
} | ||
|
||
should, msg := ce.should, ce.Message | ||
putCheckedEntry(ce) | ||
|
||
switch should { | ||
case WriteThenPanic: | ||
panic(msg) | ||
case WriteThenFatal: | ||
exit.Exit() | ||
case WriteThenGoexit: | ||
runtime.Goexit() | ||
hook := ce.after | ||
if hook != nil { | ||
hook.OnWrite(ce, fields) | ||
} | ||
putCheckedEntry(ce) | ||
} | ||
|
||
// AddCore adds a Core that has agreed to log this CheckedEntry. It's intended to be | ||
|
@@ -252,11 +270,19 @@ func (ce *CheckedEntry) AddCore(ent Entry, core Core) *CheckedEntry { | |
// Should sets this CheckedEntry's CheckWriteAction, which controls whether a | ||
// Core will panic or fatal after writing this log entry. Like AddCore, it's | ||
// safe to call on nil CheckedEntry references. | ||
// Deprecated: Use After(ent Entry, after CheckWriteHook) instead. | ||
func (ce *CheckedEntry) Should(ent Entry, should CheckWriteAction) *CheckedEntry { | ||
return ce.After(ent, should) | ||
} | ||
|
||
// After sets this CheckEntry's CheckWriteHook, which will be called after this | ||
// log entry has been written. It's safe to call this on nil CheckedEntry | ||
// references. | ||
func (ce *CheckedEntry) After(ent Entry, hook CheckWriteHook) *CheckedEntry { | ||
if ce == nil { | ||
ce = getCheckedEntry() | ||
ce.Entry = ent | ||
} | ||
ce.should = should | ||
ce.after = hook | ||
return ce | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goroutine or channel isn't necessary.
The logger.Fatal won't panic because we're overriding it.