Switch to logrus logger #22
Conversation
client/client.go
Outdated
| log.Println("Unable to resolve destination", dst, "known agents", remoteAgents) | ||
| log.WithFields(log.Fields{ | ||
| "Destination": dst, | ||
| "known agents": remoteAgents, |
There was a problem hiding this comment.
I prefer to keep keys CamelCase, in this case "known agents" should be replaced with Agents. Also, I think you can simply use the same semantics as log.Println() shows and it will properly formatted. Please try out and experiement with that. The main point is to get log in json format. I also want to move logger definition into config, i.e. in config we should specify which logger we want to use, e.g.
// set logger settings depending on configuration
if strings.Contains(strings.ToLower(config.LogFormatter), "json") {
log.SetFormatter(&log.JSONFormatter{})
}
if strings.Contains(strings.ToLower(config.LogLevel), "info") {
log.SetLevel(log.InfoLevel)
}
if strings.Contains(strings.ToLower(config.LogLevel), "warn") {
log.SetLevel(log.WarnLevel)
}
if strings.Contains(strings.ToLower(config.LogLevel), "err") {
log.SetLevel(log.ErrorLevel)
}
if strings.Contains(strings.ToLower(config.LogLevel), "debug") {
log.SetLevel(log.DebugLevel)
}
client/client.go
Outdated
| log.Println("ERROR fail with transfer request to", r.Url) | ||
| log.WithFields(log.Fields{ | ||
| "Url": r.Url, | ||
| }).Error("ERROR fail with transfer request to", r.Url) |
There was a problem hiding this comment.
Here we can take advantage of logrus to report Error so we don't need ERROR in error message.
| @@ -81,7 +82,7 @@ func main() { | |||
| config.Register = agent | |||
| } | |||
| if config.Register == "" { | |||
| log.Println("WARNING this agent is not registered with remote ones, either provide register in your config or invoke register API call") | |||
| log.Warn("WARNING this agent is not registered with remote ones, either provide register in your config or invoke register API call") | |||
There was a problem hiding this comment.
Here we don't need WARNING in a message.
|
Yes, we have so many log calls. I am still making changes. Do not merge this request. |
|
Rishi, before you going further. I want to raise a question, do you really need to replace log.Println with log.WithFields, the problem with such replacement is that if we need to rollback to standard logger we can't do it since standard logger does not have method WithFields and we'll need to redo all the work again. It would be nice to have transparent switch. So, hold on changes and think about it. |
|
Without Reference : https://github.com/sirupsen/logrus#fields |
|
Also, logger is not fully compatible with |
|
Rishi,
I saw that logrus apis is not fully backward compatible with logger. Originally
I thought otherwise, my fault. Since you already made changes to WithFields,
let's keep them.
Regarding mixture of logs as you pointed out. There is nothing wrong with that,
we just need to use both, e.g.
```
import (
"log"
"github.com/sirupsen/logrus"
)
logrus.Info()
log.Println()
```
or apply aliases as we wish.
…On 0, Rishi ***@***.***> wrote:
Also, logger is not fully compatible with `log`. If you want to use logger in `business.go` file then we need to make some changes in interface of this [function](https://github.com/vkuznet/transfer2go/blob/master/core/business.go#L168).
--
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#22 (comment)
|
|
Do you want to replace log by logrus in remaining files? |
|
Yes, the usage of standard log should be limited only to places where metrics
require it. All other places we can safely replace with logrus.
…On 0, Rishi ***@***.***> wrote:
Do you want to replace log by logrus in remaining files?
--
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#22 (comment)
|
|
@vkuznet You can now merge this PR. |
Reference issue #18