Add List method to gRPC Health service #8155

marcoshuck · 2025-03-08T15:33:12Z

This change introduces a new List RPC endpoint for the Health service, allowing clients to retrieve the statuses of all monitored services. This feature simplifies integration with status-reporting dashboards and enhances observability for microservices.

Proposal: grpc/proposal#468
gRPC-proto change: grpc/grpc-proto#143

This change introduces a new `List` RPC endpoint for the Health service, allowing clients to retrieve the statuses of all monitored services. This feature simplifies integration with status-reporting dashboards and enhances observability for microservices. Proposal: grpc/proposal#468 gRPC-proto change: grpc/grpc-proto#143 Signed-off-by: Marcos Huck <marcos@huck.com.ar>

marcoshuck · 2025-03-08T15:36:56Z

health/grpc_health_v1/health.pb.go

I regenerated the Go stubs using the work in progress version of grpc/grpc-proto#143

marcoshuck · 2025-03-08T15:37:02Z

health/grpc_health_v1/health_grpc.pb.go

I regenerated the Go stubs using the work in progress version of grpc/grpc-proto#143

codecov · 2025-03-08T15:39:57Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.02%. Comparing base (732f3f3) to head (8a7e2a3).
Report is 16 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8155      +/-   ##
==========================================
- Coverage   82.09%   82.02%   -0.07%     
==========================================
  Files         412      412              
  Lines       40491    40503      +12     
==========================================
- Hits        33242    33224      -18     
- Misses       5876     5896      +20     
- Partials     1373     1383      +10

Files with missing lines	Coverage Δ
health/server.go	`95.65% <100.00%> (+0.65%)`	⬆️

... and 19 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

purnesh42H

@marcoshuck i think we need to wait for the proposal and proto change to be merged before this? I still see them in review

@dfawley cc

health/server.go

health/server_internal_test.go

Signed-off-by: Marcos Huck <marcos@huck.com.ar>

github-actions · 2025-03-19T22:45:42Z

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

health/server.go

health/server_internal_test.go

health/server.go

Signed-off-by: Marcos Huck <marcos@huck.com.ar>

Co-authored-by: Purnesh Dixit <purnesh.dixit92@gmail.com>

Signed-off-by: Marcos Huck <marcos@huck.com.ar>

purnesh42H · 2025-03-28T19:00:18Z

@marcoshuck could you resolve the merge conflicts?

health/server.go

health/server_internal_test.go

purnesh42H · 2025-03-28T19:07:03Z

health/server_internal_test.go

+	}
+}
+
+// TestListResourceExhausted verifies that the service status list returns an error when it exceeds


nit: restrict the comments to 80 cols

health/server_internal_test.go

Signed-off-by: Marcos Huck <marcos@huck.com.ar>

purnesh42H

lgtm. Please wrap comments under 80 cols per line.

purnesh42H · 2025-04-14T05:51:25Z

health/server.go

@@ -30,6 +30,12 @@ import (
 	"google.golang.org/grpc/status"
 )

