Google/protobuf/wrappers.proto support is not implemented in K6 #3238

zibul444 · 2023-07-28T16:29:34Z

What?

The GRPC response.message is empty

Why?

The response from the GRPC service, which uses google/protobuf/wrappers, is not processed correctly

Checklist

I have performed a self-review of my code.
I have added tests for my changes.
I have run linter locally (make ci-like-lint) and all checks pass.
I have run tests locally (make tests) and all tests pass.
I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

#3232

Yes, this closes the problem

#3232

zibul444 · 2023-07-29T10:08:14Z

I could not write a test because protoparce give an error when importing package "google/protobuf/wrappers.proto";. I do not know how to solve this, I can send the code of the test itself, I rewrote the test of course. But I could not overcome the protoparce error

zibul444 · 2023-08-07T06:31:45Z

test-current-cov
GoError: context deadline exceeded: connection error: desc = "transport: failed to write client preface: write tcp 127.0.0.1:53004->127.0.0.1:34205: write: connection reset by peer" at reflect.methodValueCall (native)" does not contain "remote error: tls: bad certificate
a you help my problem?

fabiogoshippo · 2023-08-12T16:33:05Z

I got a similar error

No gRPC response.

I am binding the service on Kubernetes cluster port forward

More info:

Any chances to get this sorted? No problems calling the service with postman

olegbespalov · 2023-08-18T12:32:27Z

@zibul444

I could not write a test because protoparce give an error when importing package "google/protobuf/wrappers.proto";. I do not know how to solve this, I can send the code of the test itself, I rewrote the test of course. But I could not overcome the protoparce error

What kind of error? Could you maybe commit the test (even if it's failing)

test-current-cov GoError: context deadline exceeded: connection error: desc = "transport: failed to write client preface: write tcp 127.0.0.1:53004->127.0.0.1:34205: write: connection reset by peer" at reflect.methodValueCall (native)" does not contain "remote error: tls: bad certificate
a you help my problem?

Unfortunately this is a flaky tests that is known, you could ignore this failure

olegbespalov · 2023-08-18T12:26:28Z

lib/netext/grpcext/conn.go

@@ -162,8 +162,11 @@ func (c *Conn) Invoke(
 		// {"x":6,"y":4,"z":0}
 		raw, _ := marshaler.Marshal(resp)
 		msg := make(map[string]interface{})
-		_ = json.Unmarshal(raw, &msg)
+		er := json.Unmarshal(raw, &msg)


I'm wondering if we could somehow identify that raw is a wrapped 🤔

Or, more precisely, I'm concerned that in case of Unmarshaling error, we safely assume that we can use raw

unfortunately, I can't tell you exactly how this can be done, but I will accept any help. I will assume that it is possible, but if you complicate the checks, it can greatly affect performance - this is also bad.

I find this fix ... strange at best and mostly hacky at this point.

I can't imagine that this is how it is supposed to work and also this will practically return you broken responses in case of real errors.

Additionally the lack of unit test for this means that I can't even easily check how this change at this point.

Can we please at least get a unit test for this.

@olegbespalov I guess you are trying to figure out how this is supposed to work actually ?

olegbespalov · 2023-08-18T12:36:51Z

@fabiogoshippo, from the error, it seems your problem is either authentication or a different one, and anyway, the pull request isn't the best place to discuss it.

Could you please open the topic (with all details and script without any business specific) on our community forums? We (or the community) will try to help you there.

Thanks.

zibul444 · 2023-08-23T09:42:57Z

@olegbespalov

Unfortunately this is a flaky tests that is known, you could ignore this failure

yes, without this error, all other tests work.

What kind of error? Could you maybe commit the test (even if it's failing)

The test crashes, because server emulation has not been performed, due to the lack of dependencies, maybe you can tell where exactly they are and how you can put them?
Error:
--- FAIL: TestInvoke (0.00s)
panic: panic handling "google/protobuf/wrappers.proto": runtime error: invalid memory address or nil pointer dereference [recovered]
panic: panic handling "google/protobuf/wrappers.proto": runtime error: invalid memory address or nil pointer dereference

zibul444 · 2023-08-23T09:45:35Z

@olegbespalov
I can send my test here, in the comments, I can push it - but then the check will get another failed unit test

olegbespalov · 2023-08-23T11:57:17Z

@zibul444 I mean, the end-goal is to have the test for the functionality, so even if it's failing, I guess it's OK to push it, I'll have a look at the code and maybe come up with the advice on how to resolve failure

mstoykov · 2023-08-24T08:09:21Z

lib/netext/grpcext/conn.go

@@ -162,8 +162,11 @@ func (c *Conn) Invoke(
 		// {"x":6,"y":4,"z":0}
 		raw, _ := marshaler.Marshal(resp)
 		msg := make(map[string]interface{})
-		_ = json.Unmarshal(raw, &msg)
+		er := json.Unmarshal(raw, &msg)


nit: you should be able to assign to err without := and without getting a complain about shadowing

I don't know what you're talking about . Changing the variable name solves the shading problem. The err variable already exists. What exactly did I do wrong?

You should just assign this to err instead of addng a new variable.

I don't se ay reason not to.

Added a test for the primitive in Test Invoke

codecov-commenter · 2023-08-28T12:29:25Z

Codecov Report

Merging #3238 (09b4dec) into master (3c006a5) will decrease coverage by 0.11%.
Report is 44 commits behind head on master.
The diff coverage is 33.33%.

❗ Current head 09b4dec differs from pull request most recent head 9aaf218. Consider uploading reports for the commit 9aaf218 to get more accurate results

@@            Coverage Diff             @@
##           master    #3238      +/-   ##
==========================================
- Coverage   73.21%   73.10%   -0.11%     
==========================================
  Files         259      257       -2     
  Lines       19897    19894       -3     
==========================================
- Hits        14568    14544      -24     
- Misses       4406     4421      +15     
- Partials      923      929       +6

Flag	Coverage Δ
ubuntu	`73.10% <33.33%> (-0.05%)`	⬇️
windows	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
lib/netext/grpcext/conn.go	`84.37% <33.33%> (-1.34%)`	⬇️

... and 12 files with indirect coverage changes

zibul444 · 2023-08-28T12:38:43Z

@olegbespalov

@zibul444 I mean, the end-goal is to have the test for the functionality, so even if it's failing, I guess it's OK to push it, I'll have a look at the code and maybe come up with the advice on how to resolve failure

I have added a test, but it is always with an error

--- FAIL: Test Invoke (0.00)
panic: panic handling "google/protobuf/wrappers.proto": runtime error: invalid memory address or null pointer dereference [recovered]
panic: panic handling "google/protobuf/wrappers.proto": runtime error: invalid memory address or nil pointer dereference

I do not know how to add import "google/protobuf/wrappers.proto";, I assume that files are required

mstoykov · 2023-08-28T12:40:02Z

The xk6 tests are failing due to #3252 can you please rebase on top of the current master so that this can be fixed.

Sorry for the inconvenience 🙇

olegbespalov · 2023-09-06T18:27:47Z

lib/netext/grpcext/conn.go

@@ -162,8 +162,11 @@ func (c *Conn) Invoke(
 		// {"x":6,"y":4,"z":0}
 		raw, _ := marshaler.Marshal(resp)
 		msg := make(map[string]interface{})


Okay, I've looked into this and believe that this line is an issue, switching to this should help

Suggested change

msg := make(map[string]interface{})

var msg interface{}

olegbespalov · 2023-09-06T18:31:31Z

lib/netext/grpcext/conn_test.go

@@ -24,26 +24,70 @@ import (
 )

 func TestInvoke(t *testing.T) {
-	t.Parallel()
+	ctx := context.Background()


I believe the https://github.com/grafana/k6/blob/master/js/modules/k6/grpc/client_test.go is the better test for testing this, and we might need to introduce one more test service, which could contain a different method to test the wrappers.

olegbespalov

Hi @zibul444, I might belive I found the root cause and as mentioned inline the fix is switching from the map[string]interface to just an interface.

But we need a good test coverage for that.

I'm going to open the PR to the xk6-grpc and depending on the pace we could process this PR.

olegbespalov · 2023-09-07T10:24:41Z

There is an example of he fix + tests for the unary parts of the xk6-grpc module. I still need to do a couple of more commits related to the streaming API grafana/xk6-grpc#49

olegbespalov · 2023-09-19T09:02:43Z

Hey @zibul444

The changes I mentioned earlier were merged into the xk6-grpc module, and the k6/experimental/grpc module should now support google wrappers. So, in theory, if you build the k6 from a master branch, the issue you've reported should be fixed.

Since we want to foster community contributions, I'd like to check if you plan to finalize this PR? If not, we could backport the changes by ourselves since we're close to the moment when we're planning what should the following k6 v0.47 release includes.

In any case, thank you for reporting the issue 🙇

olegbespalov · 2023-09-20T13:20:28Z

@zibul444 since we're wrapping the v0.47 we decided to close this PR in favor of #3344. Hope that works, thanks for contributing!

Update grpc conn.go, support google protobuf wrappers

3ac28a5

github-actions bot requested review from imiric and mstoykov July 28, 2023 16:29

olegbespalov added bug area: grpc labels Aug 7, 2023

mstoykov added this to the v0.47.0 milestone Aug 16, 2023

mstoykov requested review from olegbespalov and removed request for imiric August 16, 2023 13:04

olegbespalov reviewed Aug 18, 2023

View reviewed changes

mstoykov reviewed Aug 24, 2023

View reviewed changes

Update conn_test.go

9aaf218

Added a test for the primitive in Test Invoke

olegbespalov reviewed Sep 6, 2023

View reviewed changes

olegbespalov requested changes Sep 6, 2023

View reviewed changes

olegbespalov mentioned this pull request Sep 7, 2023

Support for the google wrappers grafana/xk6-grpc#49

Merged

olegbespalov mentioned this pull request Sep 20, 2023

gRPC's google wrappers support #3344

Merged

5 tasks

olegbespalov closed this in #3344 Sep 20, 2023

codebien removed this from the v0.47.0 milestone Oct 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Google/protobuf/wrappers.proto support is not implemented in K6 #3238

Google/protobuf/wrappers.proto support is not implemented in K6 #3238

zibul444 commented Jul 28, 2023 •

edited

Loading

zibul444 commented Jul 29, 2023 •

edited

Loading

zibul444 commented Aug 7, 2023

fabiogoshippo commented Aug 12, 2023 •

edited

Loading

olegbespalov commented Aug 18, 2023

olegbespalov Aug 18, 2023

zibul444 Aug 23, 2023

mstoykov Aug 24, 2023

olegbespalov commented Aug 18, 2023

zibul444 commented Aug 23, 2023

zibul444 commented Aug 23, 2023

olegbespalov commented Aug 23, 2023

mstoykov Aug 24, 2023

zibul444 Aug 28, 2023

mstoykov Aug 28, 2023

codecov-commenter commented Aug 28, 2023

zibul444 commented Aug 28, 2023

mstoykov commented Aug 28, 2023

olegbespalov Sep 6, 2023

olegbespalov Sep 6, 2023

olegbespalov left a comment

olegbespalov commented Sep 7, 2023

olegbespalov commented Sep 19, 2023

olegbespalov commented Sep 20, 2023

Google/protobuf/wrappers.proto support is not implemented in K6 #3238

Google/protobuf/wrappers.proto support is not implemented in K6 #3238

Conversation

zibul444 commented Jul 28, 2023 • edited Loading

What?

Why?

Checklist

Related PR(s)/Issue(s)

zibul444 commented Jul 29, 2023 • edited Loading

zibul444 commented Aug 7, 2023

fabiogoshippo commented Aug 12, 2023 • edited Loading

olegbespalov commented Aug 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olegbespalov commented Aug 18, 2023

zibul444 commented Aug 23, 2023

zibul444 commented Aug 23, 2023

olegbespalov commented Aug 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 28, 2023

Codecov Report

zibul444 commented Aug 28, 2023

mstoykov commented Aug 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olegbespalov left a comment

Choose a reason for hiding this comment

olegbespalov commented Sep 7, 2023

olegbespalov commented Sep 19, 2023

olegbespalov commented Sep 20, 2023

zibul444 commented Jul 28, 2023 •

edited

Loading

zibul444 commented Jul 29, 2023 •

edited

Loading

fabiogoshippo commented Aug 12, 2023 •

edited

Loading