Support for gRPC protocol #1623

rogchap · 2020-09-09T07:33:37Z

fixes #441

Initial implementation only supports Unary requests.

Example JavaScript API:

import grpc from 'k6/protocols/grpc';
import { check } from "k6";

const client = grpc.newClient();
client.load([], "samples/grpc_server/route_guide.proto")


export default () => {
    client.connect("localhost:10000", { plaintext: true })

    const response = client.invoke("main.RouteGuide/GetFeature", {
        latitude: 410248224,
        longitude: -747127767
    })

    check(response, { "status is OK": (r) => r && r.status === grpc.StatusOK });

    client.close()
}

…ject

CLAassistant · 2020-09-09T07:34:03Z

All committers have signed the CLA.

rogchap · 2020-10-19T01:02:00Z

Hey @na-- Hope you had a good weekend?
Ok...so I think I have all the changes done; and all CI checks are now green 😅
Let me know if I've missed anything, but I think we are good to merge now.

mstoykov

I can't find any major problems. I commented in two places on things that I think will be better but we can fix them later as well

mstoykov · 2020-10-20T08:56:58Z

js/modules/k6/grpc/client.go

+	timeout := 60 * time.Second
+
+	ctx = metadata.NewOutgoingContext(ctx, metadata.New(nil))
+	for k, v := range params {


I am more and more of the opinion that https://github.com/loadimpact/k6/blob/master/js/modules/k6/http/request.go#L248-L351 should be split and moved to utility functions and then reused here and in in the ws code.
But this should be done in a different PR

definitely a separate PR

mstoykov · 2020-10-20T09:00:46Z

js/modules/k6/grpc/client.go

+
+	var response Response
+	response.Headers = header
+	response.Trailers = trailer
+
+	marshaler := protojson.MarshalOptions{EmitUnpopulated: true}
+


I do think it will be more readable if this is moved before the Invoke so taht if err != nil can be directly after the Invoke.
Also making the ctx should probably be directly before the Invoke and you can skip the header/trailer varaible and directly use response.Headers/response.Trailers as far as I can see

If I directly use response.Headers/response.Trailers this results in issues with casting, as one is map[string][]string and the other is metadata.MD which is an alias to map[string][]string. If I change the Response struct to use metadata.MD then this will not serialize correctly via goja (unmarshals to an empty object {})

mstoykov · 2020-10-20T09:19:42Z

js/modules/k6/grpc/client.go

+		close(errc)
+	}()
+
+	if err := <-errc; err != nil {


Would canceling the ctx ... make the ~~goroutine~~ Dial return faster?
Maybe we can drop the goroutine ... have the transportCreds remember the first error they get and cancel the context on it?

Possibly. We wound have to attach a context cancel function to the transportCreds object at the beginning, and in the event of an error call cancel(), and then store the TLS error on the transportCreds to later retrieve the error to check if the error was caused by TLS (as appose to some other context cancellation).

In my opinion this last part of checking an error field on a struct is not idiomatic Go and is prone to being missed (hence why I don't like the builder pattern in Go, you have to remember to check that error field).
At this point I'm still in-favour of the error channel an goroutine as I think it reads cleaner.

na-- · 2020-10-20T09:52:31Z

js/modules/k6/grpc/client.go

+
+// MethodInfo holds information on any parsed method descriptors that can be used by the goja VM
+type MethodInfo struct {
+	grpc.MethodInfo


Suggested change

grpc.MethodInfo

grpc.MethodInfo `json:"-" js:"-"`

I think I was a bit overzealous with the suggestion for code reuse here 😞 Without this change, if you run something like:

let methods = client.load([], "samples/grpc_server/route_guide.proto"); console.log(JSON.stringify(methods, null, 4));

you'd get this:

[ { "method_info": { "name": "GetFeature", "is_client_stream": false, "is_server_stream": false }, "name": "GetFeature", "is_client_stream": false, "is_server_stream": false, "package": "main", "service": "RouteGuide", "full_method": "/main.RouteGuide/GetFeature" }, { "method_info": { "name": "ListFeatures", "is_client_stream": false, "is_server_stream": true }, "name": "ListFeatures", "is_client_stream": false, "is_server_stream": true, "package": "main", "service": "RouteGuide", "full_method": "/main.RouteGuide/ListFeatures" }, // ... ]

but yeah, I'm not sure if the anonymous struct reuse is worth it at this point 🤷‍♂️

na-- · 2020-10-20T09:58:46Z

js/modules/k6/grpc/client.go

+			case int64:
+				timeout = time.Duration(float64(t)) * time.Millisecond
+			case float64:
+				timeout = time.Duration(t) * time.Millisecond


See https://play.golang.org/p/gsOWwDPPAV3 for the rationale

Suggested change

case int64:

timeout = time.Duration(float64(t)) * time.Millisecond

case float64:

timeout = time.Duration(t) * time.Millisecond

case int64:

timeout = time.Duration(t) * time.Millisecond

case float64:

timeout = time.Duration(t * float64(time.Millisecond))

Very interesting.

na-- · 2020-10-20T09:59:14Z

js/modules/k6/grpc/client.go

+			case int64:
+				timeout = time.Duration(float64(t)) * time.Millisecond
+			case float64:
+				timeout = time.Duration(t) * time.Millisecond


See https://play.golang.org/p/gsOWwDPPAV3 for the rationale

Suggested change

case int64:

timeout = time.Duration(float64(t)) * time.Millisecond

case float64:

timeout = time.Duration(t) * time.Millisecond

case int64:

timeout = time.Duration(t) * time.Millisecond

case float64:

timeout = time.Duration(t * float64(time.Millisecond))

na--

Besides the few minor nitpicks, LGTM! 🎉 Awesome 🎊

imiric

Stellar work @rogchap! 👏

I don't have any gRPC experience, but this was easy to read (minus the vendor changes 😆) and use. Don't have any code comments, just spotted a couple of typos.

Though would you mind doing some cleanup/squashing of commits before merging?

js/modules/k6/grpc/client.go

na-- · 2020-10-20T12:06:30Z

Regarding squashing - I was thinking we can squash this in just a single commit when merging the PR, through the GitHub interface? I know it has 250k LoC difference, but 99% of that is vendor/ changes... The actual k6 changes are very tidy and self-contained, so I don't think we'd loose anything if we squash them. I don't actually see a nice way to split them in multiple commits 🤷‍♂️

And if @rogchap doesn't modify the commits and force push in this PR, we would be able to look up the change history or the rationale behind some code here, just in case. Sort of the best of both worlds with this type of a change, I think?

So... here's my suggestion on how to proceed:

@rogchap merges the latest master in here, again, since we changed some module versions and httpmultibin (sorry! 😞 ), no force pushes or commit cleanup/squashing
fixes the minor nitpicks and typos we've found
we give it a final 👍 and squash+merge it in master?
party 🎉

Any objections?

btw, regarding the actual gRPC module name - we still haven't decided it. So we'll likely merge this PR with the current k6/grpc name, and rename it to k6/protocols/grpc (or whatever we decide) in a separate small PR before we release k6 v0.29.0. This way you won't have to rebase it 10 more times before we make our minds up 😅

rogchap · 2020-10-20T12:14:26Z

Only a few commits in this PR 😉
I'm especially fond of the number of commits with this message: "the linter hates me" 🤣

Sounds like a plan; for what it's worth: in our company we have set the GitHub settings in all repos to only allow squash+merge from our PRs, so seldom need to manually squash/rebase.

Will address all comments; 11pm here in the future (Sydney) so will take a look in the morning.

na-- · 2020-10-20T18:49:11Z

👍 Sounds good! btw for now we're tentatively going with k6/protocols/grpc as the module name, so please change it (and the samples/grpc.js example) as well.

rogchap · 2020-10-21T03:29:05Z

Ok will update to k6/protocols/grpc

Just my 2¢ on the subject:

I like short import/require statements like k6/http, k6/grpc (ironic as Go imports tend to be long)
I understand the need for new versions of http and others, but it's unclear to a user that k6/http is old and k6/protocol/http is the new
what would happen if in the future you have a 3rd version of k6/http etc?
Have you considered version numbers similar to Go and Protocol Buffers? eg. k6/v2/http or even k6/http/v2
If I had to choose, my preference (in order) would be k6/proto/grpc then k6/protos/grpc, k6/protocol/grpc and finally k6/protocols/grpc

na-- · 2020-10-21T06:45:29Z

I like short import/require statements like k6/http, k6/grpc (ironic as Go imports tend to be long)

I can sympathize, but we'd like to support a lot of other protocols (e.g. k6/protocols/kafka, k6/protocols/sql, k6/protocols/dns, k6/protocols/tcp, etc.), and also have several non-protocol k6 modules, which would probably occupy the shorter paths. For example, k6/execution (#1320), k6/metrics (already exists, but it will have additions with #1321), k6/data (#1539), etc. (those are probably not the final names).

I understand the need for new versions of http and others, but it's unclear to a user that k6/http is old and k6/protocol/http is the new

We will support k6/http for a long, long time, even when we have a new version. We might start emitting deprecation warnings, but we can't kill the old API, since 99% of existing scripts use it... We're likely just going to re-implement the old k6/http as a JS wrapper around the new one, but we'll see. It's still early to tell, since we keep delaying that new API in favor of other k6 features.

what would happen if in the future you have a 3rd version of k6/http etc?
Have you considered version numbers similar to Go and Protocol Buffers? eg. k6/v2/http or even k6/http/v2

This is one of the main issues we're struggling right now in our internal discussions... The other one is how to make clear to users that an API's stability is alpha (i.e. the API change or even disappear in future k6 versions) or beta (i.e. the API will stick around, but might still change a little before it stabilizes). We can document these things in the release notes and the documentation, but a lot of people just don't read these things... 😞

The current 2 top contender strategies are versioned paths, like you proposed (k6/protocols/http/v3, k6/protocols/grpc/beta, etc.), and feature flags/gates (a bit like kubernetes, so something like k6 run --features='grpc=v2,http=v1'). I currently lean towards the feature flags slightly, since they'd allow us to gradually add new (i.e. potentially unstable) features in an easy and clear way, and more or less force us to deprecate old ones, while at the same time allowing us to do it very gracefully.

The biggest drawback to versioned paths for me, especially if we go with /alpha / /beta for the initial module versions, is that this is basically the only guaranteed way that users will have to change their scripts. But it's also much less opaque than the feature gates - having global flags that modify the script behavior is a bit of an anti-pattern (which we suffer from in other places in k6 already).

na--

🎉

na--

🎉

imiric

Thank you!

rogchap added 9 commits September 3, 2020 22:19

simple starting point to have grpc (unary) requests

d1a6073

add gRPC specific metrics and refactor js API

5b52b07

add gRPC status codes and use state Dialer

2410ef5

add support for TLS

bb05b01

manage some of the errors and add the message JSON to the response ob…

be8f784

…ject

add support for custom tags, headers and timeout

989f90e

place client in it's own file

e08853c

fix TLS issues and add a timeout on the connection

3d25f99

some basic testing of the grpc client

28d5a86

rogchap added 11 commits September 10, 2020 08:54

add some standard tags

5c15a25

Add Headers/Trailers and seperate the Error message in the response

f431c07

tests for request, headers, reponse, errors, res headers and trailers

623dbbb

test for metrics an fix golint messages

f08a5bf

update unix package

070b37a

fix unix vendoring

614be83

fix up the system tags test and deal with some of the lint issues

657db28

more linting "fixes"

4ce280e

run enumer for new system tags

8250192

more linting errors

c46e726

linter really does not like me

f0e9c9c

rogchap changed the title ~~[WIP] Support for gRPC protocol~~ Support for gRPC protocol Sep 11, 2020

rogchap added 4 commits September 11, 2020 11:09

update package to match master

5f91c4c

Merge branch 'master' into grpc

907207f

vendor clean up

8ed72d4

hack to fix windows file not found error

eaeef08

This comment has been minimized.

Sign in to view

na-- requested review from imiric and mstoykov September 11, 2020 06:09

na-- added this to the v0.29.0 milestone Sep 11, 2020

rogchap added 6 commits October 19, 2020 09:49

deal with prealloc lint check

32d1812

handle serialisation errors

6c48328

scope error handling

74df86d

add tests for debug stats and support v1 proto messages

83c390b

need the APIv1 to support v1 proto messages

0f4baf0

gofumpt-ed

f7482b5

rogchap requested a review from na-- October 19, 2020 00:59

mstoykov previously approved these changes Oct 20, 2020

View reviewed changes

na-- reviewed Oct 20, 2020

View reviewed changes

imiric previously approved these changes Oct 20, 2020

View reviewed changes

js/modules/k6/grpc/client.go Outdated Show resolved Hide resolved

js/modules/k6/grpc/client.go Outdated Show resolved Hide resolved

rogchap added 2 commits October 21, 2020 14:08

address feedback from team

4c83ec6

Merge branch 'master' into grpc

512d34f

rogchap dismissed stale reviews from imiric and mstoykov via 512d34f October 21, 2020 03:13

update import to use protocols dir

29a4b25

na-- reviewed Oct 21, 2020

View reviewed changes

na-- approved these changes Oct 21, 2020

View reviewed changes

imiric approved these changes Oct 21, 2020

View reviewed changes

na-- merged commit 9d9cea4 into grafana:master Oct 21, 2020

na-- mentioned this pull request Jan 13, 2021

Discourage usage of the --rps option in favor of arrival-rate grafana/k6-docs#187

Closed

codebien mentioned this pull request Feb 3, 2022

js/grpc: modules.Module migration #2365

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for gRPC protocol #1623

Support for gRPC protocol #1623

rogchap commented Sep 9, 2020 •

edited by na--

CLAassistant commented Sep 9, 2020 •

edited

This comment has been minimized.

rogchap commented Oct 19, 2020

mstoykov left a comment

mstoykov Oct 20, 2020

na-- Oct 20, 2020

mstoykov Oct 20, 2020

rogchap Oct 21, 2020

mstoykov Oct 20, 2020 •

edited

rogchap Oct 21, 2020

na-- Oct 20, 2020

na-- Oct 20, 2020 •

edited

rogchap Oct 21, 2020

na-- Oct 20, 2020

na-- left a comment

imiric left a comment

na-- commented Oct 20, 2020

rogchap commented Oct 20, 2020

na-- commented Oct 20, 2020

rogchap commented Oct 21, 2020 •

edited

na-- commented Oct 21, 2020 •

edited

na-- left a comment

na-- left a comment

imiric left a comment

Support for gRPC protocol #1623

Support for gRPC protocol #1623

Conversation

rogchap commented Sep 9, 2020 • edited by na--

CLAassistant commented Sep 9, 2020 • edited

This comment has been minimized.

rogchap commented Oct 19, 2020

mstoykov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstoykov Oct 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

na-- Oct 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

na-- left a comment

Choose a reason for hiding this comment

imiric left a comment

Choose a reason for hiding this comment

na-- commented Oct 20, 2020

rogchap commented Oct 20, 2020

na-- commented Oct 20, 2020

rogchap commented Oct 21, 2020 • edited

na-- commented Oct 21, 2020 • edited

na-- left a comment

Choose a reason for hiding this comment

na-- left a comment

Choose a reason for hiding this comment

imiric left a comment

Choose a reason for hiding this comment

rogchap commented Sep 9, 2020 •

edited by na--

CLAassistant commented Sep 9, 2020 •

edited

mstoykov Oct 20, 2020 •

edited

na-- Oct 20, 2020 •

edited

rogchap commented Oct 21, 2020 •

edited

na-- commented Oct 21, 2020 •

edited