grpc icon indicating copy to clipboard operation
grpc copied to clipboard

gRPC reflection

Open hkrutzer opened this issue 5 years ago • 11 comments
trafficstars

Is your feature request related to a problem? Please describe. When I use a tool such as grpcurl it cannot use server reflection and one must specify the protocol buffer files.

Describe the solution you'd like Support gRPC reflection

Describe alternatives you've considered This is a non-essential feature and I don't know how much time it would take to implement. I might attempt to contribute it myself but I would like to discuss it first.

hkrutzer avatar Feb 27 '20 15:02 hkrutzer

I wanted to support this, but don't have time recently. I'm happy to see contribution. 👍

tony612 avatar Mar 21 '20 13:03 tony612

Hi guys! Undoubtedly having this functionality would be very important. Generally, most people mention that this would be important only for things like grpcurl, however, there are many important use cases like proxies, dynamic routers, and etc. that could be built on top of this functionality. I for example work on a project that creates a sidecar kubernetes pod and implements the grpc interfaces of the adjacent container. This is only possible because the sidecar is able to reflect the client container grpc contract. Today we do this in Scala + AKKA gRPC but if we could implement it in Elixir it would be much more efficient and at a much lower cost in cluster resources.

sleipnir avatar Apr 30 '20 05:04 sleipnir

Hi guys! Any news here?

sleipnir avatar Jul 18 '20 21:07 sleipnir

I'm not working on it in the foreseeable future so feel free to take it :)

hkrutzer avatar Jul 20 '20 07:07 hkrutzer

Okay, thanks @hkrutzer. I just wanted to know the status. I have no technical conditions for this at the moment.

sleipnir avatar Jul 20 '20 14:07 sleipnir

Is this issue still relevant?

polvalente avatar Jul 18 '22 08:07 polvalente

Yes

hkrutzer avatar Jul 18 '22 10:07 hkrutzer

@hkrutzer Do you want to take this on as a PR?

polvalente avatar Jul 18 '22 10:07 polvalente

@sleipnir Have you implemented this on your fork? Do you want to bring the implementation to the repo?

polvalente avatar Jul 19 '22 15:07 polvalente

@sleipnir Have you implemented this on your fork? Do you want to bring the implementation to the repo?

@polvalente I will need some time before I get busy with other Eigr project tasks but I promise to implement this as soon as I can

sleipnir avatar Jul 19 '22 17:07 sleipnir

@polvalente I started this task as a draft in PR https://github.com/elixir-grpc/grpc/pull/254

sleipnir avatar Jul 27 '22 18:07 sleipnir

Hey everyone. I've been following this issue and @sleipnir 's work for a bit, and started playing on his branch to see if I could contribute to the work. I went in a different direction and have a partially working prototype that adds reflection to a grpc-elixir server. It's currently up at https://github.com/mjheilmann/grpc-reflection. I wrote it as a separate package that can be optionally included as it does not require any changes in elixir-grpc to function.

This has three steps to work:

  1. The services and types they expose must have their proto files compiled with gen_descriptors on
  2. The services returned by "list services" is set in your application config
  3. One or all of the reflection servers must be run in your endpoint.

I wrote it against grpcurl, but added v1alpha so postman would work too. It takes a few shortcuts internally, so it could use some more testing, and perhaps someone who knows how Protobuf extensions work.

mjheilmann avatar Nov 12 '23 19:11 mjheilmann

Hey everyone. I've been following this issue and @sleipnir 's work for a bit, and started playing on his branch to see if I could contribute to the work. I went in a different direction and have a partially working prototype that adds reflection to a grpc-elixir server. It's currently up at https://github.com/mjheilmann/grpc-reflection. I wrote it as a separate package that can be optionally included as it does not require any changes in elixir-grpc to function.

This has three steps to work:

  1. The services and types they expose must have their proto files compiled with gen_descriptors on

Hi, great job @mjheilmann, I'll take a look. I preferred to use the protoc descriptor file directly as it contains all the type information that your protobufs depend on in a single descriptor. I didn't have time to update the work here but there is a version that I used in another project here:

https://github.com/eigr/massa/blob/main/apps/massa_proxy/lib/massa_proxy/reflection/reflection_service.ex

And here:

https://github.com/eigr/massa/blob/main/apps/massa_proxy/lib/massa_proxy/reflection/reflection_server.ex

My intention was that we could add the filter of which endpoints to reflect using an option directly in the endpoint definition, something like reflect: :true

But approaching it as a separate library also makes sense. I will take a look at your work. Very good. @polvalente, do you think we should follow the path of a separate library or continue along the lines of incorporating this support directly into the main library?

sleipnir avatar Nov 13 '23 12:11 sleipnir

I think going with something in the main library would be easier to maintain in the sense that changes can be applied atomically through all parts. There's the downside of me being a bus factor, though, which I'd like to solve in the near future.

polvalente avatar Nov 27 '23 04:11 polvalente

@mjheilmann What are your thoughts on this? Would you like to submit this as a feature?

polvalente avatar Nov 27 '23 04:11 polvalente

I think going with something in the main library would be easier to maintain in the sense that changes can be applied atomically through all parts. There's the downside of me being a bus factor, though, which I'd like to solve in the near future.

@polvalente I can try to share the burden with you if necessary.

sleipnir avatar Nov 27 '23 15:11 sleipnir

I'm very glad to see this feature receiving some attention again. Please allow me to join the conversation and share an opinion on the "main library vs. separate package" topic.

With the exception of Java, all languages that implement the reflection service are done in a separate package.

Language Type Strategy Ref
Java Official Integrated Link
Go Official Separate Link
C++ Official Separate Link
Python Official Separate Link
Node Community Separate Link
.Net Community Separate Link
Rust Community Separate Link

I think that's the way to go. A new repo, and package, delegated to just do reflection would also allow services running in previous versions of the elixir-grpc to work (In theory).

ianko avatar Nov 27 '23 21:11 ianko

I think that's the way to go. A new repo, and package, delegated to just do reflection would also allow services running in previous versions of the elixir-grpc to work (In theory).

Although many languages adopt this standard, I don't see this argument alone as good enough to follow separately. Not that I'm opposed to going this route, I just don't think it should be done completely decoupled from the main library. Let me explain my reasoning. I think we can define a contract in elixir-grpc and a way to explicitly configure the implementation of that contract via the core library, that way customers can implement that functionality as they see fit, and different implementations can emerge without fragmenting the ecosystem.

sleipnir avatar Nov 27 '23 23:11 sleipnir

.... I think we can define a contract in elixir-grpc and a way to explicitly configure the implementation of that contract via the core library, that way customers can implement that functionality as they see fit, and different implementations can emerge without fragmenting the ecosystem.

I think this would be a good tradeoff. Wdyt?

sleipnir avatar Nov 28 '23 00:11 sleipnir

I like the idea of having a default implementation + overridable behaviour

polvalente avatar Nov 28 '23 04:11 polvalente

I like the idea of having a default implementation + overridable behaviour

+1

ianko avatar Dec 01 '23 00:12 ianko

I like the idea of having a default implementation + overridable behaviour

I'm confused as to how this would work. The behavior is dictated by the reflection grpc service protos, which are implemented and run similar to any users' service implementation.

mjheilmann avatar Dec 02 '23 00:12 mjheilmann

I like the idea of having a default implementation + overridable behaviour

I'm confused as to how this would work. The behavior is dictated by the reflection grpc service protos, which are implemented and run similar to any users' service implementation.

The behavior in question is how you would connect the implementations with elixir-grpc. And not to the grpc reflection contract itself, which we already know is a spec. For example, the service would be enabled by individual endpoint as a Service option, or it would be a global configuration, or anything else other than that. This is the kind of thing we must deal with when implementing elixir-grpc connections with reflection.

Note. Behavior here can be interpreted beyond the elixir's reserved word.

sleipnir avatar Dec 02 '23 12:12 sleipnir

That is why I am confused. Elixir-grpc does not depend on, rely on, or even need to know about reflection. Reflection requires no modification nor special treatment from elixir-grpc. Both in my repo, and in your PR @sleipnir, we implement reflection as a separate grpc service because that is what the reflection spec requires.

If we look at Cowboy and extensions there, libraries typically implement a Plug. The reflection implementation here is like a Plug implementation for Cowboy. Cowboy does not require all Plugs, but all Plugs require Cowboy. Cowboy does not include most plugs, leaving their inclusion, exclusion, or use up to its users.

I think that Reflection does not need to be merged into elixir-grpc, which is why I have a repo and not a PR into here. This is more flexible for users to opt-into, with the added bonus of being granular as to what services have reflection enabled.

mjheilmann avatar Dec 02 '23 13:12 mjheilmann

As I said before, I partly agree with the separate strategy. But it is valid to provide the necessary binds for the different core implementations. We can adapt our personal implementations to follow this model without any problems, I believe. This also creates bridges for the main library to evolve to follow a more componentized model, allowing different parts to evolve independently.

See that for example we do not need to implement the grpc server, but only the search methods for types and contracts. The server/endpoint itself could be implemented directly in the core library. Something similar to how Cachex does. This also eliminates unnecessary code in the library that only implements the logic of searching and loading type metadata via reflection. In the end, I believe everyone wins. But that's just my view.

sleipnir avatar Dec 02 '23 15:12 sleipnir

I'm sorry, I am clearly having trouble interpreting what you are saying. What is

it is valid to provide the necessary binds for the different core implementations

supposed to mean when both you and I leveraged the existing binding that elixir-grpc exposes: the run macro?

This seems like a clear case of "separation of concerns" to me, with the largest argument towards combining these different libraries being that it may be easier to maintain the reflection library if it were combined with the elixir-grpc library.

mjheilmann avatar Dec 02 '23 16:12 mjheilmann

Sorry, I think I couldn't explain the concept, let me try again please. But in short it's very simple. The main library must expose the necessary mechanisms so that the end user can configure (and I'm not interested in how yet) reflection in their application without having to know anything about the underlying implementation. Unless he wants to use a different implementation, in this case he would have to configure this via the main library as well, in addition to, of course, adding the dependency to his project.

And I understand your initial point about almost all other libraries not going this route. But I don't see this being common in other Elixir projects. And here a small concession is made so that the end user can follow the path they want with regard to implementation is that they could configure this in their project.

I believe that once the mechanisms necessary for links with implementations to occur. It would be simple to change your library to follow the new path. However your strategy would still be valid since nothing will be broken. So for those who want to use what you've already created, this would work accordingly. So your work, one way or another, will not be lost I believe.

I hope I was able to explain the main concept better from my summary.

sleipnir avatar Dec 03 '23 00:12 sleipnir

@mjheilmann However, I would recommend that gRPC reflection implementation belong within a single organization which is similar to the other language implementations.

@sleipnir gRPC reflection isn't a core requirement for any gRPC implementation. That's why many gRPC language implementations have decided to separate this functionality into an external module and/or package. Thus, making it opt-out by default. If you need it, you can install and configure the additional package. Next, I personally would like to include the least amount of third-party code to get the job done because other forthcoming gRPC plugins exist today or will appear very soon that should be opt-out by default. For example, there are plugins like grpc-gateway and grpc-transcoding (.NET) which translate a RESTful HTTP call into gRPC. In short, I would like the core Elixir-gRPC library slim with a focus on performance and correctness.

conradwt avatar Dec 03 '23 06:12 conradwt

@sleipnir "The main library", "must expose...configure", these have assumptions built into them. Configurable monoliths may be common in some languages, but in my experience things are more easily supported, developed, and tested when they are assembled through composition.

@conradwt 🎉 If my repo is useful enough to get adopted into an org I have no complaints, I just don't want it getting merged into another lib where it doesn't make sense.

mjheilmann avatar Dec 03 '23 16:12 mjheilmann

@mjheilmann Yes, I agree with you 100% and I wish your project continued success.

conradwt avatar Dec 04 '23 01:12 conradwt