Skip to content
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

docs: clarify Error semantic #119

Merged
merged 1 commit into from
Nov 25, 2021
Merged

Conversation

pohly
Copy link
Contributor

@pohly pohly commented Nov 9, 2021

Two aspects were not spelled out explicitly, which sometimes led to
misunderstandings:

  • error messages are always printed
  • the error instance is optional

Fixes: #118

@pohly
Copy link
Contributor Author

pohly commented Nov 9, 2021

cc @wojas

@wojas
Copy link
Contributor

wojas commented Nov 9, 2021

This is not how I have interpreted the interface, as discussed in #118. I would argue that not checking the level in Error() is a bug.

@wojas
Copy link
Contributor

wojas commented Nov 9, 2021

The README and Dave's post do suggest that your interpretation is the correct one:

Info logs are things you want to tell the user which are not errors. Error logs are, well, errors. If your code receives an error from a subordinate function call and is logging that error and not returning it, use error logs.

This does lead to two unfortunate usability issues:

  • There is no convenient way to simply log an error that was handled in an informational (e.g. debug) message, which is a common occurrence. It would have to be logged as a normal value, which is unfortunate due to the prevalence of error values in Go. Implementations may also not attach the extra information here that they would otherwise, but this is fixable.
  • If you want to log an 'error' without an error value, you need to result to passing nil, which is a bit ugly.

@pohly
Copy link
Contributor Author

pohly commented Nov 9, 2021

Implementations may also not attach the extra information here that they would otherwise, but this is fixable.

Do you know what that extra information is in actual implementations?

A stack backtrace might be a candidate, but IMHO that's not really useful because it will identify the location where the error gets logged, not where it occurred.

@wojas
Copy link
Contributor

wojas commented Nov 9, 2021

Do you know what that extra information is in actual implementations?

A stack backtrace might be a candidate, but IMHO that's not really useful because it will identify the location where the error gets logged, not where it occurred.

Since Go 1.13, an error can wrap another error, and this can be unwrapped. I'm not sure if this is actually useful in logging, as the final error message will typically include all the nested information.

@pohly
Copy link
Contributor Author

pohly commented Nov 9, 2021

Wrapped errors as values can and probably will be handled the same way in Error and Info - at least all implementations that I have seen use Error() to retrieve the user-visible string.

@wojas
Copy link
Contributor

wojas commented Nov 10, 2021

It probably would have been nicer if the signatures of Info and Error would have been the same and the interface had an extra WithError(err error) method.

Then you could do:

l.WithError(err).Info("FYI, I just handled an error")
l.WithError(err).Error("This is bad!")

Adding WithError is of course still an option.

Anyway, I think it's good to clarify this in the docs.

logr.go Outdated Show resolved Hide resolved
logr.go Outdated Show resolved Hide resolved
Copy link
Contributor

@thockin thockin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nits on words.

logr.go Outdated
// information (such as stack traces) on calls to Error(). Another difference
// is that error messages always get emitted, regardless of the current
// verbosity. This can also be used when no error instance is available
// by calling Error with nil as err parameter.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd cut words:

"""
...on calls to Error(). Error() messages are always logged, regardless of the current verbosity. If there is no error instance available, passing nil is valid.
"""

logr.go Outdated
@@ -253,11 +257,12 @@ func (l Logger) Info(msg string, keysAndValues ...interface{}) {
// Error logs an error, with the given message and key/value pairs as context.
// It functions similarly to Info, but may have unique behavior, and should be
// preferred for logging errors (see the package documentations for more
// information).
// information). The log message will always be emitted, regardless of verbosity level.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

linewrap? Can't tell for sure but it looks long

logr.go Outdated
//
// The msg argument should be used to add context to any underlying error,
// while the err argument should be used to attach the actual error that
// triggered this log line, if present.
// triggered this log line, if present. The err parameter is optional
// and nil can be passed instead of an error instance.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/can/may/

@thockin thockin self-assigned this Nov 24, 2021
Two aspects were not spelled out explicitly, which sometimes led to
misunderstandings:

- error messages are always printed
- the error instance is optional
@pohly
Copy link
Contributor Author

pohly commented Nov 25, 2021

@thockin Nits addressed and commit updated, please take another look.

@thockin thockin merged commit de1ec28 into go-logr:master Nov 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logger.Error: nil err allowed?
3 participants