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

Provide an interface to allow customized gRPC options for the gRPC client #2315

Closed
cygnrh opened this issue Jan 5, 2022 · 9 comments · Fixed by #2484
Closed

Provide an interface to allow customized gRPC options for the gRPC client #2315

cygnrh opened this issue Jan 5, 2022 · 9 comments · Fixed by #2484
Assignees
Labels
evaluation needed proposal needs to be validated or tested before fully implementing it in k6 feature
Milestone

Comments

@cygnrh
Copy link

cygnrh commented Jan 5, 2022

Feature Description

The current connect interface in the gRPC client uses a pre-defined gRPC options. This limits the capability of the gRPC client in that the dial options can't be configured in a customized gRPC connectivity environment. For an environment with more complex connectivity configurations such as with gRPC look-aside loadbalancer or authentications, an interface with a customized gRPC options is needed. The users can leverage the current gRPC client by creating a k6 extension to pass in the customized gRPC options.

Suggested Solution (optional)

No response

Already existing or connected issues / PRs (optional)

No response

@cygnrh cygnrh added the feature label Jan 5, 2022
@na--
Copy link
Member

na-- commented Jan 5, 2022

Sorry, I am not sure I understand what you mean 🤔 You are not just saying that the k6 gRPC support lacks some things, but also that the params approach of configuring the Client is insufficient? That is, we should have the equivalent of DialOption and/or interceptors?

@na-- na-- added the evaluation needed proposal needs to be validated or tested before fully implementing it in k6 label Jan 5, 2022
@shibrady
Copy link

shibrady commented Jan 5, 2022

We'd like to leverage k6's grpc module for load testing, but the way we currently connect to gRPC servers requires some custom DialOptions such that we can't use k6/net/grpc out of the box. One way we were thinking about doing this (without forking code) was by writing an xk6 extension that wraps over the existing grpc client. But at the moment, I don't think we'd be able to wrap it in such a way where we can provide those custom DialOptions.

We were wondering whether it'd be possible to e.g. modify the Connect function to be able to pass in those options, like:

func (c *Client) Connect(ctxPtr *context.Context, addr string, params map[string]interface{}, addlOpts ...grpc.DialOption) (bool, error) {
  // ... later on
  opts = append(opts, addlOpts...)
}

... where our custom xk6 extension would essentially have something like:

func (c *MyWrapperClient) Connect(ctxPtr *context.Context, addr string, params map[string]interface{}) (bool, error) {
  myCustomDialOpts := getCustomDialOpts()
  return c.k6Client.Connect(ctxPtr, addr, params, myCustomDialOpts...)
}

... although this admittedly seems like it'd only exist for something like a separate xk6 extension, and wouldn't be very useful in the context of an actual k6 script (with a mostly useless param).

@na--
Copy link
Member

na-- commented Jan 6, 2022

🤔 I think I have an idea how this can be implemented in a backwards-compatible way that also improves the code quality for the gRPC module! 🎉 🤞

Consider how the k6/http module is spit:

  • the JS boundary layer (i.e. parsing and validating the params passed from JS and building the requests) is in /js/modules/k6/http
  • the "business logic" (i.e. actually making and measuring HTTP requests) is in /lib/netext/httpext

We refactored it that way a long time ago to have some clearer separation of concerns and to make testing a bit easier. However, as a side-benefit, when the code is split like this, it'll be much easier to potentially support non-JS runtimes (#751) in the future and interoperate with xk6 extensions now! There are still plenty of remaining things to fix and improve there, but the idea is the important part.

If we split the gRPC module in a similar way, we can leave only the option parsing and validation in the js/modules/k6/grpc module. The rest (protobuf parsing, request making and measurement) can be a proper well-typed Go code that can be used by any xk6 extension much more easily. For example, instead of a params map[string]interface{} mess, the non-JS code probably should use the proper grpc.DialOption abstractions for its constructor, so you can add custom ones from the extension 🤷‍♂️ WDYT?

@shibrady
Copy link

shibrady commented Jan 6, 2022

Honestly, all of those points sound great to me! Certainly a much less hacky approach than what I was suggesting.

@cygnrh
Copy link
Author

cygnrh commented Jan 6, 2022

@na-- I like the idea of separation of concerns. It give a lot more flexibility in coding around the gRPC module in K6 and makes the code cleaner. What's your plan or timeline on the code split with the gRPC module.

@na--
Copy link
Member

na-- commented Jan 7, 2022

One of us could probably start working on this in the next several weeks, after we release k6 v0.36.0. However, in this case, since you already have a complicated gRPC use case that requires this flexibility, it might be better if you try to make a PR and we support you with suggestions and PR reviews? 🤔

My thinking is that if we try to refactor the code to be cleaner, we have no way of knowing if the new architecture will work for you... 🤷‍♂️ Whereas, if you refactor the code along with writing your xk6 extension, you can see if it'll work for your use case, while we can ensure that it will be fine for merging in k6 through the PR reviews and discussions in this issue. Our merging criteria are that it's well tested, improves future maintainability, doesn't introduce any new bugs or breaking changes for JS scripts, and doesn't tie our hands by making planned future features (like gRPC streaming support and async operations) much more complicated.

@cygnrh
Copy link
Author

cygnrh commented Jan 15, 2022

Sure, we can look at it when it fits in our project planning.

@codebien
Copy link
Contributor

Hi @shibrady, we would work on this issue for the next cycle. Can you clarify this part, please?

We'd like to leverage k6's grpc module for load testing, but the way we currently connect to gRPC servers requires some custom DialOptions such that we can't use k6/net/grpc out of the box. One way we were thinking about doing this (without forking code) was by writing an xk6 extension that wraps over the existing grpc client. But at the moment, I don't think we'd be able to wrap it in such a way where we can provide those custom DialOptions.

Is the ability to set custom options the unique missing part for not using directly the k6/net/grpc module or do you need the extension in any case for doing internal/advanced logic?

If you can share, it would be also helpful to know which specific options you would set.

@cygnrh
Copy link
Author

cygnrh commented Mar 18, 2022

@codebien what we need is the ability to pass in the custom options to the k6/net/grpc module. That way we can build our own extension to create the custom options and leverage the k6 module for our use case. Does this make sense to you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
evaluation needed proposal needs to be validated or tested before fully implementing it in k6 feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants