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

Support to use gRPC Server Reflection #1930

Closed
hojongs opened this issue Mar 28, 2021 · 5 comments · Fixed by #2160
Closed

Support to use gRPC Server Reflection #1930

hojongs opened this issue Mar 28, 2021 · 5 comments · Fixed by #2160

Comments

@hojongs
Copy link

hojongs commented Mar 28, 2021

Feature Description

gRPC Server reflection allow gRPC client to gRPC call without proto file.
https://github.com/grpc/grpc/blob/master/doc/server-reflection.md

ghz, is another gRPC load testing tool, supports this feature.
https://ghz.sh/docs/options#--proto

I saw this issue #441 (comment). in the issue, gRPC Server reflection feature is mentioned.
So I looked k6 issues&PRs about this feature but there isn't.
Do you have a plan for detail schedule for it?

Indeed, k6 is using this library https://github.com/jhump/protoreflect and it seems already to contain server reflection feature. https://github.com/jhump/protoreflect#grpc-server-reflection

Suggested Solution (optional)

I'm not sure but how about using server reflection feature in jhump/protoreflect ?
Maybe, Can this API be used? https://pkg.go.dev/github.com/jhump/protoreflect/grpcreflect#NewClient

@na--
Copy link
Member

na-- commented Mar 28, 2021

IIRC, we didn't add server reflection initially because we tried to make the gRPC pull request an MVP of sorts, adding the bare minimum of features to try and minimize any breaking changes that may later be necessary. We didn't have xk6 as an experimental proving ground yet, so it was directly merged into the k6 core with a minimal amount of iteration 😅

But yes, server reflection is something we'd like to support. We don't plan on working on it ourselves soon, but we'll probably accept a good PR if someone submits it.

If you're interested in working on this, please share your implementation ideas here, before opening a PR. I haven't looked into it, but this should probably be an optional feature, maybe a parameter to the gRPC Client constructor? 🤷‍♂️

@joshcarp
Copy link
Contributor

joshcarp commented Apr 26, 2021

Hey,
I've implemented a change that solves this issue.
I've implemented a grpc.Client.reflect method that is similar to the grpc.Client.connect method.
A couple of things:

  • I didn't use github.com/jhump/protoreflect because it uses the old proto api, and the only thing that we need is to get the file descriptor protos, after that the processing is the same as load method.
    I'd love to have your input on this change!

@juwatanabe
Copy link

Would love to have seen this functionality in the latest dot release. Any reason why this wasn't included in v0.33?

@na--
Copy link
Member

na-- commented Jul 13, 2021

@juwatanabe, there were some issues with the PR (#1987) that weren't addressed in time for us to include it in v0.33.0. If they are fixed by the PR author or another contributor in roughly the next month, or if we have time to fix them ourselves, it should be included in v0.34.0.

@joshcarp
Copy link
Contributor

Hey I'm going to be working on this in the next couple of days (Apologies for the incorrect timeline; uni exams got in the way)

@inancgumus inancgumus pinned this issue Oct 5, 2021
@inancgumus inancgumus self-assigned this Oct 5, 2021
@inancgumus inancgumus unpinned this issue Oct 5, 2021
@inancgumus inancgumus linked a pull request Oct 5, 2021 that will close this issue
14 tasks
@na-- na-- added this to the v0.35.0 milestone Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants