atlas-app-toolkit icon indicating copy to clipboard operation
atlas-app-toolkit copied to clipboard

Client interceptor for logging doesn't do much

Open kd7lxl opened this issue 5 years ago • 2 comments

https://github.com/infobloxopen/atlas-app-toolkit/blob/c488b8e6a72df08a2cd3e7e4af64ba370d77c1f3/logging/interceptor.go#L65-L72

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.

kd7lxl avatar May 20 '20 16:05 kd7lxl

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.

drewwells avatar May 20 '20 20:05 drewwells

I agree, we should just use logrus's interceptor

That was also suggested during the review: https://github.com/infobloxopen/atlas-app-toolkit/pull/190#pullrequestreview-411022396

kd7lxl avatar May 20 '20 21:05 kd7lxl