Feature/grpc healthcheck #4853
Feature/grpc healthcheck #4853
Conversation
|
Hello 👋 I would like some feedback on the changes related to issue #3428. Cheers ! |
There was a problem hiding this comment.
Hey @tbourrely,
this is a great start! The general architecture seems fine to me. I've spotted a few details in comments. However, the unique major missing point is a unit test for the JavaScript API. You can find them here https://github.com/grafana/k6/blob/master/internal/js/modules/k6/grpc/client_test.go
examples/grpc_healthcheck.js
Outdated
| export default () => { | ||
| client.connect(GRPC_ADDR, { plaintext: true }); | ||
|
|
||
| const response = client.healthcheck("") |
There was a problem hiding this comment.
| const response = client.healthcheck("") | |
| const response = client.healthcheck() |
I would prefer if we support null instead of empty string for the default case.
examples/grpc_healthcheck.js
Outdated
| export default () => { | ||
| client.connect(GRPC_ADDR, { plaintext: true }); | ||
|
|
||
| const response = client.healthcheck("") |
There was a problem hiding this comment.
| const response = client.healthcheck("") | |
| const response = client.healthCheck("") |
I see you've originally proposed this name, that resonates with me because it matches the namespace of the API. Did you encounter any obstacle with it?
| @@ -278,6 +278,11 @@ func (c *Client) Connect(addr string, params sobek.Value) (bool, error) { | |||
| return true, err | |||
| } | |||
|
|
|||
| // Healthcheck runs healtcheck | |||
There was a problem hiding this comment.
| // Healthcheck runs healtcheck | |
| // Healthcheck checks if the server side is up and ready to serve responses. |
| @@ -86,6 +91,11 @@ func (mi *ModuleInstance) defineConstants() { | |||
| mustAddCode("StatusUnavailable", codes.Unavailable) | |||
| mustAddCode("StatusDataLoss", codes.DataLoss) | |||
| mustAddCode("StatusUnauthenticated", codes.Unauthenticated) | |||
|
|
|||
| mustAddServingStatus("HealthcheckUnknown", healthpb.HealthCheckResponse_UNKNOWN) | |||
There was a problem hiding this comment.
| mustAddServingStatus("HealthcheckUnknown", healthpb.HealthCheckResponse_UNKNOWN) | |
| mustAddServingStatus("HealthCheckUnknown", healthpb.HealthCheckResponse_UNKNOWN) |
It seems consistent with the current API. What is the case used on JavaScript-grpc library?
There was a problem hiding this comment.
| panic("not implemented") | ||
| } | ||
|
|
||
| func TestHealthcheck(t *testing.T) { |
There was a problem hiding this comment.
| func TestHealthcheck(t *testing.T) { | |
| func TestHealthCheckServing(t *testing.T) { |
We are missing a not serving case.
There was a problem hiding this comment.
ive also added the unknown case
05f2d36 to
225969a
Compare
|
Thanks for the review @codebien 👍 |
|
I've added the JS client tests. |
What?
Add a healthcheck method to gRPC JS client.
Why?
Remove invoke boilerplate as specified in #3428
Checklist
make check) and all pass.Related PR(s)/Issue(s)
Closes #3428