Feature/grpc healthcheck by tbourrely · Pull Request #4853 · grafana/k6

tbourrely · 2025-06-15T23:05:22Z

What?

Add a healthcheck method to gRPC JS client.

Why?

Remove invoke boilerplate as specified in #3428

Checklist

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

Related PR(s)/Issue(s)

Closes #3428

tbourrely · 2025-06-15T23:09:04Z

Hello 👋

I would like some feedback on the changes related to issue #3428.
Please let me know if I'm going in the wrong direction.

Cheers !

codebien

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

codebien · 2025-06-18T13:22:42Z

examples/grpc_healthcheck.js

Suggested change

const response = client.healthcheck("")

const response = client.healthcheck()

I would prefer if we support null instead of empty string for the default case.

codebien · 2025-06-18T13:25:57Z

examples/grpc_healthcheck.js

Suggested change

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?

none at all :)

codebien · 2025-06-18T13:27:51Z

internal/js/modules/k6/grpc/client.go

Suggested change

// Healthcheck runs healtcheck

// Healthcheck checks if the server side is up and ready to serve responses.

codebien · 2025-06-18T13:29:35Z

internal/js/modules/k6/grpc/grpc.go

Suggested change

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?

Are you referring to this https://github.com/grpc/grpc-node/blob/master/packages/grpc-health-check/src/generated/grpc/health/v1/HealthCheckResponse.ts#L6 ?

HealthCheck is more consistent with the current method.

codebien · 2025-06-18T13:31:17Z

internal/lib/netext/grpcext/conn_test.go

Suggested change

func TestHealthcheck(t *testing.T) {

func TestHealthCheckServing(t *testing.T) {

We are missing a not serving case.

ive also added the unknown case

tbourrely · 2025-06-20T16:05:51Z

Thanks for the review @codebien 👍
I'll address the js test concerns asap !

tbourrely · 2025-06-22T12:09:47Z

I've added the JS client tests.
I was unsure whether i should put the healthcheck server on the HTTPMultiBin struct or not 🤔

codebien · 2025-06-25T09:59:09Z

vendor/modules.txt

It shouldn't happen here, can you rollback it, please? If this is a requirement, then we need a dedicated pull request for it.

codebien · 2025-06-25T12:26:08Z

internal/js/modules/k6/grpc/grpc.go

HealthCheck is more consistent with the current method.

codebien · 2025-06-25T12:29:32Z

internal/lib/netext/grpcext/conn_test.go

Suggested change

res, err := c.HealthCheck(context.Background(), "")

res, err := c.HealthCheck(context.Background(), "HERE-SERVICE")

We're missing a case where we test a specific service

codebien · 2025-06-25T12:31:25Z

@tbourrely can you rebase and resolve the conflicts, please?

Closes grafana#3428

ankur22

LGTM 🚀

codebien

LGTM 🎉 @tbourrely thanks for the contribution 🙇

As soon as we have resolved the linter issue, we're working internally on it, we will merge this pull request.

tbourrely requested a review from a team as a code owner June 15, 2025 23:05

tbourrely requested review from ankur22 and codebien and removed request for a team June 15, 2025 23:05

joanlopez added this to the v1.2.0 milestone Jun 16, 2025

codebien requested changes Jun 18, 2025

View reviewed changes

tbourrely force-pushed the feature/grpc-healthcheck branch from 05f2d36 to 225969a Compare June 20, 2025 15:34

codebien requested changes Jun 25, 2025

View reviewed changes

tbourrely force-pushed the feature/grpc-healthcheck branch from 886dc73 to ca96658 Compare June 26, 2025 23:20

tbourrely closed this Jun 26, 2025

tbourrely force-pushed the feature/grpc-healthcheck branch from ca96658 to 0e3fb95 Compare June 26, 2025 23:21

tbourrely reopened this Jun 26, 2025

tbourrely force-pushed the feature/grpc-healthcheck branch from 68ec422 to b054618 Compare July 5, 2025 20:39

tbourrely added 2 commits July 5, 2025 22:40

Update grpc examples default proto path

796c852

feat: add healthcheck to grpc client

129be31

Closes grafana#3428

tbourrely force-pushed the feature/grpc-healthcheck branch from b054618 to 129be31 Compare July 5, 2025 20:40

codebien self-requested a review July 15, 2025 13:55

Merge branch 'master' into feature/grpc-healthcheck

2363b78

ankur22 approved these changes Jul 17, 2025

View reviewed changes

codebien mentioned this pull request Jul 22, 2025

Copy zizmor config in the repo #4959

Merged

8 tasks

Merge branch 'master' into feature/grpc-healthcheck

f41dbe9

codebien approved these changes Jul 22, 2025

View reviewed changes

Merge branch 'master' into feature/grpc-healthcheck

e16cb96

codebien merged commit 602467d into grafana:master Jul 25, 2025
47 of 48 checks passed

mstoykov added the documentation-needed A PR which will need a separate PR for documentation label Aug 4, 2025

codebien mentioned this pull request Aug 8, 2025

Added types for grpc.healthCheck grafana/k6-DefinitelyTyped#91

Merged

BrewTestBot mentioned this pull request Aug 13, 2025

k6 1.2.1 Homebrew/homebrew-core#233339

Merged

	const response = client.healthcheck("")
	const response = client.healthcheck()

	// Healthcheck runs healtcheck
	// Healthcheck checks if the server side is up and ready to serve responses.

	mustAddServingStatus("HealthcheckUnknown", healthpb.HealthCheckResponse_UNKNOWN)
	mustAddServingStatus("HealthCheckUnknown", healthpb.HealthCheckResponse_UNKNOWN)

	func TestHealthcheck(t *testing.T) {
	func TestHealthCheckServing(t *testing.T) {

	res, err := c.HealthCheck(context.Background(), "")
	res, err := c.HealthCheck(context.Background(), "HERE-SERVICE")

Conversation

tbourrely commented Jun 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What?

Why?

Checklist

Related PR(s)/Issue(s)

Uh oh!

tbourrely commented Jun 15, 2025

Uh oh!

codebien left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codebien Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbourrely commented Jun 20, 2025

Uh oh!

tbourrely commented Jun 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codebien commented Jun 25, 2025

Uh oh!

ankur22 left a comment

Choose a reason for hiding this comment

Uh oh!

codebien left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tbourrely commented Jun 15, 2025 •

edited

Loading

codebien left a comment •

edited

Loading

codebien Jun 18, 2025 •

edited

Loading