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

grpc_logrus: Use FieldLogger #52

Closed
sagikazarmark opened this issue May 30, 2017 · 6 comments
Closed

grpc_logrus: Use FieldLogger #52

sagikazarmark opened this issue May 30, 2017 · 6 comments

Comments

@sagikazarmark
Copy link

Haven't checked the code yet, but I saw in the documentation that the logrus middleware awaits a logrus.Entry. Wouldn't it be better to rely on the logrus.FieldLogger interface?

@mwitkow
Copy link
Member

mwitkow commented Jun 1, 2017

gTBH it's relatively easy to pass in an Entry using https://godoc.org/github.com/sirupsen/logrus#NewEntry

What are the benefitsof logrus.FieldLogger above that?

@sagikazarmark
Copy link
Author

sagikazarmark commented Jun 1, 2017

It's an interface fulfilled by logrus.Entry and logrus.Logger as well. Also, I don't see how using Entry or Logger internals is necessary, so hiding those behind the FieldLogger API makes the code more stable by depending on a contract IMHO.

@mwitkow
Copy link
Member

mwitkow commented Jun 9, 2017

Sounds sensible. Wanna submit a PR? :)

@sagikazarmark
Copy link
Author

Sure, but don't know when I will find some time to do so, so feel free to pick it if you want.

@mrbanzai
Copy link

mrbanzai commented Sep 14, 2017

I started to take a stab at this, but the payload interceptor expects to be able to access the Data variable of a logrus.Entry returned from Extract:

logEntry := entry.WithFields(Extract(ctx).Data)

It might be possible to return a logrus.Entry for a logrus.FieldLogger by calling logrus.WithFields(logrus.Fields{}), but that feels dirty.

@bwplotka
Copy link
Collaborator

This is pretty old and we know have v2 code with different structure. Let us know if this is still important, we can reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants