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

Extract zap logger from context for logging server interceptors #476

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

implmnt
Copy link

@implmnt implmnt commented Nov 29, 2021

This PR extends the current zap logging server interceptors API.
It makes possible to reuse existing logger from GRPC request context for logging user's contextual data (e.g. the user_id that was shifted before into the logging context).

@johanbrandhorst
Copy link
Collaborator

Hi Georgy. I'm not sure this is a change we want to make, could you explain the rationale for this new API? Please note that we have moved all new feature development to the v2 branch too, where we have a new structure for these sort of implementations.

@implmnt
Copy link
Author

implmnt commented Nov 30, 2021

Hi Johan. Thanks for notifying me about v2 branch. Will check it too.

The reason of the new API is making possible to reuse logger that was inited in some before step.

For expample:
I have an interceptor which lifts user_id to logger context and I expect to see it through all logs of whole application.
The current logging interceptor allows to use the only logger that was passed through its context-less constructor and does not provide access to the request context.
func UnaryServerInterceptor(logger *zap.Logger, opts ...Option) grpc.UnaryServerInterceptor {

So I think it would be better to have a more flexible API that allows extracting logger from request context with some predefined fields like user_id.

@johanbrandhorst
Copy link
Collaborator

@bwplotka do you have any thoughts on this?

@bwplotka
Copy link
Collaborator

bwplotka commented Apr 6, 2022

Sorry for lag!

Yes, while this is great, we have opportunity to change things on v2 - we would love to not add new features to current v1 (master)

@devnev devnev added the v1 label Oct 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants