-
Notifications
You must be signed in to change notification settings - Fork 241
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
Added Logrus logger support #7
Conversation
@@ -44,6 +44,20 @@ var ( | |||
respReadLimit = int64(4096) | |||
) | |||
|
|||
type GoLogger interface { |
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.
does it really need all these methods ? I would assume retryablehttp
as a lib should provide only one level of logging with possible formatting. Then you could provide a supplementary LogFunc
struct,
client.Logger = retryablehttp.LogFunc(func(args ...interface{}){
logrus.Println(args)
// or
// logrus.Debugln(args)
})
and similar for formatted logging if needed.
Interface would be defined as:
type Logger interface {
Log(...interface{})
}
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.
type LogFunc func(...interface{})
func (f LogFunc) Log(args ...interface{}) {
f(args...)
}
You can fix retryablehttp's client output to send to logrus with:
after creating the client but before any requests. |
@nicodemus26 has the right idea here. Closing this in favor of that approach. Thanks! |
In my opinion, closing this PR is a mistake. Dave Cheney presents this idea better than I likely will: https://dave.cheney.net/2017/01/23/the-package-level-logger-anti-pattern In short, by defining an interface, you can return the choice of the logging package to be used to the user. Although the suggestion presented here works for logrus, it is not as flexible as an interface would be. I can see how the LogHook functionality of retryablehttp makes this a bit more complex (since you cannot be sure exactly how users will use/abuse the logger), I still feel that an interface is a better solution for logging here. I'd be happy to open another PR so a full discussion could happen, but please let me know if you're absolutely dead set on this as the final answer. |
I think the likelier scenario for this package would be for us to use hclog (https://github.com/hashicorp/go-hclog) since that's what we're standardizing on internally. It does have an interface, can also expose stdlib loggers as needed, and also natively supports outputting JSON in (I believe) ELK-friendly format. |
I was just about to PR this exact fix myself, also to allow Logrus, but then found this PR. 😢 This is could be such a simple interface.
|
This is just a first step but a useful one for those of us using ELK stacks where non JSON formatted output is causing issues.
A next improvement is to allow us to disable the DEBUG lines