Add package stats, and export stats. #961

menghanl · 2016-10-31T21:48:43Z

Package stats exports RPC stats, including payload length and wire-length.

Package stats is for monitoring purpose only.
All stats APIs are experimental.

iamqizhao · 2016-11-02T21:12:47Z

rpc_util.go

-func encode(c Codec, msg interface{}, cp Compressor, cbuf *bytes.Buffer) ([]byte, error) {
-	var b []byte
-	var length uint
+func encode(c Codec, msg interface{}, cp Compressor, cbuf *bytes.Buffer, outgoingPayloadStats *stats.OutgoingPayloadStats) ([]byte, error) {


s/outgoingPayloadStats/outStats/

iamqizhao · 2016-11-02T21:35:02Z

rpc_util.go

@@ -311,11 +324,15 @@ func checkRecvPayload(pf payloadFormat, recvCompress string, dc Decompressor) er
 	return nil
 }

-func recv(p *parser, c Codec, s *transport.Stream, dc Decompressor, m interface{}, maxMsgSize int) error {
+func recv(p *parser, c Codec, s *transport.Stream, dc Decompressor, m interface{}, maxMsgSize int, incomingPayloadStats *stats.IncomingPayloadStats) error {


s/incomingPayloadStats/inStats/

iamqizhao · 2016-11-02T21:40:54Z

server.go

-	var cbuf *bytes.Buffer
+	var (
+		cbuf                 *bytes.Buffer
+		outgoingPayloadStats *stats.OutgoingPayloadStats


change "outgoing/Outgoing" to "out/Out" and "incoming/Incoming" to "in/In" in all the places.

iamqizhao · 2016-11-02T21:47:38Z

stats/stats.go

+ *
+ */
+
+// Package stats reports stats for gRPC.


Package stats is for collecting and reporting various network and RPC stats.

iamqizhao · 2016-11-02T21:48:52Z

stats/stats.go

+
+// Stats contains stats information about RPCs.
+// All stats types in this package implements this interface.
+type Stats interface {


name it RPCStats since you will add connection stats later?

iamqizhao · 2016-11-02T22:30:35Z

stats/stats.go

+
+// OutgoingHeaderStats indicates a header is sent.
+// Method, addresses and Encryption are only valid if IsClient is true.
+type OutgoingHeaderStats struct {


iamqizhao · 2016-11-02T22:30:44Z

stats/stats.go

+func (s *OutgoingHeaderStats) isStats() {}
+
+// OutgoingTrailerStats indicates a trailer is sent.
+type OutgoingTrailerStats struct {


iamqizhao · 2016-11-02T22:31:23Z

stats/stats.go

+func (s *OutgoingTrailerStats) isStats() {}
+
+// ErrorStats indicates an error happens.
+type ErrorStats struct {


iamqizhao · 2016-11-02T22:34:33Z

stats/stats.go

+}
+
+// RegisterHandler registers the user handler function and starts the stats collection.
+// This handler function will be called to process the stats.


add something to prevent it from calling multiple times?

iamqizhao · 2016-11-02T22:34:57Z

stats/stats.go

+	start()
+}
+
+// start starts the stats collection.


should we export this too?

iamqizhao · 2016-11-04T00:48:49Z

stats/stats.go

+	IsClient() bool
+}
+
+// InPayload contains the information for a incoming payload.


an incoming

iamqizhao · 2016-11-04T00:49:21Z

stats/stats.go

+// RPCStats contains stats information about RPCs.
+// All stats types in this package implements this interface.
+type RPCStats interface {
+	// IsClient indicates if the stats is a client stats.


IsClient is true if this RPCStats is from client side.

iamqizhao · 2016-11-04T00:49:47Z

stats/stats.go

+// InPayload contains the information for a incoming payload.
+type InPayload struct {
+	// Client indicates if this stats is a client stats.
+	Client bool


Client is true if this InPayload is from client side.

iamqizhao · 2016-11-04T00:51:27Z

stats/stats.go

+}
+
+// IsClient indicates if the stats is a client stats.
+func (s *InPayload) IsClient() bool { return s.Client }


IsClient indicates if this is from client side.

iamqizhao · 2016-11-04T00:52:04Z

stats/stats.go

+// IsClient indicates if the stats is a client stats.
+func (s *InPayload) IsClient() bool { return s.Client }
+
+// InHeader indicates a header is received.


InHeader contains stats when a header is received.

iamqizhao · 2016-11-04T00:59:03Z

stats/stats.go

+type OutTrailer struct {
+	// Client indicates if this stats is a client stats.
+	Client bool
+	// WireLength is the wire length of header.


s/header/trailer

iamqizhao · 2016-11-04T00:59:11Z

stats/stats.go

+	WireLength int
+}
+
+// IsClient indicates if the stats is a client stats.


iamqizhao · 2016-11-04T00:59:18Z

stats/stats.go

+// IsClient indicates if the stats is a client stats.
+func (s *OutTrailer) IsClient() bool { return s.Client }
+
+// RPCErr indicates an error happens.


iamqizhao · 2016-11-04T00:59:25Z

stats/stats.go

+
+// RPCErr indicates an error happens.
+type RPCErr struct {
+	// Client indicates if this stats is a client stats.


iamqizhao · 2016-11-04T00:59:32Z

stats/stats.go

+	Error error
+}
+
+// IsClient indicates if the stats is a client stats.


menghanl · 2016-11-04T17:54:44Z

Thanks for the review. All fixed. PTAL.

iamqizhao · 2016-11-04T21:02:57Z

stats/stats.go

+
+// On indicates whether stats is started.
+func On() bool {
+	return atomic.LoadInt32(on) == 1


Better use CompareAndSwapInt32.

iamqizhao · 2016-11-04T21:04:29Z

stats/stats.go

+	return atomic.LoadInt32(on) == 1
+}
+
+// Handle returns the call back function registered by user to process the stats.


Handle returns the function registered to process the stats.

iamqizhao · 2016-11-04T21:07:18Z

stats/stats.go

+}
+
+// Start starts the stats collection.
+// Stats will only be started if handler is not nil.


Start starts the stats collection and reporting if there is a registered stats handle.

iamqizhao · 2016-11-04T21:07:43Z

stats/stats.go

+}
+
+// Stop stops the collection of any further stats.
+// Stop won't unregister handler.


Stop does not

iamqizhao · 2016-11-04T21:10:32Z

stats/stats.go

+	atomic.StoreInt32(on, 1)
+}
+
+// Stop stops the collection of any further stats.


Stop stops the stats collection and processing.

iamqizhao · 2016-11-04T22:22:00Z

call.go

@@ -63,14 +64,24 @@ func recvResponse(dopts dialOptions, t transport.ClientTransport, c *callInfo, s
 		return
 	}
 	p := &parser{r: stream}
+	var inStats *stats.InPayload


iamqizhao · 2016-11-04T22:22:46Z

call.go

 			if err == io.EOF {
 				break
 			}
 			return
 		}
 	}
+	if err == io.EOF && stream.StatusCode() == codes.OK && inStats != nil {
+		// TODO in the current implementation, inTrailer is handled before inStats. Fix the order if necessary.


deterministic?

iamqizhao · 2016-11-04T22:24:25Z

call.go

 			if err == io.EOF {
 				break
 			}
 			return
 		}
 	}
