lib/netext/grpcext: Split the gRPC module #2484

codebien · 2022-04-07T08:44:49Z

The split of the gRPC business logic from the js/k6/grpc module to a netext package.
It enables a simpler process for extending the networking+metric features for gRPC without the k6/js dependencies.

An example of how the API of this package can be used for creating gRPC extensions: codebien/xk6-grpc-example

Closes #2315

na-- · 2022-04-08T14:37:12Z

lib/netext/grpcext/conn.go

+// TODO: should we support grpc.CallOption?
+func (c *Conn) Invoke(ctx context.Context, method string, md metadata.MD, req Request) (*Response, error) {


should we support grpc.CallOption?

Yes, I think this might be a good idea, given that they are present in grpc.ClientConn.Invoke(). Can you think of a reason why we shouldn't support them now, even if we don't use them yet?

Also, do we need a method parameter here when we have a MethodDescriptor in the Request?

Also, do we need a method parameter here when we have a MethodDescriptor in the Request?

Unfortunately, yes. The FullName of the method descriptor returns the full name domain as a dot notation. Instead, Invoke wants the package.serviceName/methodName format. I don't know if the simple replacement of the last dot with a slash could be fine.
Apparently, also the protoc generator generates the url instead of get+replace of the method descriptor's full name.

The Descriptor's Parent() method could be optimal for the descriptor-based generation but the doc says that it's optional and it could return nil. 🤷‍♂️

I'm thinking if we could change the name from method to url like our js module's documentation does.

lib/netext/grpcext/conn.go

na-- · 2022-04-08T14:43:36Z

js/modules/k6/grpc/client.go

@@ -353,49 +253,22 @@ func (c *Client) dial(
 	reflect bool,
 	options ...grpc.DialOption,
 ) error {


This Client.dial() code doesn't seem it has anything to do with JavaScript, what is the reason to not move it to grpcext?

It opens the connection then it routes through reflection if requested from the parameter, I think this way of connecting it's a property of the js module. 🤔 It could be a good idea to change the name of the method that probably it's not accurate after the refactor because the real dialling part is now delegated to grpcext. This method instead is mostly the core part of the Connect method.

But it seems like the vast majority of the logic related to the reflection is still can be moved to a grpcext.

I belive we can even make a private a ReflectionClient. What js nodule needs are the *descriptorpb.FileDescriptorSet, so the grpcext.Conn can has a method that tries to load them. What do your think?

And by the number of the logic, the current Client.dial is more like Client.reflect than dial 😅

lib/netext/grpcext/conn.go

cygnrh · 2022-04-13T21:50:26Z

lib/netext/grpcext/conn.go

+	"google.golang.org/grpc/status"
+	"google.golang.org/protobuf/encoding/protojson"
+	"google.golang.org/protobuf/encoding/prototext"
+	"google.golang.org/protobuf/proto"


Do you consider using github.com/gogo/protobuf/proto which has faster mashalling/unmarshalling?

Hi @cygnrh, thanks for your review.

Unfortunately, we would prefer to not include it, we are very meticulous in adding new dependencies to k6, you can read some motivations in Dependencies.md.
We would prefer to fix allowing the marshaler and unmarshaler to be injected, it would allow us to remove also the tightly coupled JSON dependency. I opened an issue for it #2497

If you have relevant observations for the grpc module, it would be helpful to get your feedback in a new issue or in #1846.

We haven't observed performance issue since we only did the initial test. If we do see anything as k6 gRPC being more widely adopted, I'll post in the thread #1846.

cygnrh · 2022-04-13T21:50:30Z

lib/netext/grpcext/conn.go

+	}
+	return &Conn{
+		raw: conn,
+	}, nil


how about including options in this struct so we don't have to pass options again for Invoke?

I think WithDefaultCallOptions covers this case. Could it work for you?

I'm a bit confused that options ...grpc.DialOption is passed in from Dial and Invoke. If options is part of the Conn struct, Invoke can use it as a receiver function.

Would you add a slice DialOption field?

type Conn struct { Options []grpc.DialOption raw clientConnCloser }

and, calling the Conn.Invoke method, would you like to not pass the Call Options?

options := []grpc.DialOption{ ..... } conn, err := Dial(ctx, addr, options...) err = conn.Invoke(ctx, url, md)

If yes, then the issue is mainly related to the different types involved. The eventual Conn.Options couldn't be used on the underhood call grpc.Invoke because the Invoke method wants the CallOption type.

Please, correct me if I misunderstood.

I guess a User can just use https://pkg.go.dev/google.golang.org/grpc#WithDefaultCallOptions as well

I see. They are different types of options. That makes sense.

It splits the gRPC business logic from the js/k6/grpc module to a netext package. It will be easier to extend the networking+metric features for gRPC in isolated environment and/or create extension not strictly depending on the js feature.

olegbespalov · 2022-04-21T10:33:53Z

js/modules/k6/grpc/client.go

 	state := c.vu.State()
 	if state == nil {
-		return nil, errInvokeRPCInInitContext
+		return nil, common.NewInitContextError("invoking RPC methods in the init context is not supported")


Suggested change

return nil, common.NewInitContextError("invoking RPC methods in the init context is not supported")

return nil, common.NewInitContextError("invoking gRPC methods in the init context is not supported")

Maybe use this? Just for consistency?

olegbespalov · 2022-04-21T11:49:44Z

js/modules/k6/grpc/client.go

@@ -353,49 +253,22 @@ func (c *Client) dial(
 	reflect bool,
 	options ...grpc.DialOption,
 ) error {


But it seems like the vast majority of the logic related to the reflection is still can be moved to a grpcext.

I belive we can even make a private a ReflectionClient. What js nodule needs are the *descriptorpb.FileDescriptorSet, so the grpcext.Conn can has a method that tries to load them. What do your think?

mstoykov

LGTM and IMO we can continue iterating in the next release on any of the currently commented stuff

codebien self-assigned this Apr 7, 2022

codebien force-pushed the grpcext branch 2 times, most recently from 83c999d to 12530dd Compare April 8, 2022 09:47

codebien marked this pull request as ready for review April 8, 2022 10:11

github-actions bot requested review from mstoykov and olegbespalov April 8, 2022 10:12

codebien requested a review from na-- April 8, 2022 13:02

na-- reviewed Apr 8, 2022

View reviewed changes

lib/netext/grpcext/conn.go Show resolved Hide resolved

na-- reviewed Apr 8, 2022

View reviewed changes

codebien added this to the v0.38.0 milestone Apr 8, 2022

codebien force-pushed the grpcext branch from 7e2e487 to 67ae006 Compare April 13, 2022 06:59

olegbespalov reviewed Apr 13, 2022

View reviewed changes

lib/netext/grpcext/conn.go Show resolved Hide resolved

Base automatically changed from grpc-connect-refactor to master April 13, 2022 14:12

cygnrh reviewed Apr 13, 2022

View reviewed changes

codebien added 2 commits April 14, 2022 15:15

lib/netext/grpcext: Split the gRPC module

0ad75c3

It splits the gRPC business logic from the js/k6/grpc module to a netext package. It will be easier to extend the networking+metric features for gRPC in isolated environment and/or create extension not strictly depending on the js feature.

Accept a variadic CallOption on Invoke

1f3bb9b

codebien force-pushed the grpcext branch from 67ae006 to 1f3bb9b Compare April 14, 2022 13:18

codebien added 2 commits April 14, 2022 15:31

Renamed method argument to url

33a13fc

Use Empty message for NoOp request

8b64a14

codebien requested a review from na-- April 19, 2022 08:14

na-- removed their request for review April 20, 2022 12:06

codebien requested a review from olegbespalov April 21, 2022 07:35

olegbespalov reviewed Apr 21, 2022

View reviewed changes

mstoykov approved these changes Apr 21, 2022

View reviewed changes

olegbespalov approved these changes Apr 21, 2022

View reviewed changes

codebien merged commit 187d336 into master Apr 21, 2022

codebien deleted the grpcext branch April 21, 2022 13:50

codebien mentioned this pull request Apr 21, 2022

Amended the gRPC module refactor #2501

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib/netext/grpcext: Split the gRPC module #2484

lib/netext/grpcext: Split the gRPC module #2484

codebien commented Apr 7, 2022 •

edited

Loading

na-- Apr 8, 2022 •

edited

Loading

codebien Apr 8, 2022 •

edited

Loading

na-- Apr 8, 2022

codebien Apr 8, 2022

olegbespalov Apr 21, 2022 •

edited

Loading

olegbespalov Apr 21, 2022

cygnrh Apr 13, 2022

codebien Apr 14, 2022

cygnrh May 6, 2022

cygnrh Apr 13, 2022

codebien Apr 14, 2022

cygnrh Apr 14, 2022

codebien Apr 15, 2022 •

edited

Loading

mstoykov Apr 21, 2022

cygnrh May 6, 2022

olegbespalov Apr 21, 2022

olegbespalov Apr 21, 2022 •

edited

Loading

mstoykov left a comment

		// TODO: should we support grpc.CallOption?
		func (c Conn) Invoke(ctx context.Context, method string, md metadata.MD, req Request) (Response, error) {

	return nil, common.NewInitContextError("invoking RPC methods in the init context is not supported")
	return nil, common.NewInitContextError("invoking gRPC methods in the init context is not supported")

lib/netext/grpcext: Split the gRPC module #2484

lib/netext/grpcext: Split the gRPC module #2484

Conversation

codebien commented Apr 7, 2022 • edited Loading

na-- Apr 8, 2022 • edited Loading

Choose a reason for hiding this comment

codebien Apr 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olegbespalov Apr 21, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codebien Apr 15, 2022 • edited Loading

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

olegbespalov Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

mstoykov left a comment

Choose a reason for hiding this comment

codebien commented Apr 7, 2022 •

edited

Loading

na-- Apr 8, 2022 •

edited

Loading

codebien Apr 8, 2022 •

edited

Loading

olegbespalov Apr 21, 2022 •

edited

Loading

codebien Apr 15, 2022 •

edited

Loading

olegbespalov Apr 21, 2022 •

edited

Loading