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

fix: allow grpc logger to truncate specific methods output #1360

Merged
merged 2 commits into from
Aug 30, 2018

Conversation

zavgorodnii
Copy link
Contributor

Short method names should be provided, i.e., PushTask, not /sonm.Worker/PushTask.

@zavgorodnii zavgorodnii added 💩 T: bug This PR/Issue fixes a bug P: low This PR/Issue has minor priority labels Aug 29, 2018
@@ -240,40 +240,48 @@ func verifyUnaryInterceptor() grpc.UnaryServerInterceptor {
//
// Note, that to enable tracing logging you should specify this option AFTER
// trace interceptors.
func RequestLogInterceptor(log *zap.Logger) ServerOption {
func RequestLogInterceptor(log *zap.Logger, truncatedMethods []string) ServerOption {
var truncatedMethodsSet = map[string]bool{}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is more common to use map[string]struct{} for sets (yep, go sucks)

return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {
executeRequestLogging(ctx, req, info.FullMethod, log)
executeRequestLogging(ctx, req, info.FullMethod, log, truncatedMethods[MethodInfo(info.FullMethod).Method])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you test it? Because go panics on missing key

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nuh-uh no it doesn't

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, it looks like I was not drunk

@antmat antmat merged commit bfa3632 into master Aug 30, 2018
@antmat antmat deleted the fix/reduce-logging-for-stream-methods branch August 30, 2018 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P: low This PR/Issue has minor priority 💩 T: bug This PR/Issue fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants