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

etcdserver: add grpc interceptor to log info on incoming request to etcdserver #9990

Merged
merged 3 commits into from
Aug 11, 2018

Conversation

jingyih
Copy link
Contributor

@jingyih jingyih commented Aug 9, 2018

This is to improve debuggability of etcd v3. Added a grpc interceptor to log information on incoming requests to etcdserver.

The log output includes:

  • request starting time,
  • request handling latency,
  • remote client info,
  • request key count,
  • request byte size,
  • response key count,
  • response byte size,
  • request content (with value field redacted).

Example log output:

./etcdctl put foo bar
{"level":"debug","ts":1533772428.8998141,"caller":"v3rpc/interceptor.go:168","msg":"request stats","start time":1533772428.89934,"time spent":0.000403816,"remote":"127.0.0.1:46300","response type":"/etcdserverpb.KV/Put","request count":1,"request size":10,"response count":0,"response size":28,"request content":"key:\"foo\" value_size:3 "}

Logging is enable by --debug flag.

Fixes #9989

cc @jpbetz @gyuho

@gyuho
Copy link
Contributor

gyuho commented Aug 9, 2018

I don't see anywhere we use https://godoc.org/google.golang.org/grpc/peer#NewContext? Is peer information automatically embedded by middleware?

@jingyih
Copy link
Contributor Author

jingyih commented Aug 9, 2018

@gyuho I think the peer information is been added during grpc remote call? Here is what I found:

./vendor/google.golang.org/grpc/transport/http2_server.go:	s.ctx = peer.NewContext(s.ctx, pr)
./vendor/google.golang.org/grpc/transport/handler_server.go:	s.ctx = peer.NewContext(ctx, pr)
./vendor/google.golang.org/grpc/transport/http2_client.go:	ctx = peer.NewContext(ctx, t.getPeer())

@gyuho
Copy link
Contributor

gyuho commented Aug 9, 2018

@jingyih I see. Then, can you document those as a comment? Also, CIs are failing.

@jingyih
Copy link
Contributor Author

jingyih commented Aug 9, 2018

@gyuho Sounds good! I will add the comment.

@jingyih
Copy link
Contributor Author

jingyih commented Aug 9, 2018

@gyuho Regarding the failed CIs. Except for one go format issue, others seems to be caused by missing the grpc-middleware package in vendor. I saw there is scripts/updatedep.sh. Should I run this? Is there instructions on how to update the vendor? Thanks!

@gyuho
Copy link
Contributor

gyuho commented Aug 9, 2018

@jingyih Yes, ./scripts/updatedep.sh [dependency-import-path-goes-here] should work.

@jingyih
Copy link
Contributor Author

jingyih commented Aug 10, 2018

@gyuho After I ran updatedep.sh, I observed two changes to the Gopkg.lock file.

  1. the missing package in CI was added. (i.e., go-grpc-middleware)
  2. grpc package was downgraded from v1.14.0 to v1.13.0

I am aware of the PR from you which upgraded gprc to v1.14.0 recently, so I did some investigation on the version downgrade. In master branch, I see that the grpc version is still v1.13.0 under folder https://github.com/coreos/etcd/tree/master/vendor/google.golang.org/

Is this consistent with the Gopkg.lock file which says the grpc version is v1.14.0?

cc @jpbetz

@gyuho
Copy link
Contributor

gyuho commented Aug 10, 2018

@jingyih Ah, previous godep run seemed to fail on that. Let me fix it shortly, to bump up gRPC to v1.14.

@jingyih
Copy link
Contributor Author

jingyih commented Aug 10, 2018

@gyuho

I noticed that grpc moved transport package from their root folder to internal, in release 1.14.0 (https://github.com/grpc/grpc-go/releases/tag/v1.14.0)

On the other hand, etcd does have dependency on "google.golang.org/grpc/transport". (in ./clientv3/integration/server_shutdown_test.go)

Not sure if this is the reason grpc is v1.13.0 in etcd vendor folder.

}
var responseType string = info.FullMethod
var reqCount, respCount int64 = 0, 0
var reqSize, respSize int = 0, 0
Copy link
Contributor

Choose a reason for hiding this comment

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

No need = 0, 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. They are zero-valued by default. Thanks!

@gyuho
Copy link
Contributor

gyuho commented Aug 10, 2018

@jingyih Good catch!

Could you rebase from master? Should be fixed by #9994.

etcd server

To improve debuggability of etcd v3. Added a grpc interceptor to log
info on incoming requests to etcd server. The log output includes
remote client info, request content (with value field redacted), request
handling latency, response size, etc. Uses zap logger if available,
otherwise uses capnslog.

Also did some clean up on the chaining of grpc interceptors on server
side.
Rebased to master PR etcd-io#9994.  Fixed a Go format issue in
v3rpc/interceptor.go.  Updated vendor to include go-grpc-middleware.
@jpbetz
Copy link
Contributor

jpbetz commented Aug 10, 2018

LGTM, I'll wait on @gyuho's approval for this one since I was involved in pre-review.

@@ -90,7 +210,8 @@ func newStreamInterceptor(s *etcdserver.EtcdServer) grpc.StreamServerInterceptor
}
}

return prometheus.StreamServerInterceptor(srv, ss, info, handler)
err := handler(srv, ss)
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

We can just simply return handler(srv, ss)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure I will make the change.

@gyuho
Copy link
Contributor

gyuho commented Aug 10, 2018

@jingyih Just small nits, then should be good to merge. Thanks!

Code clean up in interceptor.go
Copy link
Contributor

@gyuho gyuho left a comment

Choose a reason for hiding this comment

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

lgtm thanks @jingyih !

@gyuho gyuho merged commit 3a03774 into etcd-io:master Aug 11, 2018
gyuho added a commit that referenced this pull request Aug 15, 2018
…-origin-release-3.3

Automated cherry pick of #9990
jpbetz added a commit that referenced this pull request Aug 20, 2018
…-origin-release-3.2-1534373481

etcdserver: cherry pick of #9990 to release-3.2
jpbetz added a commit that referenced this pull request Aug 20, 2018
…lease-3.1

etcdserver: cherry pick of #9990 to release 3.1
@jingyih
Copy link
Contributor Author

jingyih commented Aug 27, 2018

/cc @wenjiaswe

jingyih added a commit to jingyih/etcd that referenced this pull request Sep 29, 2018
jingyih added a commit that referenced this pull request Sep 29, 2018
@jingyih jingyih deleted the dev_workload_analysis_tool branch September 7, 2019 07:08
case *pb.PutResponse:
_req, ok := req.(*pb.PutRequest)
if ok {
reqCount = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

This count is 1 for PUTs, but 0 for other types of requests and not set for TXN, what does this represent? Just trying to understand. Thanks. @jingyih

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants