search

infobloxopen / atlas-app-toolkit

This repository provides common Go utilities and helpers that are reusable from project-to-project. The goal is to prevent code duplication by encouraging teams to use and contribute to toolkit libraries. The toolkit is not a framework. Rather, it is a set of (mostly gRPC-related) plugins and helpers.
Apache License 2.0
99 stars 115 forks source link

Client interceptor for logging doesn't do much #191

Open kd7lxl opened 4 years ago

kd7lxl commented 4 years ago

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.

drewwells commented 4 years ago

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 commented 4 years ago

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