Skip to content
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

Logrus adapter for Logger #752

Closed
AlmogBaku opened this issue Aug 16, 2018 · 10 comments
Closed

Logrus adapter for Logger #752

AlmogBaku opened this issue Aug 16, 2018 · 10 comments

Comments

@AlmogBaku
Copy link

AlmogBaku commented Aug 16, 2018

I suggest we'll provide a Logrus adapter for our log interface.

This adapter will mainly be used to support features of gokit, i.e. "gokit's http-server logging"

@AlmogBaku
Copy link
Author

AlmogBaku commented Aug 16, 2018

simple example:

type serverLogger struct {
	*logrus.Entry
}
func (l serverLogger) Log(keyvals ...interface{}) error {
	if len(keyvals)%2 == 0 {
		logger := l.Entry
		for i := 0; i < len(keyvals); i += 2 {
			logger = logger.WithField(fmt.Sprint(keyvals[i]), keyvals[i+1])
		}
		logger.Error()
	} else {
		l.Error(keyvals)
	}
	return nil
}

@aswinmprabhu
Copy link
Contributor

I am new to Golang and this is my attempt at this issue. Any feedback would be appreciated.

@AlmogBaku
Copy link
Author

In the meanwhile, you can use the code I attached above. I'll send a PR later on

@aswinmprabhu
Copy link
Contributor

aswinmprabhu commented Sep 2, 2018

I would like to make a PR and contribute.
Shall I write tests and make a PR?

@ChrisHines
Copy link
Member

I would like to understand more about the rationale behind writing a log.Logger implementation backed by logrus. Should we instead add features to support some specific logrus behavior more directly in the go-kit/log/... packages? If the desire is simply to make logrus implement the log.Logger interface, does that belong in go-kit, in logrus, or somewhere else? Why?

@AlmogBaku
Copy link
Author

In our use-case, we decided to use logrus organization-wide. However, we using some components of go-kit(such as kithttp.NewServer) which require a kit.logger.

I think it would make sense to provide as a part of the kit an adapter for logrus (like we have adapters for other stuff here)

@dytomode
Copy link

dytomode commented Sep 2, 2018

go-kit http server only requires for a logger to implement this interface.

type Logger interface {
	Log(keyvals ...interface{}) error
}

You can use any struct you want if it has Log(keyvals ...interface{}) error
I don't see why logrus should be added to go-kit.

@AlmogBaku
Copy link
Author

I'm not suggesting to use Logrus, but to provide an adapter to our logger.
See the code here I attached above as a reference.

@ChrisHines
Copy link
Member

I am willing to review a PR that adds a package with a logrus adapter. Suggestions for the package path:

  • github.com/go-kit/kit/log/logrus
  • github.com/go-kit/kit/log/kitlogrus

@aswinmprabhu
Copy link
Contributor

I will make a PR after writing the tests

aswinmprabhu added a commit to aswinmprabhu/kit that referenced this issue Sep 5, 2018
ChrisHines pushed a commit that referenced this issue Sep 18, 2018
Add logrus adapter for Logger interface (#752)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants