contrib/google.golang.org/grpc: streaming interceptor #277

dd-caleb · 2018-07-23T21:34:54Z

This is an implementation of the streaming interceptor for gRPC. Much of the code was reused from the existing unary interceptors.

A few questions:

This approach generates a trace on every send/recv of the stream on both sides. Should we be concerned about generating too many traces?
Should the send/recv traces have the same tags as the existing rpc traces? It would not be hard to add this if we want it.
The client stream is kind of complicated because it returns before the stream is closed, which means we have to delay finishing the span. From reading the documentation in the source code I think waiting for stream.Context.Done() should signal that the span should finish, but I'm not 100% sure. We should probably do a more exhaustive test on this so we're not leaking goroutines.

If the number of traces and tagging makes sense, I will add more unit tests.

mbyio · 2018-07-23T22:17:04Z

Thanks for starting on this so quickly! Some context - we currently are using streaming RPCs just to split up large unary messages (eg. for a file upload) so we might prefer to have the entire duration of a stream show as a single trace. But I could see a use case for one trace per send/recv too, eg. for streaming notifications from one service to another. Unfortunately, I'm not sure what the best strategy is.

dd-caleb · 2018-07-23T23:06:26Z

@mbyio thank you for the feedback! The current implementation will trace both the main stream request and the messages sent back and forth. In this example there were 10 sends/recvs:

One solution could be an interceptor option to change what is traced for the streaming interceptor (currently our only interceptor option for gRPC is the service name). The default would still be full tracing, but the send/recv trace could be disabled, or vice-versa the outer start-stream trace. (I think there are circumstances where streaming could be used for a very long time, as basically a long-lived connection, and a trace for the entire duration isn't particularly useful) This is not hard to add so I could look into it tomorrow.

mbyio · 2018-07-23T23:10:11Z

Oh awesome! Thanks!

gbbr

Thanks for doing this. It will definitely come in handy. I left some small nits but am happy to do a more thorough review as soon as you take off the WIP label.

It's looking really good.

gbbr · 2018-07-24T08:17:19Z

contrib/google.golang.org/grpc/client.go

+	return err
+}
+
+// StreamClientInterceptor will trace streaming requests for the given gRPC client


// StreamClientInterceptor returns a grpc.StreamClientInterceptor which will trace client // streams using the given set of options.

I think this would be more correct because there isn't a "given gRPC client" here, in the context of this function signature.

gbbr · 2018-07-24T08:19:16Z

contrib/google.golang.org/grpc/client.go

+	}
+}
+
+// UnaryClientInterceptor will add tracing to a gprc client.


Can we do something similar here?

// UnaryClientInterceptor returns a unary client interceptor which will trace requests using // the given set of options.

gbbr · 2018-07-24T08:22:43Z

contrib/google.golang.org/grpc/client.go

+	}
+}
+
+// startTraceClientRequest is used by both the unary and streaming interceptors


In case you've not noticed yet I'm picky with docs 😄 We should say what the function does, not where it is used, so maybe:

// doClientRequest performs the given client requests, returning the wrapping span.

It might also be more Go-like to call it simply doClientRequest (like http.Do). The context of this whole library is tracing so we shouldn't necessarily use the word everywhere.

gbbr · 2018-07-24T08:24:31Z

contrib/google.golang.org/grpc/client.go

