client: Add error log for missing health package import during health check

[arjan-bal]

Addresses: #8590

When using the health producer for health checks, and the health package is not imported by the application, a no op health producer is used without logging any errors. This PR adds an error log similar to the one for the old health checks started by the subchannel.

grpc-go/clientconn.go

Lines 1475 to 1481 in e350804

	if healthCheckFunc == nil {
	// The health package is not imported to set health check function.
	//
	// TODO: add a link to the health check doc in the error message.
	channelz.Error(logger, ac.channelz, "Health check is requested but health check function is not set.")
	return
	}

RELEASE NOTES: N/A

[codecov]

Codecov Report

❌ Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.05%. Comparing base (e350804) to head (02d971e).
⚠️ Report is 21 commits behind head on master.

Files with missing lines	Patch %	Lines
balancer_wrapper.go	60.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8595      +/-   ##
==========================================
+ Coverage   81.89%   82.05%   +0.15%     
==========================================
  Files         415      415              
  Lines       40694    40701       +7     
==========================================
+ Hits        33328    33397      +69     
+ Misses       5976     5923      -53     
+ Partials     1390     1381       -9     

Files with missing lines	Coverage Δ
balancer_wrapper.go	81.00% <60.00%> (-4.67%)	⬇️

... and 28 files with indirect coverage changes

🚀 New features to boost your workflow:

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

[evanj]

This would be very helpful, thank!

I mentioned this on my issue #8590 since I missed the magic import: Could we consider making the magic import unnecessary? I assume we need this indirection to reduce dependencies of the core gRPC library or something like that.

[easwars]

The client-side health checking spec has the following: https://github.com/grpc/proposal/blob/master/A17-client-side-health-checking.md#client-behavior. Specifically:

Client-side health checking will be disabled by default; service owners will need to 
explicitly enable it via the [service config]
(https://github.com/grpc/proposal/blob/master/A17-client-side-health-checking.md#service-config-changes) 
when desired. There will be a channel argument that can be used on the client to 
disable health checking even if it is enabled in the service config.

Our implementation does ensure that we enable health checking only when explicitly enabled via the service config. Our implementation also provides a dial option to disable health checks: https://pkg.go.dev/google.golang.org/grpc#WithDisableHealthCheck

So, I don't see any reason why we should also force the user to blank import the health package to get client-side health checking. If we remove this restriction though, it would be a breaking change.

@dfawley : What do you think? The error log being added does tell the user to import the health package, but it still seems unnecessary to have the user do that.

[easwars]

I just realized that I had this review comment as a draft, but had forgotten to hit enter.

I now remember that @arjan-bal mentioned that one possibility for explicitly asking users to blank import the health package is to avoid taking a proto dependency. Let's wait on @dfawley to decide on this.

[evanj]

Hmm but doesn't grpc-go already depend on proto? In particular, the root package imports status, eg to call status.Error(). status imports google.golang.org/genproto/googleapis/rpc/status, which is a protobuf generated file: https://github.com/googleapis/go-genproto/blob/main/googleapis/rpc/status/status.pb.go

[dfawley]

Yes, the reason for this was to avoid the health proto dependency. We already depend on proto, and have done so since before 1.0, because the proto codec is included by default (as is the status proto).

How big is the health proto dependency itself?

[dfawley]

Also I think this change is fine as-is. If we want to remove the requirement to import the health package manually, that can be separate.