Add /metrics endpoint#99
Conversation
We now read `vss.proto` from the local repository instead of dowloading it from a previous commit on github. We also update the checked-in `types.rs` file to match the one generated with the `genproto` flag, and now skip any formatting of this auto-generated file.
This endpoint allows clients to check the version of the VSS API that the server is serving. It also serves as a health check.
|
👋 Thanks for assigning @tnull as a reviewer! |
| pub mod kv_store; | ||
|
|
||
| /// Contains request/response types generated from the API definition of VSS. | ||
| #[rustfmt::skip] |
There was a problem hiding this comment.
Why skip rather than formatting post-generation?
There was a problem hiding this comment.
So that we don't need to run format post-generation ? Happy to drop if you prefer.
There was a problem hiding this comment.
Yeah, it's always a debate whether to format or not format auto-generated code. I always lean towards yes, others towards no.
In the end it doesn't matter much, so I'd say I'd leave it up to you / your preference. Feel free to leave as-is.
There was a problem hiding this comment.
See below, I removed the skip and now format this file too.
| #[cfg(genproto)] | ||
| fn generate_protos() { | ||
| download_file( | ||
| "https://raw.githubusercontent.com/lightningdevkit/vss-server/7f492fcac0c561b212f49ca40f7d16075822440f/app/src/main/proto/vss.proto", |
There was a problem hiding this comment.
AFAIU, the original intention here was to pin the protocol version to a specific commit. But you're right that we probably should do that via the new protocol versioning, and proper vss-server tags/releases.
|
|
||
| [target.'cfg(genproto)'.build-dependencies] | ||
| prost-build = { version = "0.11.3" } | ||
| reqwest = { version = "0.11.13", default-features = false, features = ["rustls-tls", "blocking"] } |
| let prefix_stripped_path = path.strip_prefix(BASE_PATH_PREFIX).unwrap_or_default(); | ||
|
|
||
| match prefix_stripped_path { | ||
| "/version" => { |
There was a problem hiding this comment.
Adding this endpoint is great, but shouldn't we also add a (I guess optional, for backwards compat) protocol_version field to all other messages? Otherwise, if we require the client to always manually first call this endpoint and wait on the response a) it's error prone b) it would require another full RTT on startup which is not great. So maybe we make version negotiation implicit and only fail the flow on mismatch? Thoughts?
There was a problem hiding this comment.
See below, I add a vss-protocol-version header to every response, clients can read the protocol version from any response they get back from the server.
There was a problem hiding this comment.
Cool. Now that we did this, maybe the endpoint could be a more general /health or /status endpoint? Or maybe this shouldn't even be a VSS-style proto response but rather a prometheus-compatible endpoint then?
There was a problem hiding this comment.
See below, I remove /version, and add /metrics, which offers a prometheus-style gauge for now
3 participants
Compatible with prometheus