Skip to content
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

proposal: add introspection support for service/method descriptors #489

Closed
misberner opened this issue Jan 20, 2018 · 24 comments
Closed

proposal: add introspection support for service/method descriptors #489

misberner opened this issue Jan 20, 2018 · 24 comments
Labels

Comments

@misberner
Copy link

It seems to me that the generated Go code for a proto file with services does not allow a general way to obtain descriptors for services. As a consequence, there is no way of inspecting custom ServiceOptions or MethodOptions extensions at runtime.

The only exported symbols are the Register... method for the service side, and New...Client... methods for the client. Both are not suitable for obtaining a descriptor. While it maybe possible to get to that information via the FileDescriptorProto, there likewise is no general way to obtain a file descriptor from the Go code generated from a proto file - after all, no messages might be declared in that same file.

Am I missing something here? This seems to be a surprising omission.

@paranoiacblack
Copy link
Contributor

I don't think you're missing anything, it's probably more of a lack of needing this use-case. The generated protos simply register their generated descriptor; they shouldn't export it necessarily. The grpc package could provide a method such as func (*Server) GetServiceDesc(name string) *ServiceDesc that takes in a fully-qualified service name and returns the ServiceDesc. It wouldn't require any particular changes to the generated protos for a gRPC service.

About inspecting custom ServiceOptions and MethodOptions, can you elaborate on what you're referring to or interested in inspecting? It's hard to tell if you'd like to inspect a ServiceConfig, which you should do with the WithServiceConfig DialOption (https://godoc.org/google.golang.org/grpc#WithServiceConfig), in order to look at the underlying MethodConfig. Or did you intend to observe the CallOptions used to Invoke a particular method?

@dsnet dsnet added the grpc label Feb 2, 2018
@paranoiacblack
Copy link
Contributor

@misberner Feel free to re-open this discussion if you think adding a method to the grpc package to do this is the wrong approach.

@alecthomas
Copy link

I would personally like the ability to retrieve the file descriptors, and ideally the service descriptors too.

I don't think this would be inconsistent with the current API, as messages (structs) already include a Desciptor() method, and the file descriptor bytes can be indirectly retrieved via proto.FileDescriptor(filename). But it would be much more convenient if the file descriptor were exposed directly in the generated code so that the retrieval of the file descriptor is decoupled from the filename. eg. mypackage.FileDescriptor()

@mightyguava
Copy link

mightyguava commented Feb 19, 2018

@paranoiacblack if I'm understanding correctly, the option of adding a method to the grpc package requires that a service already be registered with a grpc server. I would like to be able to inspect the ServiceDescriptorProto and ServiceDesc without having to register with a server.

The use case here is for doing introspection on the service, so that we can do things like:

  1. Check if a service being passed in is already registered with the server
  2. Dynamically register services with the server as-needed
  3. List the methods a service implements for presenting to a user (or program), from a client perspective, without the protobuf available

In general though, having FileDescriptor and ServiceDescriptor exported is useful for doing introspection on the protobuf without having to resort to reflection. This is something I believe is decently common practice within Google when writing framework-level code around protobufs and rpc services, at least for Java/Python, though I left Google quite a while ago and that may have changed.

I made #516. It's a minimal PR to expose the ServiceDescriptorProto and ServiceDesc, though not quite familiar enough with the generator to be sure it's correct

@paranoiacblack
Copy link
Contributor

Hi @mightyguava, you mention being able to inspect ServiceDescriptorProto and ServiceDesc without having to register with a server. I suppose that is not unreasonable, but it's not clear how this helps with your use cases.

  1. Checking if a service is already registered. It seems that regardless of how easy it is to access the service descriptor, you ultimately have to ask the grpc.Server to tell you if something is already registered. If not, you'd need to keep track of your own registration list. I don't see how this is an improvement over a grpc provided method such as func (s *grpc.Server) IsRegistered(service name) bool. Having the exact value of the service descriptor won't help with multiple registration of the same named service.

  2. I'd make a similar argument. It seems like you would need both assistance from grpc's library and to keep track of your own set of registered services to achieve this. If grpc provided the method I mentioned earlier, you would combine your stored information for dynamic re-registration via grpc.Server.RegisterService(grpc.Server.GetServiceDesc(name)). It isn't clear to me how exporting service descriptors makes this much easier, except that it means gRPC does not have to add this method.

  3. This is a fine use-case, but it's not clear to me how you can do this without the protobuf available. Can you elaborate? It seems like you need to protobuf no matter what and exporting it only gives you a fast path to listing each of the individual methods in a service. If you are referring to listing the methods on a server, again, you need help from gRPC to tell you this information; the protos have no idea with which server they are registered.

I think accessing these fields via reflection is a bad idea, as you stated. #516 seems okay on the face of it, but it would be much better if you could show me code that demonstrates how you're using it to achieve your goal. I still don't think exporting these descriptors gets you very far because it does not answer any particular questions about your service; it just gives you a simpler way to query it.

@paranoiacblack
Copy link
Contributor

@alecthomas I agree that it is not inconsistent with current exposure of the FileDescriptor. Although, I would argue that the proto package is something that can help you inspect and use protos similarly to the grpc package helps you inspect and use servers. Whether or not it is more convenient is unclear to me. Having a single place/import/package that can answer my meta-questions about protobufs seems like a good separation of concerns whereas having each generated proto learn to answer meta-questions about itself seems awkward in my experience with these libraries. Has anyone tried to make an issue with gRPC to hear their opinion on this? I'm willing to make very similar arguments there.

@mightyguava
Copy link

@paranoiacblack For 1 and 2, you are right, in principle, that having an IsRegistered(service name) bool is better for dynamically registering services. However, how does one get the service name? As far as I am aware, the canonical name is only available in 2 places: in ServiceDesc.Name, and in the service descriptor, neither of which is exposed. There aren't even any server structs that one can pass in in place of it for the grpc library to reflect on. Even if there were, how would I print the canonical name of the service? Do I have to ask grpc for it? And if say we were to generate the canonical name of the service and expose it on the protobuf, how do I get the canonical method name for it? Do I open another issue to ask grpc to implement it for me?

I'm sure there can be some way for all of this to be provided by the grpc library, this sounds like a rabbit hole. Should the grpc package predict, implement, and constrain all possible uses of grpc services and wrap all reflection operations? Or should enough information about services be exposed so that the clients of the protobufs and the grpc library can do it themselves, safely?

I think you make a good point that the grpc package is probably the right place for most things around inspecting and use of servers in place. I'd like to add to that argument that the protoc-gen-grpc plugin also belongs in the grpc package, because the stubs appear to be for the exclusive use of the grpc package. It does not belong in this repo. Following along this line of reasoning, that the grpc plugin belongs in the grpc package, I think it makes perfect sense that code generated for and by the grpc package offer the ability to introspect services. It doesn't appear to be technically feasible to separate the plugin from proto though, since the plugin system isn't actually pluggable.

On point 3, I phrased it poorly. By client I meant the client fo the grpc library and the end-user, not the grpc client. I am not referring to listing methods on the server. I am referring to listing the canonical name of the service and methods of the service.

For my use case, I would like to, before registering a service dynamically with a server, know which methods this service will be adding. I don't need information on the server they are being registered against because the server hasn't been created yet and I have full knowledge of the parameters it will have. I can do this by directly inspecting either the ServiceDesc or ServiceDescriptor. The ServiceDesc provides at a higher level the properties of the service and methods. The ServiceDescriptor provides basically full information about the service. This information can be useful for logging and debugging when dynamically registering grpc services to a server. I don't really want to write out additional code here, since my use case is pretty context dependent and wouldn't make much sense outside my project, as would be many other uses of this feature, or grpc in general really.

@paranoiacblack
Copy link
Contributor

paranoiacblack commented Feb 22, 2018

The third place the canonical names are listed, on a given server, is https://godoc.org/google.golang.org/grpc#Server.GetServiceInfo, which also would answer your questions about whether or not that service is registered or not as well as which methods the server claims exist. My understanding of your change is that you would just use the ServiceDesc to get a canonical name to lookup, rather than going through the ServiceInfo mapping to discover it. There's not unreasonable, but I'd like to demonstrate its use in some test code or something before moving forward on that change. If the way you were going to use this is different, please explain.

I don't buy the argument that protoc-gen-grpc belongs in the grpc package, it's just a code generator that relies on grpc's API. It doesn't know anything about grpc, particularly, really, except how to invoke a method using a name it generated. The regular proto compiler could also do this work, but it's wasted effort if there actually aren't any services in your proto.

About information about services being exposed to clients, I thought that was the intention behind https://github.com/grpc/grpc/blob/master/doc/service_config.md, although it seems like you would be unable to use this if you are dynamically registering services and methods.

So to your actual problem, you want to do this all a-priori, before there's any server running I suppose. I'd still like you to provide a unit test that demonstrates how to use this feature, though. If your use-case is context dependent, we can massage it a bit into a reasonable test case. To be clear, I think your request is reasonable and appreciate the clarification. Please move forward with a small example and we can try adding this.

@dsnet dsnet changed the title No way to get service/method descriptors for GRPC services? proposal: add introspection support for service/method descriptors Feb 23, 2018
@dsnet dsnet added the proposal label Feb 23, 2018
@paranoiacblack
Copy link
Contributor

@dfawley Can you share any thoughts on the line between grpc's service introspection and what the grpc proto plugin can provide to the user. I'm on the line about this change because it isn't clear to me how this interacts with services that use interception or service/method renaming. It seems like regardless of what any given generated service descriptor might tell you, grpc is the only package that can tell you the true state of any given server. Am I missing something?

@mightyguava
Copy link

Hey @paranoiacblack, thanks for reopening the issue. What I'm trying to do is definitely context-dependent.

Not really sure what you are looking for in terms of a unit test. I couldn't find any tests for the grpc generator in this repo. Here's a test that demonstrates the use of the ServiceDesc. mightyguava@a5540aa

A practical use of the descriptor proto in code would be very context dependent and doesn't really add any value here. If the goal is correctness and regression testing, golden tests should suffice. If the goal is understanding further why someone would want to introspect a service descriptor before registering with a server, let me know what I can add here other than the comments above.

@dfawley
Copy link
Contributor

dfawley commented Mar 5, 2018

@paranoiacblack,

In an ideal world, the grpc proto plugin should only need to generate the stubs the user needs to use grpc with protobufs in a type-safe and user-friendly way. It should not be relied upon to provide "core" proto functionality like introspection/reflection of proto definitions themselves -- operations that could be useful with all RPC services.

I'm sure there's a good reason for this, but can we not export fileDescriptor<n> from the generated code as a slice? Then a user with access to a package could trivially get the FD protos for it without needing to know its name.

Short of that, I'm not really sure what the best path forward is. The default proto generator currently doesn't generate any symbols for services. The grpc plugin only generates interfaces for services, meaning we can't attach a Descriptor() method to them like message types have. We could theoretically generate FooDescriptor() functions that return ServiceDescriptorProtos for every service, but it wouldn't be possible to programmatically find this function without reflection, so I'm not sure it would have much value.

I also would like to learn more about use cases 1 and 2, and see how these would work in code.

For 3, in case you aren't already aware, gRPC offers a reflection service that can be used to retrieve FileDescriptorProtos for all the registered services in a given server. But as @mightyguava seems to want to avoid, you'd need to first register the service with the server before using this.

@mightyguava
Copy link

We could theoretically generate FooDescriptor() functions that return ServiceDescriptorProtos for every service, but it wouldn't be possible to programmatically find this function without reflection, so I'm not sure it would have much value.

Having FooDescriptor() is actually fine for my purposes. I would really just like any way to reference the ServiceDescriptorProto for a service. It currently can't even be done with reflection. It would be nicer if I could iterate over all Services in a file, but I could move on without it.

I think you make a very key argument here, that exposing the descriptor allows "operations that could be useful with all RPC services."

I also would like to learn more about use cases 1 and 2, and see how these would work in code.

I admit, these are more hypotheticals to make a case for exposing the ServiceDescriptorProtos. I don't personally have a use for this. Care more about case 3.

For 3, in case you aren't already aware, gRPC offers a reflection service that can be used to retrieve FileDescriptorProtos for all the registered services in a given server. But as @mightyguava seems to want to avoid, you'd need to first register the service with the server before using this.

Exactly. I am aware of the reflection service. But it would be super roundabout to register an implementation of a service on a dummy server, then register the reflection service, just to get the descriptor proto. It kind of defeats the purpose of using protobuf at all.

@awalterschulze
Copy link

I am thinking maybe exposing the ServiceDesc would actually help the reflection service to be injectable, rather than relying on a registration pattern.

Then this can also help to solve this
grpc/grpc-go#1873

If we have this simple example:

s := grpc.NewServer()
pb.RegisterYourOwnServer(s, &server{})
reflection.Register(s)
s.Serve(lis)

The reflection.Register can then call ServiceDesc to get the service descriptor.
Ah ok maybe that won't work, since the ServiceDesc does not have all the information.

@paranoiacblack
Copy link
Contributor

@dfawley Thanks for the feedback. We could export fileDescriptor<n> as you stated, but it seems like that's really just another way for users to get to the ServiceDescriptor and they'll have to write these GetServiceDesc functions in each of their clients or servers, anyways. I suppose it's fine to provide this mechanism.

@awalterschulze I'm not really familiar about the intersection between proto reflection and service registration. But yes, this could be used to get partial information about the service, such as its canonical name and canonical method set, before the service is altered on registration.

@mightyguava Let's move forward with your change in #516. I still haven't seen a compelling use-case for this feature and you haven't provided a demonstration of its utility because you say it's context-dependent. So I'm reluctant to be exporting yet another poorly understood feature in this library, but I'm willing to take on the technical debt for when we end up breaking it. Overall, it seems reasonable, even if I don't understand it.

