Allow disabling logging #85
Conversation
There is now a `log` package, which wraps the `logrus` methods used in oxy. In addition to the methods that are used, there is a `.Disable` method that disables logging by sending them to `/dev/null`.
roundrobin/rr.go
Outdated
| @@ -86,7 +86,8 @@ func (r *RoundRobin) ServeHTTP(w http.ResponseWriter, req *http.Request) { | |||
|
|
|||
| if log.GetLevel() >= log.DebugLevel { | |||
| //log which backend URL we're sending this request to | |||
| log.WithFields(log.Fields{"Request": utils.DumpHttpRequest(req), "ForwardURL": url}).Debugf("vulcand/oxy/roundrobin/rr: Forwarding this request to URL") | |||
| //log.WithFields(log.Fields{"Request": utils.DumpHttpRequest(req), "ForwardURL": url}).Debugf("vulcand/oxy/roundrobin/rr: Forwarding this request to URL") | |||
roundrobin/rebalancer.go
Outdated
| @@ -147,7 +147,8 @@ func (rb *Rebalancer) ServeHTTP(w http.ResponseWriter, req *http.Request) { | |||
|
|
|||
| if log.GetLevel() >= log.DebugLevel { | |||
| //log which backend URL we're sending this request to | |||
| log.WithFields(log.Fields{"Request": utils.DumpHttpRequest(req), "ForwardURL": url}).Debugf("vulcand/oxy/roundrobin/rebalancer: Forwarding this request to URL") | |||
| //log.WithFields(log.Fields{"Request": utils.DumpHttpRequest(req), "ForwardURL": url}).Debugf("vulcand/oxy/roundrobin/rebalancer: Forwarding this request to URL") | |||
| const DebugLevel = logrus.DebugLevel | ||
|
|
||
| func Debugf(format string, args ...interface{}) { | ||
| log.Debugf(format, args...) |
There was a problem hiding this comment.
I believe because of the extra layer, your wrapper will log incorrect lines, you should fix that.
Also I would like to avoid using global variables here.
There was a problem hiding this comment.
I believe because of the extra layer, your wrapper will log incorrect lines, you should fix that.
Could you elaborate on that or give an example? Were are simply passing the arguments through.
I got it wrong, you mean line numbers, right?
There was a problem hiding this comment.
Also I would like to avoid using global variables here.
Not sure how we could do that, apart from reintroducing something like the context logger that was removed in #50.
|
This is something we're facing too. But I have conflicted feelings about setting log settings per-module. Something I always like to rely on when merging a library, is whatever common settings I've linked to, are being honored, and someone downstream isn't able to override them without making it known to me. In general Logrus could do with a per-package or per-module log setting. Due to golang's mutable nature, such modifications are very difficult to track down in code. On the other hand, I also buy into the "You break it, you buy it" paradigm. So you might argue that is the caller's responsibility. |
|
I think in the end this is not what we want. We're favoring another approach: I would prefer returning the "context logger" that used to be supported in oxy, which was a field/option settable on the structs. That would allow us to customize the logger, and allow us to set specific log level for oxy. I prototyped that model, which you can find in the context-logger branch. (This branch only changes the Please tell us if you would merge something like that. |
|
Yes, I'm in favor of this. Since Oxy is always used as a library, it makes sense for it to not force a global logger. This also means the provided logger override can then apply whatever processing it needs. |
|
Okay, then let's continue in #93, which has the initial implementation for the |
There is now a
logpackage, which wraps thelogrusmethods used inoxy. In addition to the methods that are used, there is a
.Disablemethod that disables logging by sending them to
/dev/null.Fixes #83.