apex / log

Structured logging package for Go.
MIT License
1.36k stars 110 forks source link

ignore nil error on WithError (fixes #67) #68

Closed blockloop closed 4 years ago

tj commented 5 years ago

ahh thought we had an issue discussing this but it was a previous pr #23, TLDR; I think it's a user-error so I don't think we should be defensive personally. I found that behaviour in pkg/errors to be really unclear

blockloop commented 5 years ago

Ah okay. We have some pieces of configuration where things flow through certain areas of code if there is an error or not and often we have the same log message and fields regardless. Something like

n, err := strconv.Atoi(num)
log.WithField("export_level", n).
    WithError(err).
    Info("exporter level configured")

We want to log regardless of the error so it requires a bit more code to accomplish

n, err := strconv.Atoi(num)
ll := log.WithFields("export_level", n)
if err != nil {
    ll = ll.WithError(err)
}
ll.Info("exporter level configured")

In this case we default the value to 0 and log the error if it exists and continue on with a sane default of 0 while still notifying the user if there was some kind of parse error.

tj commented 5 years ago

ahhh I guess that's a little unconventional as well since it's not an Error("xxx"). Might be nice to have some kind of conditional error/info method which handles the nil error, not sure how I feel about it haha

jackmcguire1 commented 4 years ago

+1 for this, we shouldn't be panicking for a 'user error'

tj commented 4 years ago

@jackmcguire1 unfortunately Go doesn't agree :D, otherwise it wouldn't have nil to begin with

jackmcguire1 commented 4 years ago

@tj apologies but I’m sure none of us are expecting a nil panic, much like why this thread was created in the first place, it’s not at all necessary to panic, especially in the scenario if we don’t want to cause unnecessary nesting if statements...

I’m just better off doing

‘’’ log. WithField(“err”, err) ‘’’