Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(health): Correctly implement spec for overall health #897

Merged
merged 2 commits into from
Jan 24, 2022

Conversation

snowpoke
Copy link
Contributor

This PR adds a service of empty name ("") to each Health Reporter, which will always return a SERVING status.

Motivation

The description of the GRPC Health Checking Protocol includes the following line:

The server should use an empty string as the key for server's overall health status, so that a client not interested in a specific service can query the server's status with an empty request.
(https://github.com/grpc/grpc/blob/master/doc/health-checking.md)

At the moment, this behavior is not implemented in the tonic-health crate. In particular, this can cause problems when running health checkers in their default settings.
For example, if you try to run Google's GRPC Health Probe script (https://github.com/grpc-ecosystem/grpc-health-probe) like described in the example, it will cause an unexpected error:

error: health rpc failed: rpc error: code = NotFound desc = service not registered

The expected behavior would be to return status: SERVING.

Solution

My PR changes HealthReporter::new such that on creation, we register a service "" and associate it with ServingStatus::Serving. Instead of HashMap::new(), a HashMap based on this service is used when the HealthReporter is initiated.
I also expanded the tests to check for this particular behavior.

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

Good catch! Thanks for the PR!

@LucioFranco LucioFranco changed the title tonic-health: Implement overall server health as described in protocol standard fix(health): Correctly implement spec for overall health Jan 24, 2022
@LucioFranco LucioFranco merged commit 2b0ffee into hyperium:master Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants