-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Convert DFatal => DPanic(Level) #197
Conversation
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.
Some small nits, otherwise lgtm. Please file an issue tracking the remaining weirdness with DPanic
; it's important that we iron all these issues out before 1.0.
@@ -63,6 +63,8 @@ const ( | |||
// ErrorLevel logs are high-priority. If an application is running smoothly, | |||
// it shouldn't generate any error-level logs. | |||
ErrorLevel | |||
// DPanicLevel logs an error message, but panics only in development. |
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.
nit: To me, this would be clearer as "DPanicLevel logs are particularly important errors. In development, the logger panics after writing the message."
// after all sub-loggers have received the message, then the Tee | ||
// terminates the process (using os.Exit or panic() per usual | ||
// semantics). | ||
// Exceptions are made for the DPyanic, Panic, and Fatal methods: the returned |
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.
spelling: "DPanic"
// level; each sub-logger's DFatal method dynamically chooses to either call | ||
// Error or Fatal. | ||
// NOTE: DPanic will currently never panic, since the Tee Logger does not | ||
// accept options (nor even have a development flag). |
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.
Please file an issue for this, so we can make sure to fix this before 1.0.
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.
} | ||
func (ml multiLogger) DPanic(msg string, fields ...Field) { | ||
ml.log(DPanicLevel, msg, fields) | ||
// TODO: Implement development/DPanic? |
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.
As mentioned above, let's track this via issues.
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.
indeed, #202 should get it.
@@ -71,6 +76,9 @@ func (c *counters) Reset(key string) { | |||
// FatalLevel, if it happens is sampled and will call the underlying logger Log | |||
// method, which should NOT panic() or terminate. | |||
// | |||
// NOTE: logging at DPanic level currently IS sampled, but calls to DPanic and | |||
// Check(DPanicLevvel) are NOT sampled. |
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.
spelling: "DPanicLevel"
Fixes #175.
Doing this ahead of coming interface-pocalypse discused with @akshayjshah et al (mentioned in the tabled #194).