reflection: improve server implementation #5197

jhump · 2022-02-16T18:14:36Z

This updates the server reflection implementation in two key ways:

Support alternate source of descriptors, like for RPC servers that get their descriptors dynamically and are dynamic proxies.
- Instead of always getting descriptors from the set of global registered files (when generated code is linked into the calling program), this allows caller to supply custom resolvers.
- This also allows caller to supply a set of service names instead of supplying a *grpc.Server.
Use the new protobuf API v2 stuff to get the descriptors, which is much more sane than the old APIs.
- This allows much of the code to be eliminated since standard APIs now support symbol and descriptor lookup.
- The potential downside of this simplification is that encoded descriptors (pre-serialized to bytes) are no longer retained, so descriptor protos are re-created and marshaled with each call. Given the expected frequency of reflection requests, this is likely a non-issue and not worth adding back some sort of cache.

RELEASE NOTES:

A new NewServer(ServerOptions) function has been added to the google.golang.org/grpc/reflection package that allows creating the reflection server with customizations to the list of advertised services and to the mechanism used to lookup descriptors and extensions.

jhump · 2022-02-16T18:23:44Z

With improvements like this, I could also incorporate my custom wrapper around the existing proto registries, like to augment the descriptors with source code info:
https://github.com/jhump/protoreflect/blob/master/desc/sourceinfo/registry.go#L37
Without a change like this, I have to fork the entire implementation of the reflection service.

But, as mentioned in the description, another use case would be a dynamic server/proxy, whose service descriptors might be retrieved dynamically.

jhump · 2022-02-16T18:30:51Z

@dfawley, @menghanl, is this (or something not far from this) something you'd consider merging?

jhump · 2022-02-16T19:52:11Z

reflection/serverreflection_test.go

