Support instrumenting unary grpc #292

tony84727 · 2021-04-01T15:22:21Z

Issue #, if available:
#110

Description of changes:
Continue @sawadashota's effort (#264) to implement grpc support.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

original reviews: aws#264 (comment) aws#264 (comment) aws#264 (comment)

original review: aws#264 (comment)

bhautikpip · 2021-04-26T16:01:13Z

README.md

+
+```go
+grpcServer := grpc.NewServer(
+    grpcmiddleware.WithUnaryServerChain(


We should probably include below imports here to give clear idea about grpcmiddleware package

grpc_middleware "github.com/grpc-ecosystem/go-grpc-middleware"

grpc had added similar chaining methods so we don't need to use grpc_middleware now.

To apply the interceptors, we can use grpc.UnaryInterceptor or grpc.WithUnaryInterceptor directly. So I simplify the examples a bit and add comments to show how to apply multiple interceptors.

bhautikpip · 2021-04-26T16:01:49Z

xray/grpc.go

+import (
+	"context"
+	"errors"
+	"github.com/golang/protobuf/proto"


Can we format this file to avoid review dog warnings?

Ran the go fmt for the file, those warning should be fixed.

bhautikpip · 2021-04-26T16:20:30Z

README.md

+**grpc client**
+
+```go
+conn, err := grpc.Dial(


Can we add a little description here on how below grpc client and server instrumentation works ?

Added a short description to point out the instrumentation is done by applying xray.UnaryServerInterceptor or xray.UnaryClientInterceptor.

format xray/grpc.go

add a description of unary interceptors usage. also, replace `grpc` by `gRPC` in the README.md to align with the name shows on gRPC official website. update examples to use grpc chain methods (remove the usage of `grpc_middleware`)

tony84727 · 2021-04-27T05:25:56Z

xray/grpc.go

+}
+
+// UnaryServerInterceptor provides gRPC unary server interceptor.
+func UnaryServerInterceptor(ctx context.Context, sn SegmentNamer) grpc.UnaryServerInterceptor {


AFAIK, the ctx here is only used to pass in the value of RecorderContextKey, which should be a Config.

Normally, it's a nil so the recorder uses the global config.

I can see that for HTTP instrumentation, there's two types of wrapper:

xray.Handler

xray.HandlerWithContext

Maybe we should break the interceptor into two signatures as well? One without context and another one with context.

I think that's a fair call out. We can have two types of wrapper here as well with ctx and without ctx

added UnaryServerInterceptorWithContext.

Although grpc.WithInsecure is commonly used for development, it isn't necessary for using the xray interceptor.

refactor the original UnaryServerInterceptor into two signatures, one with ctx, another without ctx

bhautikpip · 2021-04-28T15:21:05Z

xray/grpc.go

+		seg.Unlock()
+
+		resp, err = handler(ctx, req)
+		recordContentLength(seg, resp)


I think we also probably should include setting up response header and set _x_amzn_trace_id value. Reference can be taken from here - https://github.com/tony84727/aws-xray-sdk-go/blob/support_grpc/xray/handler.go#L133. If we are not generating response header and add it as a value to the _x_amzn_trace_id key then downstream services will keep on sampling even if we did sampling on upstream service.

Calling grpc.SendHeader to send the response trace header. grpc.SendHeader might returns an error, I'm using multierr to wrap multiple errors together. Not sure whether it's a good idea. Or should we just ignore the error and fail silently?

anuraaga · 2021-05-13T04:42:14Z

xray/grpc.go

+func UnaryClientInterceptor(host string) grpc.UnaryClientInterceptor {
+	return func(ctx context.Context, method string, req, reply interface{}, cc *grpc.ClientConn, invoker grpc.UnaryInvoker, opts ...grpc.CallOption) error {
+		return Capture(ctx, host, func(ctx context.Context) error {
+			seg := GetSegment(ctx)


This looks like it is getting the server segment and is then mutated below. The unit test seems sane though, could you point out where the subsegment is being created for the client request?

Hello, @anuraaga
I believe the subsegment is created by Capture at line 26, which call BeginSubsegment.

anuraaga · 2021-05-13T04:42:49Z

xray/grpc.go

+			seg.Lock()
+			seg.Namespace = "remote"
+			seg.GetHTTP().GetRequest().URL = "grpc://" + host + method
+			seg.GetHTTP().GetRequest().Method = method


This needs to be an HTTP method, not gRPC method. It's always POST.

anuraaga · 2021-05-13T04:44:16Z

xray/grpc.go

+		ctx, seg = NewSegmentFromHeader(ctx, name, &http.Request{
+			Host:   host,
+			URL:    &requestURL,
+			Method: info.FullMethod,


Ditto this needs to be POST

anuraaga · 2021-05-13T04:44:49Z

xray/grpc.go

+		seg.Lock()
+		seg.GetHTTP().GetRequest().ClientIP, seg.GetHTTP().GetRequest().XForwardedFor = clientIPFromGrpcMetadata(md)
+		seg.GetHTTP().GetRequest().URL = requestURL.String()
+		seg.GetHTTP().GetRequest().Method = info.FullMethod


anuraaga · 2021-05-13T04:46:19Z

xray/grpc.go

+		}
+		recordContentLength(seg, resp)
+		if headerErr := addResponseTraceHeader(ctx, seg, traceHeader); headerErr != nil {
+			err = multierr.Combine(err, headerErr)


Can we avoid adding this new library dependency on multierr?

As described here #292 (comment)
At this line there're two possible error sources:

The error returned by the service handler itself

The error returned by calling grpc.SendHeader

I'm not sure how to report multiple errors within an interceptor. Maybe change this to print a line of error message when failling to send headers? What do you think?

I think we can drop the error, it seems pretty rare. Alternatively a debug log message could be ok too. But yeah we shouldn't add it to the err here. It's not quite as important if err is actually set but imagine if its nil - then it means our interceptor changes the status of the actual gRPC request to an error which will probably cause problems in user code. The interceptor should only record issues but not introduce errors to the business logic like that.

Okay, using the internal logger to print the debug message.

anuraaga · 2021-05-13T04:46:53Z

README.md

+grpcServer := grpc.NewServer(
+    // use grpc.ChainUnaryInterceptor instead to apply multiple interceptors
+    grpc.UnaryInterceptor(
+        xray.UnaryServerInterceptor(context.TODO(), xray.NewFixedSegmentNamer("myApp")),


Suggested change

xray.UnaryServerInterceptor(context.TODO(), xray.NewFixedSegmentNamer("myApp")),

xray.UnaryServerInterceptor(xray.NewFixedSegmentNamer("myApp")),

Updated. Thanks.

anuraaga · 2021-05-13T04:48:07Z

xray/grpc.go

+}
+
+// UnaryServerInterceptor provides gRPC unary server interceptor.
+func UnaryServerInterceptor(sn SegmentNamer) grpc.UnaryServerInterceptor {


It would be nice to have a default interceptor that uses the gRPC service name as the segment name

Agree. So I guess there will be four variants of the server interceptor?

UnaryServerInterceptor()

UnaryServerInterceptorWithContext(ctx context.Context)

UnaryServerInterceptorWithNamer(sn SegmentNamer)

UnaryServerInterceptorWithContextAndNamer(ctx context.Context, sn SegmentNamer)

Yeah - can you use the options pattern to have some symmetry with upstream gRPC and avoid the need for the explosion of factory methods?

https://github.com/grpc/grpc-go/blob/master/server.go#L118

anuraaga · 2021-05-13T04:52:19Z

xray/grpc.go

+	switch grpcStatus.Code() {
+	case codes.ResourceExhausted:
+		seg.Throttle = true
+	case codes.Internal, codes.Unimplemented, codes.DataLoss:


Error is like HTTP 4xx. This list is a good one to refer to for mapping

https://github.com/open-telemetry/opentelemetry-collector/pull/3110/files#diff-3c93ea7fd05dbbc4514087f190d5c07c0d125e606559d8cdd96923081058b63dL38

https://github.com/open-telemetry/opentelemetry-collector/pull/3110/files#diff-3c93ea7fd05dbbc4514087f190d5c07c0d125e606559d8cdd96923081058b63dL38

The mapping doesn't classify all the possible grpc status codes here:
https://github.com/grpc/grpc-go/blob/dc77d7ffe311f78f2e577572d984af3c0a8df82b/codes/codes.go

So, I'm referencing the mapping here:
https://github.com/googleapis/googleapis/blob/b29fb6b9fcdf3bc67487fa22ab7783915023041e/google/rpc/code.proto#L35

mapping is referenced from https://github.com/googleapis/googleapis/blob/master/google/rpc/code.proto

This reverts commit 5dbb380.

in addition, refactor UnaryServerInterceptor, apply options pattern to prevent multiple interceptor factory methods.

grpc-go seems to not using http.Client. So hooks of ClientTrace aren't invoked at all.

correct the example of UnaryServerInterceptor.

remove dep: multierr

correct the server interceptor example

anuraaga · 2021-05-14T04:10:50Z

README.md

+    // use grpc.ChainUnaryInterceptor instead to apply multiple interceptors
+    grpc.UnaryInterceptor(
+        xray.UnaryServerInterceptor(),
+        // or xray.UnaryServerInterceptor(xray.ServerInterceptorWithSegmentNamer(xray.NewFixedSegmentNamer("myApp"))) to use a custom segment namer


This looks a bit wrong now

Do you mean there's a typo or I shouldn't use comments to mention the usage of options?

I think typo, it's xray.UnaryServerINterceptor(xray.NewFixedSegmentNamer(...)) right?

By the options pattern and the current implementation, adding a segment namer to the unary server interceptor should look like this (at line 265):

aws-xray-sdk-go/xray/grpc_test.go

Lines 262 to 265 in 63d8684

grpc.UnaryInterceptor(

UnaryServerInterceptor(

ServerInterceptorWithContext(ctx),

ServerInterceptorWithSegmentNamer(NewFixedSegmentNamer("test")))),

Ah I don't think we need to be redundant with e.g. ServerInterceptor. Just UnaryServerInterceptor(WithContext(ctx), WithSegmentNamer(...)) can be done I think, there's no difference between server / client versions of these options right? If we can simplify these names I think we're good to go

anuraaga · 2021-05-14T04:11:13Z

xray/grpc.go

+)
+
+// UnaryClientInterceptor provides gRPC unary client interceptor.
+func UnaryClientInterceptor(host string) grpc.UnaryClientInterceptor {


One last point, how about we follow option pattern for client too and make host an option. By default, can use the service name here too

Applied the options pattern to UnaryClientInterceptor. The original host here is used as the segment name and host of the recorded request URL. So I added two options:

Segment Namer

Host

Although I think it's a rare case to override the host?

Ah - it looks like you found host in the interceptor parameters, cc.Target. If that's available than I agree there isn't much use case to override host. Having the segment namer option looks good, how about we just go ahead and remove the host option for now and add it if someone asks for it?

👍 Removed the host option.

anuraaga · 2021-05-14T04:12:42Z

xray/grpc_test.go

+		assert.NoError(t, err)
+		segment, err := td.Recv()
+		assert.NoError(t, err)
+		assert.Equal(t, "bufnet/mwitkow.testproto.TestService/Ping", segment.Name)


Oh sorry also this should only be the service name by default, definitely not method and we don't need host in the default (a gRPC service name is semantically similar to a segment name, like a host would be on HTTP). So it should just be mwitkow.testproto.TestService

Okay. Fixed

instead of gRPC full method name

also, add some comments and allow setting a custom segment namer for ClientInterceptor

tony84727 · 2021-05-15T07:02:17Z

xray/grpc.go

+func ServerInterceptorWithSegmentNamer(sn SegmentNamer) ServerInterceptorOption {
+	return newFuncServerInterceptorOption(func(option *serverInterceptorOption) {
+		option.segmentNamer = sn
+	})
+}


Maybe we should rename this into UnaryServerInterceptorWithSegmentNamer? Or organize interceptors and options into sub-packages?

We might need to find a proper way to organize those options because interceptors of gRPC have four variants:

Unary / Client

Unary / Server

In the future, we might implement interceptors for gRPC streaming. Then we will have:
3. Stream / Client
4. Stream / Server

I can imagine, different types of interceptors will have different types of concerns and options.

anuraaga

Sorry for the extreme delay in getting back, just a comment about the option names, I think we're pretty close.

anuraaga · 2021-06-01T08:11:39Z

README.md

+    // use grpc.ChainUnaryInterceptor instead to apply multiple interceptors
+    grpc.UnaryInterceptor(
+        xray.UnaryServerInterceptor(),
+        // or xray.UnaryServerInterceptor(xray.ServerInterceptorWithSegmentNamer(xray.NewFixedSegmentNamer("myApp"))) to use a custom segment namer


Ah I don't think we need to be redundant with e.g. ServerInterceptor. Just UnaryServerInterceptor(WithContext(ctx), WithSegmentNamer(...)) can be done I think, there's no difference between server / client versions of these options right? If we can simplify these names I think we're good to go

anuraaga · 2021-06-01T08:15:24Z

Also it could be great reference to look through the OpenTelemetry instrumentation for good patterns re: naming, etc.

https://github.com/open-telemetry/opentelemetry-go-contrib/blob/main/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go

They don't seem to have separate packages for unarystream / clientserver so I'm thinking it's relatively scalable.

tony84727 · 2021-06-01T14:30:08Z

Ah I don't think we need to be redundant with e.g. ServerInterceptor. Just UnaryServerInterceptor(WithContext(ctx), WithSegmentNamer(...)) can be done I think, there's no difference between server / client versions of these options right? If we can simplify these names I think we're good to go

Merged Server/ClientInterceptorOption -> GrpcOption.

I also add a refactor that changes the API a little bit:
Changed the option WithContext -> withRecorder because the option was used to provide a custom xray.Config to the server handling context. We only propagate the xray.Config value of the given context. We're not propagating other values associated with the context. This should make the option's intent more clear. (Also, it seems to be a code smell to store a context inside a struct)

anuraaga

Thanks for the patience! @bhautikpip or @srprash can you also give it a look over and we'll merge?

bhautikpip

LGTM :)

sawadashota and others added 8 commits January 16, 2021 22:56

Support gRPC unary RPCs

79073d4

fix: Typo

7feb344

fix: Dialing at GRPC test

374235b

Drop Go v1.9 support

7827cbb

Update dependencies

cc8f037

Populate grpc request method

d7312ab

set flags based on grpc status and record response content-length

1921eed

original reviews: aws#264 (comment) aws#264 (comment) aws#264 (comment)

populate http.Request for sampling decision

13ec0dc

original review: aws#264 (comment)

tony84727 force-pushed the support_grpc branch from 908f831 to 13ec0dc Compare April 1, 2021 16:40

tony84727 added 5 commits April 2, 2021 00:47

Document current implementation doesn't support grpc streaming

399865a

original review: aws#264 (comment)

refactor function to put context as the first argument

77c8bbd

refactor client unary interceptor test

502e30b

refactor server unary interceptor test

9fc7292

run go mod tidy

ac689e1

tony84727 changed the title ~~Support grpc~~ Support instrumenting unary grpc Apr 2, 2021

tony84727 marked this pull request as ready for review April 5, 2021 09:48

bhautikpip self-requested a review April 6, 2021 00:36

bhautikpip reviewed Apr 26, 2021

View reviewed changes

bhautikpip mentioned this pull request Apr 27, 2021

Support gRPC unary RPCs #264

Closed

tony84727 added 2 commits April 27, 2021 13:01

fix review dog issues

a5732bb

format xray/grpc.go

update README.md gRPC section

9555c2c

add a description of unary interceptors usage. also, replace `grpc` by `gRPC` in the README.md to align with the name shows on gRPC official website. update examples to use grpc chain methods (remove the usage of `grpc_middleware`)

tony84727 commented Apr 27, 2021

View reviewed changes

tony84727 added 2 commits April 27, 2021 13:51

remove usage of grpc.WithInsecure from the example

08f9fbc

Although grpc.WithInsecure is commonly used for development, it isn't necessary for using the xray interceptor.

add UnaryServerInterceptorWithContext

5dbb380

refactor the original UnaryServerInterceptor into two signatures, one with ctx, another without ctx

bhautikpip reviewed Apr 28, 2021

View reviewed changes

tony84727 added 5 commits April 29, 2021 22:38

make UnaryServerInterceptor send trace header

1079528

go fmt xray/grpc_test.go

89aa6ce

run go mod tidy

b4f607b

run goimports on xray/grpc.go xray/grpc_test.go

610f581

Merge branch 'master' into support_grpc

f466bbd

willarmiros requested a review from anuraaga May 11, 2021 00:19

anuraaga reviewed May 13, 2021

View reviewed changes

tony84727 added 8 commits May 13, 2021 15:03

correct recorded http method

8cb9191

correct grpc status code mapping

f295e46

mapping is referenced from https://github.com/googleapis/googleapis/blob/master/google/rpc/code.proto

Revert "add UnaryServerInterceptorWithContext"

f0b9c5d

This reverts commit 5dbb380.

make default interceptor name the segment by gRPC service

193d34d

in addition, refactor UnaryServerInterceptor, apply options pattern to prevent multiple interceptor factory methods.

remove http client race from UnaryClientInterceptor

3a57753

grpc-go seems to not using http.Client. So hooks of ClientTrace aren't invoked at all.

Update README.md

6bbf8b8

correct the example of UnaryServerInterceptor.

print a debug message when fail to send the grpc trace header

79431a2

remove dep: multierr

Update README.md

7a4b483

correct the server interceptor example

anuraaga reviewed May 14, 2021

View reviewed changes

tony84727 added 4 commits May 14, 2021 12:48

use gRPC service name as the default segment name

108308b

instead of gRPC full method name

refactor ClientInterceptor by options pattern

6985a10

also, add some comments and allow setting a custom segment namer for ClientInterceptor

refactor test to use grpc for interceptors

63d8684

remove client interceptor option: host

4b80070

tony84727 commented May 15, 2021

View reviewed changes

tony84727 requested a review from anuraaga May 27, 2021 02:43

anuraaga reviewed Jun 1, 2021

View reviewed changes

tony84727 added 3 commits June 1, 2021 21:47

remove redundant Server/ClientInterceptor name prefix

b5dbf4b

refactor: allow passing in custom xray.Config directly

bb8f052

propagate custom xray.Config to the incoming client context

efa054b

rename the function

9530e62

anuraaga approved these changes Jun 2, 2021

View reviewed changes

bhautikpip approved these changes Jun 4, 2021

View reviewed changes

bhautikpip merged commit b2f8cb6 into aws:master Jun 7, 2021

danielwhite mentioned this pull request Jun 15, 2021

Large external dependencies in xray package introduced by v1.5.0 #310

Closed

	xray.UnaryServerInterceptor(context.TODO(), xray.NewFixedSegmentNamer("myApp")),
	xray.UnaryServerInterceptor(xray.NewFixedSegmentNamer("myApp")),

	grpc.UnaryInterceptor(
	UnaryServerInterceptor(
	ServerInterceptorWithContext(ctx),
	ServerInterceptorWithSegmentNamer(NewFixedSegmentNamer("test")))),

Support instrumenting unary grpc #292

Support instrumenting unary grpc #292

Conversation

tony84727 commented Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

tony84727 Apr 27, 2021 • 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

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

tony84727 May 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tony84727 May 14, 2021 • 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

anuraaga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga commented Jun 1, 2021

tony84727 commented Jun 1, 2021

anuraaga left a comment

Choose a reason for hiding this comment

bhautikpip left a comment

Choose a reason for hiding this comment

tony84727 commented Apr 1, 2021 •

edited

Loading

tony84727 Apr 27, 2021 •

edited

Loading

tony84727 May 15, 2021 •

edited

Loading

tony84727 May 14, 2021 •

edited

Loading