+const (
+	// maxAllowedServices defines the maximum number of resources a List operation can return.


nit: wrap comments to 80 cols per line.

purnesh42H · 2025-04-14T05:52:57Z

health/server_internal_test.go

+	if len(out.GetStatuses()) != len(s.statusMap) {
+		t.Fatalf("len(out.GetStatuses()) = %d, want %d", len(out.GetStatuses()), len(s.statusMap))
+	}
+	for key := range out.GetStatuses() {


nit: for k, v := range out.GetStatuses() then don't need line 115

purnesh42H · 2025-04-14T06:19:03Z

health/server_internal_test.go

+		t.Fatalf("s.List(ctx, &in) return nil error, want non-nil")
+	}
+	if !errors.Is(err, status.Errorf(codes.ResourceExhausted, "server health list exceeds maximum capacity: %d", maxAllowedServices)) {
+		t.Fatal("List should have failed with resource exhausted")


Suggested change

t.Fatal("List should have failed with resource exhausted")

t.Fatal("s.List() = %v, want %v, err, status.Errorf(codes.ResourceExhausted, "server health list exceeds maximum capacity: %d", maxAllowedServices)")

Nit: don't spell status.Errorf.... twice. Assign to a want instead.

purnesh42H · 2025-04-14T06:25:18Z

Could you run ./scripts/vet.sh to catch vet errors and fix them?

dfawley

Just some comments about the tests. Otherwise LGTM, thanks!

dfawley · 2025-04-16T21:19:26Z

health/server_internal_test.go

+	// Remove the zero value
+	delete(s.statusMap, "")


What is this for?

dfawley · 2025-04-16T21:19:56Z

health/server_internal_test.go

+	delete(s.statusMap, "")
+	// Fill out status map with information
+	for i := 1; i <= 3; i++ {
+		s.statusMap[fmt.Sprintf("%d", i)] = healthpb.HealthCheckResponse_SERVING


Can this use s.SetServingStatus instead, if possible?

dfawley · 2025-04-16T21:20:18Z

health/server_internal_test.go

+	if err != nil {
+		t.Fatalf("s.List(ctx, &in) returned err %v, want nil", err)
+	}
+	if len(out.GetStatuses()) != len(s.statusMap) {


Why len(s.statusMap)? We added 3 things, so we should know it's 3?

dfawley · 2025-04-16T21:24:34Z

health/server_internal_test.go

+	for key := range out.GetStatuses() {
+		v, ok := s.statusMap[key]
+		if !ok {
+			t.Fatalf("key %s does not exist in s.statusMap", key)
+		}
+		if v != healthpb.HealthCheckResponse_SERVING {
+			t.Fatalf("%s returned status %d, want %d", key, healthpb.HealthCheckResponse_SERVING, v)
+		}
+	}


Please replace this with something that confirms it is what we expect it to be statically, not what exists in s.statusMap dynamically. I.e.

want := healthpb.HealthListRequest{ Statuses: map[string]*healthpb.HealthCheckResponse{ "1": healthpb.HealthCheckResponse_SERVING.Enum(), // etc } } if diff := cmp.Diff(got, want); diff != "" { t.Fatalf("....: %s", diff) }

dfawley · 2025-04-16T21:24:49Z

health/server_internal_test.go

+	// Remove the zero value
+	delete(s.statusMap, "")


dfawley · 2025-04-16T21:25:07Z

health/server_internal_test.go

+	for i := 1; i <= maxAllowedServices+1; i++ {
+		s.statusMap[fmt.Sprintf("%d", i)] = healthpb.HealthCheckResponse_SERVING
+	}
+	s.mu.Unlock()


Also as above: please use s.SetServingStatus

dfawley · 2025-04-16T21:25:40Z

health/server_internal_test.go

+	if err == nil {
+		t.Fatalf("s.List(ctx, &in) return nil error, want non-nil")
+	}


This is unnecessary since you check the error code next anyway.

dfawley · 2025-04-16T21:26:25Z

health/server_internal_test.go

+		t.Fatalf("s.List(ctx, &in) return nil error, want non-nil")
+	}
+	if !errors.Is(err, status.Errorf(codes.ResourceExhausted, "server health list exceeds maximum capacity: %d", maxAllowedServices)) {
+		t.Fatal("List should have failed with resource exhausted")


Nit: don't spell status.Errorf.... twice. Assign to a want instead.

github-actions · 2025-04-22T22:46:01Z

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

marcoshuck commented Mar 8, 2025

View reviewed changes

health/grpc_health_v1/health.pb.go Outdated

Copy link

Author

marcoshuck Mar 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I regenerated the Go stubs using the work in progress version of grpc/grpc-proto#143

marcoshuck commented Mar 8, 2025

View reviewed changes

health/grpc_health_v1/health_grpc.pb.go Outdated

Copy link

Author

marcoshuck Mar 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I regenerated the Go stubs using the work in progress version of grpc/grpc-proto#143

purnesh42H reviewed Mar 10, 2025

View reviewed changes

purnesh42H assigned marcoshuck Mar 10, 2025

purnesh42H added Area: Observability Includes Stats, Tracing, Channelz, Healthz, Binlog, Reflection, Admin, GCP Observability Type: Feature New features or improvements in behavior Status: Requires Reporter Clarification labels Mar 10, 2025

marcoshuck mentioned this pull request Mar 10, 2025

A90: Add List Method to gRPC Health Service grpc/proposal#468

Merged

Address comments from code review

23a4f3a

Signed-off-by: Marcos Huck <marcos@huck.com.ar>

github-actions bot added the stale label Mar 19, 2025

marcoshuck requested a review from purnesh42H March 19, 2025 23:07

github-actions bot removed the stale label Mar 20, 2025

dfawley removed the Status: Requires Reporter Clarification label Mar 20, 2025

dfawley assigned purnesh42H and unassigned marcoshuck Mar 20, 2025

dfawley added Area: Observability Includes Stats, Tracing, Channelz, Healthz, Binlog, Reflection, Admin, GCP Observability and removed Area: Observability Includes Stats, Tracing, Channelz, Healthz, Binlog, Reflection, Admin, GCP Observability labels Mar 20, 2025

purnesh42H reviewed Mar 24, 2025

View reviewed changes

purnesh42H assigned marcoshuck and unassigned purnesh42H Mar 24, 2025

marcoshuck and others added 3 commits March 24, 2025 12:54

Address comments from code review

6e62ea4

Signed-off-by: Marcos Huck <marcos@huck.com.ar>

Apply suggestions from code review

500bc52

Co-authored-by: Purnesh Dixit <purnesh.dixit92@gmail.com>

Fix Fatalf usage

174617c

Signed-off-by: Marcos Huck <marcos@huck.com.ar>

marcoshuck requested a review from purnesh42H March 24, 2025 15:57

purnesh42H reviewed Mar 28, 2025

View reviewed changes

Merge branch 'master' into health_list

dfe4a8e

Address comments from code review

8a7e2a3

Signed-off-by: Marcos Huck <marcos@huck.com.ar>

arjan-bal added the Status: Requires Reporter Clarification label Apr 14, 2025

purnesh42H approved these changes Apr 14, 2025

View reviewed changes

purnesh42H assigned dfawley Apr 14, 2025

dfawley approved these changes Apr 16, 2025

View reviewed changes

dfawley removed their assignment Apr 16, 2025

github-actions bot added the stale label Apr 22, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add List method to gRPC Health service #8155

Add List method to gRPC Health service #8155

marcoshuck commented Mar 8, 2025

marcoshuck Mar 8, 2025

marcoshuck Mar 8, 2025

codecov bot commented Mar 8, 2025 •

edited

Loading

purnesh42H left a comment

github-actions bot commented Mar 19, 2025

purnesh42H commented Mar 28, 2025

purnesh42H Mar 28, 2025

purnesh42H left a comment

purnesh42H Apr 14, 2025

purnesh42H Apr 14, 2025

purnesh42H Apr 14, 2025

dfawley Apr 16, 2025

purnesh42H commented Apr 14, 2025

dfawley left a comment

dfawley Apr 16, 2025

dfawley Apr 16, 2025

dfawley Apr 16, 2025

dfawley Apr 16, 2025

dfawley Apr 16, 2025

dfawley Apr 16, 2025

dfawley Apr 16, 2025

dfawley Apr 16, 2025

github-actions bot commented Apr 22, 2025

	t.Fatal("List should have failed with resource exhausted")
	t.Fatal("s.List() = %v, want %v, err, status.Errorf(codes.ResourceExhausted, "server health list exceeds maximum capacity: %d", maxAllowedServices)")

Add List method to gRPC Health service #8155

Are you sure you want to change the base?

Add List method to gRPC Health service #8155

Conversation

marcoshuck commented Mar 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 8, 2025 • edited Loading

Codecov Report

purnesh42H left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 19, 2025

purnesh42H commented Mar 28, 2025

Choose a reason for hiding this comment

purnesh42H 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

purnesh42H commented Apr 14, 2025

dfawley 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

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

github-actions bot commented Apr 22, 2025

codecov bot commented Mar 8, 2025 •

edited

Loading