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

Client interceptor for logging doesn't do much #191

Open
kd7lxl opened this issue May 20, 2020 · 2 comments
Open

Client interceptor for logging doesn't do much #191

kd7lxl opened this issue May 20, 2020 · 2 comments

Comments

@kd7lxl
Copy link
Contributor

kd7lxl commented May 20, 2020

func UnaryClientInterceptor(entry *logrus.Entry, opts ...Option) grpc.UnaryClientInterceptor {
options := initOptions(opts)
return func(ctx context.Context, method string, req, reply interface{}, cc *grpc.ClientConn, invoker grpc.UnaryInvoker, opts ...grpc.CallOption) error {
startTime := time.Now()
fields := newLoggerFields(method, startTime, DefaultClientKindValue)
setInterceptorFields(ctx, fields, entry.Logger, options, startTime)

I'm curious what the use cases for this client interceptor are and if they work. The way it's implemented, it doesn't seem to do anything useful. It adds fields like account_id to the logger sourced from the incoming metadata, while clients utilize outgoing metadata. This is probably a bug. It seems like it would be more valuable to log the subject that is being used in the client, not the subject used in whatever call triggered it (which is only set for service-to-service cases, like the test exercises). In the service-to-service case, I would expect these fields to already be set in the logger by the server interceptor, meaning the client interceptor isn't adding anything beyond what the logrus client interceptor already does.

@drewwells
Copy link
Contributor

I agree, we should just use logrus's interceptor https://github.com/grpc-ecosystem/go-grpc-middleware/blob/master/logging/logrus/client_interceptors.go#L17
An interceptor on our side could be an abstraction to the underlying logger, but this code appears to make assumptions that logrus is using.

@kd7lxl
Copy link
Contributor Author

kd7lxl commented May 20, 2020

I agree, we should just use logrus's interceptor

That was also suggested during the review: #190 (review)

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

2 participants