-
Notifications
You must be signed in to change notification settings - Fork 210
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 reflection #148
Comments
I wanted to support this, but don't have time recently. I'm happy to see contribution. 👍 |
Hi guys! |
Hi guys! |
I'm not working on it in the foreseeable future so feel free to take it :) |
Okay, thanks @hkrutzer. I just wanted to know the status. I have no technical conditions for this at the moment. |
Is this issue still relevant? |
Yes |
@hkrutzer Do you want to take this on as a PR? |
@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 |
@polvalente I started this task as a draft in PR #254 |
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 This has three steps to work:
I wrote it against |
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: And here: My intention was that we could add the filter of which endpoints to reflect using an option directly in the endpoint definition, something like But approaching it as a separate library also makes sense. I will take a look at your work. Very good. |
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. |
@mjheilmann What are your thoughts on this? Would you like to submit this as a feature? |
@polvalente I can try to share the burden with you if necessary. |
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.
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 |
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. |
I think this would be a good tradeoff. Wdyt? |
I like the idea of having a default implementation + overridable behaviour |
+1 |
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. Note. Behavior here can be interpreted beyond the elixir's reserved word. |
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. |
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. 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. |
I'm sorry, I am clearly having trouble interpreting what you are saying. What is
supposed to mean when both you and I leveraged the existing binding that elixir-grpc exposes: the 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. |
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. I hope I was able to explain the main concept better from my summary. |
@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. |
@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 Yes, I agree with you 100% and I wish your project continued success. |
Let's close this issue for now, as given all the discussion I also agree that it makes more sense for the feature to be composed into the framework as a separate library. How do you all feel about us bringing the repo into the elixir-grpc org + pointing to it on this repo's README? |
Just to record my answer to @mjheilmann and @conradwt I wasn't proposing that grpc be implemented in core. I suggested that the details should be implemented by third-party libraries or a separate library, but in a way that the main library has the links. I don't see how this would break the component model you proposed. That said, I'm glad that an agreement was reached on how to proceed. |
@polvalente That sounds like a good idea. Is there anything I would need to do to enable that, or would the elixir-grpc org just fork it and I point my repo back to you? |
@mjheilmann https://docs.github.com/en/repositories/creating-and-managing-repositories/transferring-a-repository You would transfer the repository, and I'll add you to the org with owner access to the repo |
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.
The text was updated successfully, but these errors were encountered: