[Inspection Service] Add simple consensus health check endpoint. #15512
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
⏱️ 1h 54m total CI duration on this PR
🚨 1 job on the last run was significantly faster/slower than expected
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Comment on lines
37
to
39
| if gauge_value == "0" { | ||
| return ( | ||
| StatusCode::OK, | ||
| Body::from("Consensus health check passed!"), | ||
| CONTENT_TYPE_TEXT.into(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
The health check logic appears to be reversed. The code returns OK when gauge_value == "0", indicating consensus is not executing. However, based on the function's documented purpose of checking if "the node is currently participating in consensus", it should return OK when gauge_value == "1". This would align with the gauge's behavior set in update_executing_component_metrics() where 1 indicates active consensus participation.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
JoshLind
force-pushed
the
in_consensus_probe
branch
from
6c1c559 to
67569b0
Compare
| use hyper::{Body, StatusCode}; | ||
| use prometheus::TextEncoder; | ||
|
|
||
| // The metric key for the consensus execution gauge | ||
| const CONSENSUS_EXECUTION_GAUGE: &str = "aptos_state_sync_consensus_executing_gauge{}"; |
There was a problem hiding this comment.
The metric name aptos_state_sync_consensus_executing_gauge{} includes empty curly braces that should be removed since this gauge doesn't use any labels. To match the registered metric name in the code below, this should be aptos_state_sync_consensus_executing_gauge.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
JoshLind
force-pushed
the
in_consensus_probe
branch
from
67569b0 to
ea3a5b9
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
geekflyer
reviewed
Dec 6, 2024
| const CONSENSUS_EXECUTION_GAUGE: &str = "aptos_state_sync_consensus_executing_gauge{}"; | ||
|
|
||
| /// Handles a consensus health check request. This method returns | ||
| /// 200 iff the node is currently participating in consensus. |
There was a problem hiding this comment.
Suggested change
| /// 200 iff the node is currently participating in consensus. | |
| /// 200 if the node is currently participating in consensus. |
There was a problem hiding this comment.
Aah, this was meant to be "if and only if" 😄 https://www.merriam-webster.com/dictionary/iff
There was a problem hiding this comment.
Aah, this was meant to be "if and only if" 😄 merriam-webster.com/dictionary/iff
I recommend simply using if and only if in that case. I doubt that even half of our eng team knows the iff thingy.
There was a problem hiding this comment.
SGTM. Changed it to if. Don't feel as strongly as you do 😄
geekflyer
approved these changes
Dec 6, 2024
JoshLind
force-pushed
the
in_consensus_probe
branch
from
ea3a5b9 to
bf4f448
Compare
zekun000
approved these changes
Dec 10, 2024
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR adds a simple consensus health check endpoint to the node inspection service. Specifically, the new endpoint
/consensus_health_checkwill return a 200 status code iff the node is currently executing consensus.To achieve this, I added a new metric gauge that is set iff consensus is executing on the validator node. The inspection service simply fetches the value of this gauge. The gauge can be seen here.
Testing Plan
Existing test infrastructure, and manual verification, e.g., I ran a local validator and pinged the endpoint: