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

Grpc logging #81

Merged
merged 3 commits into from
Aug 1, 2018
Merged

Grpc logging #81

merged 3 commits into from
Aug 1, 2018

Conversation

smacker
Copy link
Contributor

@smacker smacker commented Jul 27, 2018

Fix: #47
Fix: #11

Changes:

  • -v flag overrides log level to debug (as discussed before)
  • remove internal gRPC logging, subscribe to the connection status change event. Dummy analyzer doesn't subscribe to statuses because it's expected to run without a connection to data server for it.
  • add custom gRPC logging interceptors for server and client. They log incoming/outcoming requests/responses

Reasoning:

Internal gRPC logging logs quite much useless staff without a possibility to filter it by level or anyhow else. The only useful logs I have noticed are connection change status.

Internal gRPC logging doesn't log calls which are very useful. Existing logrus middleware requires workaround because we use src-d/go-log instead of pure logrus. Another problem it logs only when a request is finished. In my experience, it's very useful to log when request has come too in production. (helps to debug cases when the server received request but never responded because of panic/restart/whatever)
It's also very useful for our SDK, when the changes are big enough you can see that request is actually processing instead of empty logs for a dozen seconds. To solve these problems I have created very simple interceptors based on src-d/go-log.

@smacker smacker requested review from carlosms and bzz July 27, 2018 14:13
Signed-off-by: Maxim Sukharev <maxim@sourced.tech>
Signed-off-by: Maxim Sukharev <maxim@sourced.tech>
Signed-off-by: Maxim Sukharev <maxim@sourced.tech>
Copy link
Contributor

@carlosms carlosms left a comment

Choose a reason for hiding this comment

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

👏

@smacker smacker merged commit ee4b06d into src-d:master Aug 1, 2018
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

Successfully merging this pull request may close these issues.

3 participants