-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Expose grpc_health_v1 descriptor set in tonic-health for use with the reflection service #620
Expose grpc_health_v1 descriptor set in tonic-health for use with the reflection service #620
Conversation
I'm not very familiar with tonic-health. @jen20 are you up for taking a look at this? |
@@ -28,6 +28,10 @@ pub mod proto { | |||
#![allow(unreachable_pub)] | |||
#![allow(missing_docs)] | |||
tonic::include_proto!("grpc.health.v1"); | |||
|
|||
#[allow(dead_code)] | |||
pub const GRPC_HEALTH_V1_FILE_DESCRIPTOR_SET: &'static [u8] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to include a test here maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do that. It will be a few days before I have the time, but I will get that worked in.
@byblakeorriver I think we can merge this if you want to add that one test? |
@LucioFranco is the test really necessary? After all it's just including something generated by |
Sorry I have been swamped. I am not sure when I would be able to get that test written. I am okay with waiting until I can do that. |
fn main() -> Result<(), Box<dyn std::error::Error>> { | ||
let grpc_health_v1_descriptor_set_path: PathBuf = | ||
PathBuf::from(env::var("OUT_DIR").unwrap()).join("grpc_health_v1.bin"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OUT_DIR might not be set except in the build environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure it is set when build.rs is ran.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OUT_DIR — If the package has a build script, this is set to the folder where the build script should place its output. See below for more information. (Only set during compilation.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But, if you want, I can add a check instead of using .unwrap() here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only question in that case would be, what behavior would you expect if the OUT_DIR didn't exist?
Would there just be an error saying that the OUT_DIR wasn't set and not produce the descriptor set?
What would say about this @LucioFranco?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can trust that OUT_DIR
will be there, because this script is also compiling the proto I think its safe.
Thanks @LucioFranco!!! |
Motivation
Fixes #619
When using Kubernetes a readiness and liveness check should be defined for a deployment. This is usually done by having an endpoint to hit that will return the serving status. For gRPC there are some common tools used to hit these endpoints (grpcurl or grpc_health_probe). These tools require either a .protoset file for the service that they are going to interact with, or they require the service to have a reflection service registered. The health check is usually handled by hitting the endpoint of the grpc health service to see if it is serving or not. In order to register the health service with the reflection service using the tonic-reflection crate you have to have the file descriptor set binaries of the health.proto file. It seems appropriate for this to be produced by the tonic-health crate.
Solution
In order to make this work, I just needed to create the file descriptor binaries in the build.rs file and then expose it in the proto mod of the lib.rs file.