search

vkuznet / transfer2go

Distributed, loosely couple agent-based transferring system
MIT License
8 stars 2 forks source link

Switch to logrus logger #22

Closed rishiloyola closed 7 years ago

rishiloyola commented 7 years ago

Reference issue #18

rishiloyola commented 7 years ago

Yes, we have so many log calls. I am still making changes. Do not merge this request.

vkuznet commented 7 years ago

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.

rishiloyola commented 7 years ago

Without WithFields method, it will just add some prefix to logs. If you want to highlight fields and its values then we have to use WithFields method.

Reference : https://github.com/sirupsen/logrus#fields

rishiloyola commented 7 years ago

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 the interface of this function.

vkuznet commented 7 years ago

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 notifications@github.com 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.

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/vkuznet/transfer2go/pull/22#issuecomment-302617462

rishiloyola commented 7 years ago

Do you want to replace log by logrus in remaining files?

vkuznet commented 7 years ago

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 notifications@github.com 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: https://github.com/vkuznet/transfer2go/pull/22#issuecomment-302772906

rishiloyola commented 7 years ago

@vkuznet You can now merge this PR.