@dfawley
Copy link
Contributor

dfawley commented Mar 6, 2018

@paranoiacblack maybe we should chat more about this offline, but my hope was that by exporting the full FileDescriptorProtos for every package directly, we could avoid the need for GetFooServiceDesc[riptor] functions in the grpc code generator. Users could pretty easily scan through the services in the package until they find the one they need by its name. The issue AIUI is that right now the only way to get access to the ServiceDescriptorProtos is by using the filename that contains the service, which may not be known at runtime.

@mightyguava in #516, why do also want access to ServiceDescs?

@mightyguava
Copy link

mightyguava commented Mar 12, 2018

@dfawley I want to be able to programmatically reference something that I can use to inspect the service. It doesn't matter as much whether this is a getFooServiceDescriptor() method or a getFooServiceDesc() method. I exposed both in the PR figuring that someone would tell me one is better than the other. I don't think we touched on this point in the discussion over the past few weeks...

Did you guys end up chatting offline? I can see having list of FileDescriptorProtos for the package can open up a lot of possibilities. It doesn't solve the specific use case of being able to reference the descriptor/desc though.

@dfawley
Copy link
Contributor

dfawley commented Mar 12, 2018

It doesn't solve the specific use case of being able to reference the descriptor/desc though.

Why not? The FileDescriptorProtos should contain the ServiceDescriptorProtos you need -- no?

@mightyguava
Copy link

I would like to be able to directly reference getFooServiceDesc() or getFooServiceDescriptor() in code, rather than have to find it by string.

@dsnet
Copy link
Member

dsnet commented Apr 2, 2018

I apologize for jumping into this discussion late in the game.

I've been working on #364, which provides a behaviorally complete API for interfacing with proto messages. In order to do this, there needs to be an abstraction over the protobuf type system, which are encapsulated in proto as descriptors. As such, the API I'm working on fundamentally needs to involve some abstraction over the ProtoDescriptors. As I continue to work on this, it is becoming increasingly clear to me that Service and Method descriptors should be a part of the API that allows you to get the descriptors for all proto declarations.

I understand there is a desire for this feature today, but perhaps it should wait until work on #364 is complete. I'd rather not add another function that we will likely mark as deprecated in the near future.

@dsnet
Copy link
Member

dsnet commented Apr 2, 2018

It just occurred to me that this is entirely a subset of #293. Closing as duplicate.

@dsnet dsnet closed this as completed Apr 2, 2018
@prasek
Copy link

prasek commented Apr 27, 2018

For the OP's question, here's a simple way to get custom method and service options:
https://github.com/prasek/go-grpc-info

@dfawley
Copy link
Contributor

dfawley commented Jan 11, 2019

It just occurred to me that this is entirely a subset of #293. Closing as duplicate.

@dsnet,

  1. Given an import of a generated proto package (import "foopb"), will I be able to use the v2 API to enumerate all the FileDescriptorProtos in that package (e.g. foopb.FileDescriptorProtos)?

  2. Given a proto package name and a service name (as strings), will I be able to use the v2 API to access the ServiceDescriptorProto for that service?

The former is an important part of this issue, and the latter is also functionality that would be very useful for things like interceptors that are used with multiple services/methods and might need access to their related options.

@dsnet
Copy link
Member

dsnet commented Jan 11, 2019

tl;dr. Yes and Yes.

  • The v2 API will generate a Go variable for every proto file that contains the file descriptor. More powerfully, this will not be the raw bytes of the file descriptor, nor the file descriptor proto message itself, but a more structured representation that enables following dependencies or efficiently searching for declaration within a file.
  • Since a file descriptor has a list of the imports, it is planned that as long as you have one protoreflect.FileDescriptor, you can inspect that descriptor to obtain a transitive closure of all file dependencies. The caveat is that this may not work reliably initially. The problem is that in the open-source world, it takes time people to regenerate messages. If your generated file depends on a generated file that has not been recently regenerated with v2, we can't determine the file dependency.
  • The v1 file registration functionality will be enhanced in v2 to support registering a protoreflect.FileDescriptor. The current design of the v2 file registry provides a number of different lookup methods. Specifically, the protoregistry.Files.FindDescriptorByName method will allow you lookup a service by it's full name (which is functionally the concatenation of the proto package and the service name) and retrieve a protoreflect.Descriptor, which you can type assert to a protoreflect.ServiceDescriptor

@dfawley
Copy link
Contributor

dfawley commented Jan 11, 2019

@dsnet - That all sounds great to me. Thanks!

@golang golang locked as resolved and limited conversation to collaborators Jun 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

8 participants