Add logrus adapter for Logger interface (#752)#759
Conversation
log/logrus_logger.go
Outdated
| // NewLogrusLogger returns a logger that logs the keyvals to Writer | ||
| // with a time stamp at the logrus.InfoLevel | ||
| func NewLogrusLogger(w io.Writer) Logger { | ||
| newLogger := logrus.New() |
There was a problem hiding this comment.
I believe the main use case for this package is to adapt a *logrus.Logger used as the main logger in an application so it can be passed to code that expects a Go kit log.Logger. For that use case this constructor would be more flexible if it were declared as NewLogrusLogger(logger *logrus.Logger) log.Logger.
This form allows the main application to configure logrus however it wants and then simply adapt it to the Go kit log.Logger interface.
log/logrus_logger.go
Outdated
| "fmt" | ||
| "io" | ||
|
|
||
| "github.com/sirupsen/logrus" |
There was a problem hiding this comment.
We don't want the base Go kit log package to have a dependency on logrus. The logrus adapter should be in its own package in a subdirectory. I suggest it go in github.com/go-kit/kit/log/logrus.
log/logrus/logrus_logger.go
Outdated
| @@ -0,0 +1,31 @@ | |||
| package log | |||
There was a problem hiding this comment.
Now that the code is moved into the log/logrus package, the package name here should also be logrus.
There was a problem hiding this comment.
Please add at least a sentence of package documentation.
There was a problem hiding this comment.
Can you please clarify on what you mean by package documentation? I looked at other packages like endpoint.go and metrics.go and didn't see anything other that function and type docs.
ChrisHines
left a comment
ChrisHines
left a comment
There was a problem hiding this comment.
Sorry for the delayed response and the way I split comments between stand alone comments and review comments. I was trying the new VS Code Github PR extension and didn't realize right away that it didn't support grouping comments into a single review (or I don't know how to use it right yet).
log/logrus/logrus_logger.go
Outdated
| @@ -0,0 +1,31 @@ | |||
| package log | |||
There was a problem hiding this comment.
Please add at least a sentence of package documentation.
ChrisHines
left a comment
ChrisHines
left a comment
There was a problem hiding this comment.
The code looks good now. Just a couple fixes in the docs and this should be ready to merge.
ChrisHines
left a comment
ChrisHines
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
3 participants
No description provided.