+	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer"
+)
+
+type tracingClientStream struct {


Let's minimize the use of the word "tracing" and just call it clientStream. We did the same in all other integrations. Note that this package is usually imported as grpctrace based on our examples. Possibly it should be called that but that's a different story. In that context, thinking grpctrace.clientStream is better than grpctrace.tracingClientStream (even if this is an unexported structure).

gbbr · 2018-07-24T08:26:19Z

contrib/google.golang.org/grpc/client.go

+func startTraceClientRequest(
+	ctx context.Context, method string, serviceName string, opts []grpc.CallOption,
+	handler func(ctx context.Context, opts []grpc.CallOption) error,
+) (tracer.Span, error) {


I know this is confusing and possibly not the best design decision (happy to hear suggestions), but let's try to use ddtrace.Span because that's the original interface. tracer.Span is just an alias I've used to get better results from godoc.

gbbr · 2018-07-24T08:26:43Z

contrib/google.golang.org/grpc/client.go

+	if !ok {
+		md = metadata.MD{}
+	}
+	_ = tracer.Inject(span.Context(), grpcutil.MDCarrier(md))


I think we should either find a way to handle an error or completely remove the _ = part as it doesn't serve any purpose (unless it's a reminder for you to handle it)

gbbr · 2018-07-24T08:27:56Z

contrib/google.golang.org/grpc/server.go

+	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer"
+)
+
+type tracingServerStream struct {


Let's do the same here and just use serverStream

gbbr · 2018-07-24T08:32:56Z

One solution could be an interceptor option to change what is traced for the streaming interceptor (currently our only interceptor option for gRPC is the service name). The default would still be full tracing, but the send/recv trace could be disabled, or vice-versa the outer start-stream trace. (I think there are circumstances where streaming could be used for a very long time, as basically a long-lived connection, and a trace for the entire duration isn't particularly useful) This is not hard to add so I could look into it tomorrow.

I think this solution sounds like the way to go. Start full fledged and then be able to narrow it down or fine tune it based on options. This is one of those cases where having what Opentracing calls "logging" or Opencensus calls "annotations" would come in handy.

gbbr · 2018-07-24T08:36:46Z

contrib/google.golang.org/grpc/client.go

+			return nil, err
+		}
+		go func() {
+			<-stream.Context().Done()


You've expressed your concern in the PR description about leaking here. This is indeed tricky. I've looked at the code myself and it seems that this context is cancelled only when the stream closes with a nil error.

This is a very brute idea but we could possibly introduce a timeout here as a temporary workaround. Although it's hard to say how long that timeout should be. In the case where there is a MethodConfig set with a timeout, then we have no problem here because the context will always be cancelled, but that's not something we can rely on. Not sure what else we can do to ensure we don't leak here, WDYT?

We should also handle the error from the returned context, whether we use a timeout or not.

ctx, _ := context.WithTimeout(stream.Context(), ???) <-ctx.Done() span.Finish(tracer.WithError(ctx.Err()))

Looks like the gRPC code does:

select { case <-cc.ctx.Done(): cs.finish(ErrClientConnClosing) case <-ctx.Done(): cs.finish(toRPCErr(ctx.Err())) }

And finish does:

cs.cancel()

And the docs say:

// To ensure resources are not leaked due to the stream returned, one of the following // actions must be performed: // 1. Call Close on the ClientConn. // 2. Cancel the context provided. // 3. Call RecvMsg until a non-nil error is returned. A protobuf-generated // client-streaming RPC, for instance, might use the helper function // CloseAndRecv (note that CloseSend does not Recv, therefore is not // guaranteed to release all resources). // 4. Receive a non-nil, non-io.EOF error from Header or SendMsg.

So I think we will only leak if gRPC was already going to leak.

gbbr · 2018-07-24T08:38:53Z

contrib/google.golang.org/grpc/grpc.go

-		span.SetTag("grpc.code", grpc.Code(err).String())
-		span.Finish(tracer.WithError(err))
-		return err
+// withStreamError does a tracer.WithError but ignores EOF and Canceled errors


// withStreamError returns a tracer.WithError finish option, disregarding OK, EOF and Canceled errors.

dd-caleb · 2018-07-24T20:23:35Z

@gbbr when you get a chance, I made the following changes:

rename tracingClientStream/tracingServerStream to clientStream/serverStream
rename startTraceClientRequest to doClientRequest
update various docs
handle tracer.Inject error
allow control over how much is traced when streaming using a new StreamMode
extract setSpanTargetFromPeer method and add target tags for send/recv messages and streaming calls
fix a bug when setting the span error from the stream context
extract a injectSpanIntoContext method
create some constants for tag names
create a more exhaustive streaming test
change message operation names to match the java integration

The new StreamMode can be used to adjust what is traced for streaming:

grpctrace.StreamServerInterceptor(
	grpctrace.WithServiceName("example-grpc-server"),
	grpctrace.WithStreamMode(grpctrace.StreamModeCalls),
)

It defaults to tracing both calls and messages

gbbr

Thanks Caleb. Just a final set of thoughts. Otherwise LGTM

gbbr · 2018-07-25T09:54:46Z

contrib/google.golang.org/grpc.v12/tags.go

+
+// Tags used for gRPC
+const (
+	TagMethod = "grpc.method"


Do these need to be exported? They aren't used anywhere else.

gbbr · 2018-07-25T09:57:17Z

contrib/google.golang.org/grpc/grpc_test.go

+	}
+
+	t.Run("All", func(t *testing.T) {
+		mt.Reset()


I'm not sure if sharing the mocktracer is a good idea. Don't these sub-tests run concurrently? That could cause trouble. I think we should have a separate mocktracer for each test.

gbbr · 2018-07-25T10:04:07Z

contrib/google.golang.org/grpc/option.go

-type interceptorConfig struct{ serviceName string }
+// The StreamMode is used to control what is traced in the
+// StreamClientInterceptor and StreamServerInterceptor
+type StreamMode byte


While I really love this pattern, I feel it's overkill here. I find it works well when there are many options and many combinations of them, while in this case we have only two:

Trace calls

Trace messages

The third one is implied when both are set. I'd dumb it down completely from an API standpoint, getting rid of bitwise operations and simply having options such as grpctrace.WithStreamCalls and grpctrace.WithStreamMessages. Much simpler IMO, what do you think?

gbbr

Thanks a lot for your patience, and for implementing this. It looks great!

Feel free to (squash &) merge whenever you like. One last comment I’d make is that generally I’ve tried my best to finish all (public) documentation sentences with a period. It’s a comment I got myself on many PRs using Go.

- handle tracer.Inject error - allow control over how much is traced when streaming - extract setSpanTargetFromPeer method and add target tags for send/recv messages and streaming calls - extract a injectSpanIntoContext method - create some constants for tag names - create a more exhaustive streaming test

dd-caleb requested a review from gbbr July 23, 2018 21:35

gbbr added the do-not-merge/WIP label Jul 24, 2018

gbbr changed the title ~~[wip] streaming interceptor~~ contrib/google.golang.org/grpc: streaming interceptor Jul 24, 2018

gbbr reviewed Jul 24, 2018

View reviewed changes

dd-caleb force-pushed the caleb/grcp-streaming branch from 916080a to c71eeae Compare July 24, 2018 13:02

dd-caleb removed the do-not-merge/WIP label Jul 24, 2018

gbbr added this to the 1.1.0 milestone Jul 25, 2018

gbbr reviewed Jul 25, 2018

View reviewed changes

gbbr approved these changes Jul 25, 2018

View reviewed changes

dd-caleb force-pushed the caleb/grcp-streaming branch from 06dc214 to adc86d4 Compare July 25, 2018 19:18

dd-caleb merged commit fe1b672 into v1 Jul 25, 2018

dd-caleb deleted the caleb/grcp-streaming branch July 25, 2018 19:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

contrib/google.golang.org/grpc: streaming interceptor #277

contrib/google.golang.org/grpc: streaming interceptor #277

dd-caleb commented Jul 23, 2018

mbyio commented Jul 23, 2018

dd-caleb commented Jul 23, 2018

mbyio commented Jul 23, 2018

gbbr left a comment

gbbr Jul 24, 2018 •

edited

Loading

gbbr Jul 24, 2018

gbbr Jul 24, 2018

gbbr Jul 24, 2018

gbbr Jul 24, 2018

gbbr Jul 24, 2018

gbbr Jul 24, 2018

gbbr commented Jul 24, 2018 •

edited

Loading

gbbr Jul 24, 2018

dd-caleb Jul 24, 2018

gbbr Jul 24, 2018

dd-caleb commented Jul 24, 2018

gbbr left a comment

gbbr Jul 25, 2018

gbbr Jul 25, 2018

gbbr Jul 25, 2018

gbbr left a comment

contrib/google.golang.org/grpc: streaming interceptor #277

contrib/google.golang.org/grpc: streaming interceptor #277

Conversation

dd-caleb commented Jul 23, 2018

mbyio commented Jul 23, 2018

dd-caleb commented Jul 23, 2018

mbyio commented Jul 23, 2018

gbbr left a comment

Choose a reason for hiding this comment

gbbr Jul 24, 2018 • edited Loading

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

gbbr commented Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dd-caleb commented Jul 24, 2018

gbbr left a 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

gbbr left a comment

Choose a reason for hiding this comment

gbbr Jul 24, 2018 •

edited

Loading

gbbr commented Jul 24, 2018 •

edited

Loading