@@ -348,7 +356,7 @@ func testFileContainingSymbol(t *testing.T, stream rpb.ServerReflection_ServerRe
 		{"grpc.testingv3.SearchResponseV3.Result.Value.val", fdTestv3Byte},
 		{"grpc.testingv3.SearchResponseV3.Result.Value.str", fdTestv3Byte},
 		{"grpc.testingv3.SearchResponseV3.State", fdTestv3Byte},
-		{"grpc.testingv3.SearchResponseV3.State.FRESH", fdTestv3Byte},
+		{"grpc.testingv3.SearchResponseV3.FRESH", fdTestv3Byte},


This was a bug in the previous formulation. In protobuf, enum values are defined at the same scope as their enclosing enum.

jhump · 2022-02-16T19:54:43Z

reflection/serverreflection.go

-		s.processEnum(fd, prefix, en)
+// NewServer returns a reflection server implementation using the given options.
+// It returns an error if the given options are invalid.
+func NewServer(opts ServerOptions) (rpb.ServerReflectionServer, error) {


I'm curious what your thoughts on this style of API, vs. Register where the server object is never exposed.

One advantage here is that it allows the caller to wrap/decorate the service. (Other formulation requires caller to use an interceptor to do so.)

Also, the options include an optional GRPCServer. But we'd have to have a server to do the actual Register... call. So it feels like a confusing API to have a required server argument and then also an optional field in the options struct.

Or, we define an interface with func GetServiceInfo() (and you won't need the other field ServiceNames []string).

Did we talk about this interface option before? I don't remember if we got any conclusion.

There is already an interface named GRPCServer. The list of ServiceNames is useful if the caller wants more control over exactly which services are advertised by the reflection service.

In a dynamic proxy case, even the list of service names could change over time. So providing the separate names and having this factory for a server (vs. the Register style function, where the server impl is never exposed) allows you to handle this:

The caller would create a wrapper for the service impl. The wrapper delegates to the service impl. But it can also swap out the underlying impl in response to an event that changes the set of services. (Like watching services in kubernetes or something like that).

The actual gRPC server handling is all implemented via an UnknownServiceHandler, that can dynamically proxy requests instead of returning "Not Implemented" errors.

Oh, right. I forgot we already made the GRPCServer interface.

What I meant is a smaller interface, with just GetServiceInfo() map[string]grpc.ServiceInfo.
You can still pass it the grpc server (that's the default behavior), or the dynamic factory.
Or if you want a static []string, you can pass it an implementation that returns the list (and you won't need the other field in ServerOptions).

Okay, I think I see what you mean.

Would you object to me adding a utility function, like ServiceInfoFromNames, that takes a slice of names ([]string) and returns this new interface, that provides the service info?

Done. (I went ahead and added the utility function I asked about. Let me know what you think.)

FYI, I realized that the whole sync.Once initialization was not necessary, since this is no longer trying to accumulate an index. So I've removed it, which I think improves things a bit.

1. Support alternate source of descriptors, like for RPC servers that get their descriptors dynamically and are dynamic proxies 2. Use the new protobuf API v2 stuff to get the descriptors, which is much more sane than the old APIs

menghanl · 2022-02-17T18:18:59Z

reflection/serverreflection.go

+	// Optional resolver used to load descriptors.
+	DescriptorResolver protodesc.Resolver
+	// Optional resolver used to query for known extensions.
+	ExtensionResolver ExtensionResolver


Let's document the default value for these. If unset, protoregistry.GlobalFiles will be used

Done. These were documented above. So I rearranged a little and moved the relevant comments down to the fields' Go doc.

menghanl · 2022-02-17T18:24:38Z

reflection/serverreflection.go

+// NewServer returns a reflection server implementation using the given options.
+// It returns an error if the given options are invalid.
+func NewServer(opts ServerOptions) (rpb.ServerReflectionServer, error) {
+	if opts.Server != nil && len(opts.ServiceNames) > 0 {


(Related to the other comment, if we merge these two fields)
We can remove this error. Also clean up the code in Register.

jhump · 2022-02-17T21:32:01Z

reflection/serverreflection.go

+// ServiceInfoFromNames returns a ServiceInfoProvider backed by the given
+// slice of service names. This allows for creating a reflection server
+// with exactly the given advertised services.
+func ServiceInfoFromNames(serviceNames []string) ServiceInfoProvider {


Let me know if you object to this helper. Though I think it's useful since it might not otherwise be obvious how to customize the set of advertised services.

I personally think this isn't necessary.

If your concern is that the interface is confusing (since only the map keys are used), we can change the interface to return []string, and wrap *grpc.Server to implement the interface.

@dfawley WDYT

Okay. I went ahead and removed it and added some extra documentation to the Services field in ServerOptions.

…eak Go docs

menghanl

Overall LGTM. Thanks for the change!

menghanl · 2022-02-17T21:59:24Z

reflection/serverreflection.go

-	for _, msg := range fd.MessageType {
-		s.processMessage(fd, prefix, msg)
+// NewServer returns a reflection server implementation using the given options.
+// It returns an error if the given options are invalid.


Let's document that Register() is still recommended unless the user wants to customize the server behaviors.

And to be a bit more cautious, mark the new functions/types as experimental?

menghanl · 2022-02-17T22:06:28Z

reflection/serverreflection.go

+// ServiceInfoFromNames returns a ServiceInfoProvider backed by the given
+// slice of service names. This allows for creating a reflection server
+// with exactly the given advertised services.
+func ServiceInfoFromNames(serviceNames []string) ServiceInfoProvider {


I personally think this isn't necessary.

If your concern is that the interface is confusing (since only the map keys are used), we can change the interface to return []string, and wrap *grpc.Server to implement the interface.

@dfawley WDYT

menghanl · 2022-02-17T22:11:06Z

reflection/serverreflection.go

-func fileDescWithDependencies(fd *dpb.FileDescriptorProto, sentFileDescriptors map[string]bool) ([][]byte, error) {
-	r := [][]byte{}
-	queue := []*dpb.FileDescriptorProto{fd}
+func (s *serverReflectionServer) fileDescWithDependencies(fd protoreflect.FileDescriptor, sentFileDescriptors map[string]struct{}) ([][]byte, error) {


On map[string]bool vs map[string]struct{}.

I'm not sure which is more memory efficient, but I think the difference is small enough that we wouldn't care

In gRPC-Go, we've mostly (but not as an official policy) adopted map[string]bool as "set"s. Since if the entry doesn't exist, the map read returns the default bool (which is false). And we don't need to write struct{}{} when adding entries.

Let me know what you think.

Ok. I can walk that part back.

My habit is map[string]struct{} from my early experiences with Go... Admittedly, since it only elides a single byte from the map entry, thanks to pointer word alignment, it may have zero impact on memory usage 🤷

…ove ServiceInfoFromNames

…ckage; but sourceinfo.GlobalFiles needs to implement add'l methods to serve extension info

jhump · 2022-02-18T17:32:52Z

ping @dfawley

dfawley

LGTM! Just a couple small things, though.

dfawley · 2022-02-18T20:06:17Z

reflection/serverreflection.go

+//
+// The reflection service is only interested in the service names, but the
+// signature is this way so that *grpc.Server implements it.
+type ServiceInfoProvider interface {


Please add an Experimental comment here, too, and on all of the newly added exported symbols.

I'd like to give this some time for users to starting using this and provide feedback before declaring anything stable.

Done. Added here and to the ExtensionResolver interface, too.

dfawley · 2022-02-18T20:09:38Z

reflection/serverreflection.go

+	// It is okay for a custom implementation to return zero values for the
+	// grpc.ServiceInfo values in the map.


Should we move this comment to ServiceInfoProvider?

There are similar remarks there already:
https://github.com/grpc/grpc-go/pull/5197/files/a97ada894ee90340c4b53e0d0db7a7ec108e4283#diff-8aead0172d481e4bc1112c2b69e1e60cd91a32e638ff30f9905623ce41c47215R56-R57

I'll remove this and put more clarification over in the other Go docs.

…ted symbols as experimental

… (the newly added stuff), then the unexported server impl

jhump · 2022-02-18T20:27:30Z

I re-ordered some elements in the file in my last commit, hoping to make the file layout a little more intuitive. All of the new experimental stuff now appears after the preferred non-experimental API elements.

I think I've addressed all comments. PTAL and let me know.

And thanks for the consideration/review!

dfawley

I like the code reorganization, thank you for the extra care.

jhump · 2022-02-18T21:34:55Z

Thank you, @dfawley and @menghanl!

…ckage; but sourceinfo.GlobalFiles needs to implement add'l methods to serve extension info

Thanks to grpc/grpc-go#5197, no longer necessary But sourceinfo.GlobalFiles does need to implement add'l methods to serve extension info

jhump force-pushed the jh/use-new-protoreflect-and-support-alternate-src-of-descriptors branch from af3c5b2 to 4313fb4 Compare February 16, 2022 18:30

zasweq assigned menghanl Feb 16, 2022

menghanl self-requested a review February 16, 2022 19:19

jhump force-pushed the jh/use-new-protoreflect-and-support-alternate-src-of-descriptors branch 2 times, most recently from de01755 to 94c0e5d Compare February 16, 2022 19:47

jhump commented Feb 16, 2022

View reviewed changes

jhump force-pushed the jh/use-new-protoreflect-and-support-alternate-src-of-descriptors branch from 94c0e5d to 7c1ba55 Compare February 16, 2022 19:57

improve server reflection

743e65d

1. Support alternate source of descriptors, like for RPC servers that get their descriptors dynamically and are dynamic proxies 2. Use the new protobuf API v2 stuff to get the descriptors, which is much more sane than the old APIs

jhump force-pushed the jh/use-new-protoreflect-and-support-alternate-src-of-descriptors branch from 7c1ba55 to 743e65d Compare February 16, 2022 20:50

menghanl reviewed Feb 17, 2022

View reviewed changes

jhump commented Feb 17, 2022

View reviewed changes

review feedback; use single interface for source of service names; tw…

5999441

…eak Go docs

jhump force-pushed the jh/use-new-protoreflect-and-support-alternate-src-of-descriptors branch from 011b877 to 5999441 Compare February 17, 2022 21:33

menghanl reviewed Feb 17, 2022

View reviewed changes

menghanl added the Type: Feature New features or improvements in behavior label Feb 17, 2022

menghanl added this to the 1.45 Release milestone Feb 17, 2022

menghanl requested a review from dfawley February 17, 2022 22:16

review feedback: mark new stuff as experimental; prefer Register; rem…

0588e9b

…ove ServiceInfoFromNames

menghanl assigned dfawley and unassigned menghanl Feb 17, 2022

menghanl approved these changes Feb 17, 2022

View reviewed changes

jhump added a commit to jhump/protoreflect that referenced this pull request Feb 17, 2022

due to pending PR grpc/grpc-go#5197, no need for srcinforeflection pa…

17314e8

…ckage; but sourceinfo.GlobalFiles needs to implement add'l methods to serve extension info

jhump mentioned this pull request Feb 17, 2022

no need for srcinforeflection package jhump/protoreflect#501

Merged

no need for sync.Once

a97ada8

dfawley reviewed Feb 18, 2022

View reviewed changes

dfawley assigned jhump and unassigned dfawley Feb 18, 2022

jhump added 2 commits February 18, 2022 15:24

review comments: more tweaks to comments; label all other added expor…

281c819

…ted symbols as experimental

reorder elements: preferred exported symbols, then experimental stuff…

505fb99

… (the newly added stuff), then the unexported server impl

dfawley approved these changes Feb 18, 2022

View reviewed changes

dfawley changed the title ~~improve server reflection implementation~~ reflection: improve server implementation Feb 18, 2022

dfawley merged commit 18564ff into grpc:master Feb 18, 2022

jhump added a commit to jhump/protoreflect that referenced this pull request Feb 19, 2022

due to pending PR grpc/grpc-go#5197, no need for srcinforeflection pa…

3112d97

…ckage; but sourceinfo.GlobalFiles needs to implement add'l methods to serve extension info

jhump added a commit to jhump/protoreflect that referenced this pull request Feb 19, 2022

no need for srcinforeflection package (#501)

a2874ac

Thanks to grpc/grpc-go#5197, no longer necessary But sourceinfo.GlobalFiles does need to implement add'l methods to serve extension info

github-actions bot locked as resolved and limited conversation to collaborators Aug 18, 2022

		// It is okay for a custom implementation to return zero values for the
		// grpc.ServiceInfo values in the map.

reflection: improve server implementation #5197

reflection: improve server implementation #5197

Conversation

jhump commented Feb 16, 2022 • edited Loading

jhump commented Feb 16, 2022

jhump commented Feb 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhump Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

menghanl Feb 17, 2022 • 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

jhump Feb 17, 2022 • 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

menghanl left a comment • 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

jhump commented Feb 18, 2022

dfawley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhump Feb 18, 2022 • 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

jhump commented Feb 18, 2022

dfawley left a comment

Choose a reason for hiding this comment

jhump commented Feb 18, 2022 • edited Loading

jhump commented Feb 16, 2022 •

edited

Loading

jhump Feb 17, 2022 •

edited

Loading

menghanl Feb 17, 2022 •

edited

Loading

jhump Feb 17, 2022 •

edited

Loading

menghanl left a comment •

edited

Loading

jhump Feb 18, 2022 •

edited

Loading

jhump commented Feb 18, 2022 •

edited

Loading