-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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: Add a pointer of server to ctx passed into stats handler #6750
Conversation
internal/internal.go
Outdated
@@ -73,6 +73,11 @@ var ( | |||
// xDS-enabled server invokes this method on a grpc.Server when a particular | |||
// listener moves to "not-serving" mode. | |||
DrainServerTransports any // func(*grpc.Server, string) | |||
// IsRegisteredMethod returns whether the passed in method is registered as | |||
// a method on the server. | |||
IsRegisteredMethod any // func(*grpc.Server, string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This returns a bool
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right, whoops. Added.
// Guarantees we satisfy this interface; panics if unimplemented methods are | ||
// called. | ||
testgrpc.TestServiceServer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you bundling a stats handler with a service implementation? That seems undesirable unless it's somehow important to keep the two things together. (And now that I've look at the rest of the code, it seems unused and unnecessary.)
Also, we should put this under testutils
(either in that package or a subdirectory of it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, whoops, this is leftover from the stubserver copy paste. Changed to embedding a stats.Handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved file to within testutils as internal/testutils/stubstatshandler.go.
// isRegisteredMethod returns whether the passed in method is registered as a | ||
// method on the server. /service/method and service/method will match if the | ||
// service and method are registered on the server. | ||
func (s *Server) isRegisteredMethod(serviceMethod string) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this very similar to the code that looks up the handler for a method? Can we reuse code somehow, or even just call it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is very similar, but I chose not to do it because this line is very specific: https://github.com/grpc/grpc-go/blob/master/server.go#L1731, and I thought it would be hard to generalize what's shared into a helper. If you feel strongly about this after reading the handleStream() code, I can try and pull out logic into helper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah please see what you can do. I think a method that returns (service string, method string, ok bool)
would be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried writing it, and it seems distinct enough logically to not warrant a helper. Your function signature doesn't distinguish between a. malformed method name (important here for tracing and logging: https://github.com/grpc/grpc-go/blob/master/server.go#L1732) b. registered unary rpc c. registered streaming name (important here: https://github.com/grpc/grpc-go/blob/master/server.go#L1770, to chose to go through processUnary or processStreaming). This helper's scope is not to log anything in traces or channelz, but to parse a :method header received from the wire and determine whether it's registered or not registered. I think it's sufficiently different to not warrant a new helper.
stats/stats_test.go
Outdated
UnaryCallF: func(ctx context.Context, in *testpb.SimpleRequest) (*testpb.SimpleResponse, error) { | ||
return &testpb.SimpleResponse{}, nil | ||
}, | ||
FullDuplexCallF: func(stream testpb.TestService_FullDuplexCallServer) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You never call this; either delete it or test it please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do test this. See line 1493. However, you are right that I never initiate a bidirectional streaming RPC from the test. Would you like me to add that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This closure being set or not doesn't change the test at line 1494. The method is registered because it's in the service descriptor. I'm fine with deleting this; I don't think there's any need to test both streaming and unary RPCs unless there are different code paths that lead to setting the server in the context, which there aren't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right, these closures != service descriptor, it's all there anyway. Deleting.
stats/stats_test.go
Outdated
isRegisteredMethod := internal.IsRegisteredMethod.(func(*grpc.Server, string) bool) | ||
// /s/m and s/m are valid. | ||
if !isRegisteredMethod(server, "/grpc.testing.TestService/UnaryCall") { | ||
errorCh.Send(errors.New("UnaryCall should be a registered method according to server")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't actually need the error channel for this. You can just do t.Errorf
here. Then all you need to do is ensure the code actually was called. You can do this by closing a done
, channel or with a waitgroup, etc. One benefit of this is that you'll see all the test case failures in a single run if multiple of these checks don't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right, this was when it was a defined stats handler rather than a stub with methods declared in test. Thus, before I couldn't close on t *testing.T, and now I can :). Switched.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chose a waitGroup.
// Guarantees we satisfy this interface; panics if unimplemented methods are | ||
// called. | ||
testgrpc.TestServiceServer | ||
stats.Handler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're fully implementing it (which we must be given the earlier snapshot) then this is unnecessary; just delete it. If you want to have a paranoid check that we're implementing it, you can add a var _ stats.Handler = (*StubStatsHandler)(nil)
. But that's not strictly necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok deleted.
This PR adds a pointer to the grpc.Server into the context passed into the stats handler. It also adds an internal only helper on the server that returns whether a given method is registered or not. This will be used in OpenTelemetry instrumentation for security purposes with respect to the cardinality of the method attribute. The next step after these two PRs is to move the rest of the stats handler callouts to the gRPC layer, get registered methods plumbed client side, canoncalize Target() without breaking it and then the OpenTelemetry instrumentation itself :).
RELEASE NOTES: N/A