+	if err == io.EOF && stream.StatusCode() == codes.OK && inStats != nil {


put "inStats != nil" in the beginning of the condition.

iamqizhao · 2016-11-04T22:25:04Z

call.go

-	var cbuf *bytes.Buffer
+	var (
+		cbuf     *bytes.Buffer
+		outStats *stats.OutPayload


outPayload?

iamqizhao · 2016-11-05T00:06:22Z

call.go

 	"google.golang.org/grpc/transport"
 )

 // recvResponse receives and parses an RPC response.
 // On error, it returns the error and indicates whether the call should be retried.
 //
 // TODO(zhaoq): Check whether the received message sequence is valid.
-func recvResponse(dopts dialOptions, t transport.ClientTransport, c *callInfo, stream *transport.Stream, reply interface{}) (err error) {
+// TODO ctx is userCtx, not stream.Context. It is used for stats handling. Change this later if necessary.


ctx is used for stats collection and processing. It is the context passed from the application.

iamqizhao · 2016-11-07T18:52:56Z

transport/transport.go

+	// LocalAddr returns the local network address.
+	LocalAddr() net.Addr
+	// RemoteAddr returns the remote network address.
+	RemoteAddr() net.Addr


These 2 APIs are not needed.

iamqizhao · 2016-11-07T19:06:26Z

call.go

 	err = t.Write(stream, outBuf, opts)
+	if outPayload != nil {


should we check the value of "err"? what if it is not nil?

iamqizhao · 2016-11-07T19:11:47Z

stream.go

+		Host:     cc.authority,
+		Method:   method,
+		Flush:    desc.ServerStreams && desc.ClientStreams,
+		FailFast: c.failFast,


Is it okay to put fail fast into stats.Begin so that we do not need to pass it into the transport layer? You can refactor NewClientStream and newClientStream somehow (e.g., move opt processing to NewClientStream).

iamqizhao · 2016-11-07T19:20:23Z

stream.go

@@ -275,6 +311,16 @@ func (cs *clientStream) SendMsg(m interface{}) (err error) {
 		cs.mu.Unlock()
 	}
 	defer func() {
+		if err != nil && stats.On() {
+			// Only handle end stats if err != nil.


what if err == nil?

iamqizhao · 2016-11-07T19:21:38Z

call.go

-			Method: method,
+			Host:     cc.authority,
+			Method:   method,
+			FailFast: c.failFast,


can we put fail fast into Begin stats so that it is not needed to pass it into transport?

iamqizhao · 2016-11-07T19:26:24Z

rpc_util.go

@@ -333,6 +350,11 @@ func recv(p *parser, c Codec, s *transport.Stream, dc Decompressor, m interface{
 	if err := c.Unmarshal(d, m); err != nil {
 		return Errorf(codes.Internal, "grpc: failed to unmarshal the received message %v", err)
 	}
+	if inPayload != nil {
+		inPayload.Payload = m
+		inPayload.Data = d


add a TODO to truncate the large payload. Also for outPayout.

iamqizhao · 2016-11-07T19:28:24Z

server.go

@@ -547,10 +554,36 @@ func (s *Server) sendResponse(t transport.ServerTransport, stream *transport.Str
 		// the optimal option.
 		grpclog.Fatalf("grpc: Server failed to encode response %v", err)
 	}
-	return t.Write(stream, p, opts)
+	if outPayload != nil {
+		outPayload.SentTime = time.Now()


should this happen after write returns?

iamqizhao · 2016-11-07T22:17:38Z

server.go

@@ -569,6 +600,13 @@ func (s *Server) processUnaryRPC(t transport.ServerTransport, stream *transport.
 	p := &parser{r: stream}
 	for {
 		pf, req, err := p.recvMsg(s.opts.maxMsgSize)
+		var inPayload *stats.InPayload


what if err is not nil?

iamqizhao · 2016-11-07T22:22:05Z

server.go

 		}
 		statusCode := codes.OK
 		statusDesc := ""
 		df := func(v interface{}) error {
+			if inPayload != nil {


consider the error case

iamqizhao · 2016-11-07T22:36:02Z

server.go

 				}
+				return Errorf(codes.Internal, err.Error())


remove this?

iamqizhao · 2016-11-07T22:51:42Z

server.go

 			if pf == compressionMade {
 				var err error
 				req, err = s.opts.dc.Do(bytes.NewReader(req))
 				if err != nil {
 					if err := t.WriteStatus(stream, codes.Internal, err.Error()); err != nil {
 						grpclog.Printf("grpc: Server.processUnaryRPC failed to write status %v", err)
 					}
-					return err
+					return Errorf(codes.Internal, err.Error())


The reason for this change?

iamqizhao · 2016-11-07T23:08:29Z

server.go

+			}
+			stats.Handle(stream.Context(), end)
+		}
+	}()


Can we move this part (from line 736-754 to the caller of processStreamingRPC and processUnaryRPC) since they are common for both cases?

iamqizhao · 2016-11-08T02:52:35Z

stream.go

@@ -274,6 +300,7 @@ func (cs *clientStream) SendMsg(m interface{}) (err error) {
 		}
 		cs.mu.Unlock()
 	}
+	// TODO generate stats.End if err != nil && err != io.EOF.


TODO: Investigate how to signal the stats handling party.

iamqizhao · 2016-11-08T03:28:40Z

stream.go

+				Client:  true,
+				EndTime: time.Now(),
+				Error:   e,
+			}


if stats.On() { end := &stats.End{ Client: true, EndTime. time.Now(), } if err != nil && err != io.EOF { end.Error = toRPCErr(err) } }

iamqizhao · 2016-11-08T21:27:01Z

transport/http2_server.go

+		inHeader := &stats.InHeader{
+			FullMethod: s.method,
+			RemoteAddr: t.conn.RemoteAddr(),
+			LocalAddr:  t.conn.LocalAddr(),


There is a chance the t.conn is broken when these are called so that they cannot get the values. You should probably cache them when the connection is setup.

iamqizhao · 2016-11-08T23:32:51Z

stats/grpc_testing/test.proto

+package grpc.testing;
+
+// Unary request.
+message SimpleRequest {


not a unary request since it is also used in FullDuplexCall.

iamqizhao · 2016-11-08T23:33:02Z

stats/grpc_testing/test.proto

+}
+
+// Unary response, as configured by the request.
+message SimpleResponse {


iamqizhao · 2016-11-08T23:59:11Z

stats/stats_test.go

+	)
+	tc := testpb.NewTestServiceClient(te.clientConn())
+	if c.success {
+		req = &testpb.SimpleRequest{Id: 1}


use errorID+1 instead?

iamqizhao · 2016-11-09T00:35:48Z

stats/stats_test.go

+	outheader
+	outtrailer
+	errors
+)


capitalize the 1st letter of the second word in the strings.

iamqizhao · 2016-11-09T00:38:51Z

stats/stats_test.go

+	}
+}
+
+func TestServerStatsUnaryRPC(t *testing.T) {


Is there a way we can verify the contexts from the stats of a particular rpc are same?

iamqizhao · 2016-11-09T02:23:25Z

server.go

+	if !ok {
+		grpclog.Fatalf("cannot get trace from context while EnableTracing == true")
+	}
+


Due the latest change, it seems this function can be simplified as:

tr, ok := trace.FromContext(stream.Context()) if !ok { return nil } ...

iamqizhao · 2016-11-09T02:26:46Z

And you need to fix transport_test.go.

- cloud.google.com/go: nothing of note. - github.com/cockroachdb/c-jemalloc: - Update upstream to 4.4.0 (cockroachdb/c-jemalloc@910bc3a) - github.com/cockroachdb/cmux: nothing of note. - github.com/coreos/etcd: nothing of note. - github.com/elastic/gosigar: nothing of note. - github.com/gogo/protobuf: - Allow usage of message types with customtype (gogo/protobuf@f9114da) - github.com/golang/protobuf: nothing of note. - github.com/google/go-github: nothing of note. - github.com/grpc-ecosystem/grpc-gateway: nothing of note. - github.com/lib/pq: - Use space to separate date and time portions of Time literals (lib/pq@8af01c1) - github.com/lightstep/lightstep-tracer-go: - Remove vendor directory (lightstep/lightstep-tracer-go@70a2e8a) - github.com/opentracing/opentracing-go: - Avoid panic marshaling nil error (opentracing/opentracing-go@5e5abf8) - github.com/prometheus/common: nothing of note. - golang.org/x/net: nothing of note. - golang.org/x/tools: nothing of note. - google.golang.org/grpc: - Add stats (grpc/grpc-go#961) - Add transport tap (grpc/grpc-go#968) - Add FailOnNonTempDialError (grpc/grpc-go#974 & grpc/grpc-go#985) - honnef.co/go/staticcheck: nothing of note. - honnef.co/go/unused: nothing of note.

menghanl force-pushed the add_stats_package branch 4 times, most recently from e18a430 to 78d4f0d Compare November 2, 2016 17:18

iamqizhao reviewed Nov 2, 2016

View reviewed changes

menghanl changed the title ~~Add package stats, and export stats on server side.~~ Add package stats, and export stats. Nov 3, 2016

menghanl force-pushed the add_stats_package branch 3 times, most recently from d8d33f8 to ca1e7b9 Compare November 3, 2016 23:19

iamqizhao reviewed Nov 4, 2016

View reviewed changes

iamqizhao reviewed Nov 7, 2016

View reviewed changes

menghanl added 15 commits November 7, 2016 15:16

Add package stats, and export stats on server side.

17ee5a3

fix race in test and add import comments

46e80bf

change CallBack to handler, and move ctx out of Stats.

7984a9c

add errorStats

85c5e0e

add payload with original type to payload stats

1054f1c

add isclient to errorstats

aa91466

remove initStats, merge into headerStats

adaae03

change stats interface API to include ClientStats() function

2de8bac

add client stats

e42a66c

Renaming types and variables

c698588

change behavior of stats register handler and start stop

be66502

change package stats comment

0d345b5

add failfast option to stats

c0087b3

remove isStats from interface

e478486

change stats comments

8a126b0

menghanl force-pushed the add_stats_package branch 2 times, most recently from ed61dca to 433d594 Compare November 8, 2016 01:55

fix comments

badd554

menghanl force-pushed the add_stats_package branch 2 times, most recently from 99efc01 to 69b1142 Compare November 8, 2016 02:07

remote end stats in header() and SendMsg()

3de821b

menghanl force-pushed the add_stats_package branch from 69b1142 to 3de821b Compare November 8, 2016 02:30

iamqizhao reviewed Nov 8, 2016

View reviewed changes

fix comments

ff0b1db

iamqizhao reviewed Nov 8, 2016

View reviewed changes

cache conn LocalAddr and RemoteAddr

0b062e4

iamqizhao reviewed Nov 8, 2016

View reviewed changes

remove wrong comments from stats testing proto

11b929b

iamqizhao reviewed Nov 9, 2016

View reviewed changes

menghanl added 2 commits November 8, 2016 16:48

fix comments in stats testing

183e5e9

fix wrong context when handling stats

6445ded

menghanl force-pushed the add_stats_package branch from afb1a30 to 6445ded Compare November 9, 2016 02:17

iamqizhao reviewed Nov 9, 2016

View reviewed changes

menghanl added 3 commits November 8, 2016 21:22

fix failing tests

f80f54e

remove Stream.TraceContext

f511c49

fixes in test

88bdb95

iamqizhao merged commit c098900 into grpc:master Nov 9, 2016

tamird mentioned this pull request Jan 9, 2017

Update dependencies cockroachdb/cockroach#12789

Merged

menghanl deleted the add_stats_package branch June 6, 2017 01:34

menghanl added 1.4 and removed 1.4 labels Jun 7, 2017

soltysh mentioned this pull request Jan 11, 2018

bump(google.golang.org/grpc): 708a7f9f3283aa2d4f6132d287d78683babe55c8 openshift/origin#18071

Closed

lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019

Add package stats, and export stats. #961

Add package stats, and export stats. #961

Conversation

menghanl commented Oct 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

menghanl commented Nov 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamqizhao commented Nov 9, 2016