feat(zksync_cli): Health checkpoint improvements #3193
Conversation
core/node/node_framework/src/implementations/layers/house_keeper.rs
Outdated
Show resolved
Hide resolved
core/node/node_framework/src/implementations/layers/house_keeper.rs
Outdated
Show resolved
Hide resolved
| pub prev_l2_block_hash: H256, | ||
| pub prev_l2_block_timestamp: u64, |
There was a problem hiding this comment.
Bikeshedding: Do these 2 fields provide valuable info? IMO, it'd make more sense to provide info like the number of txs in the current block.
There was a problem hiding this comment.
the number of txs in the current block
I don't feel this info is somehow interesting. We have normally 2-3 tps so we have to update it pretty frequent
core/node/node_framework/src/implementations/layers/state_keeper/mod.rs
Outdated
Show resolved
Hide resolved
| pub installed_on: DateTime<chrono::Utc>, | ||
| pub success: bool, | ||
| pub checksum: String, | ||
| pub execution_time: i64, |
There was a problem hiding this comment.
Nit: The type of this field looks weird. If it's a duration, can it be converted to std::time::Duration? Alternatively, it may make sense to add a suffix (like _ms) to the field name and convert it correspondingly.
| #[derive(Debug, Serialize, Deserialize)] | ||
| pub struct EthTxManagerHealthDetails { | ||
| pub last_mined_tx: EthTxDetails, | ||
| pub tx_status: TxStatus, |
There was a problem hiding this comment.
Is that last_mined_tx_status? If so, please rename the field (or combine it with last_mined_tx by adding). Also, just to be clear: Is it intentional the health for EthTxManager is updated in a loop for each transaction in inflight_txs? Is the ordering of inflight_txs correct to talk about "last mined transaction"?
| const SCRAPE_INTERVAL: Duration = Duration::from_secs(60); | ||
|
|
||
| #[derive(Debug, Deserialize, Clone, PartialEq)] | ||
| pub struct Config { |
There was a problem hiding this comment.
This config looks redundant; it's not used and is always initialized to a default value. Also, you can probably reuse SCRAPE_INTERVAL for the new task.
| pub use values::BIN_METADATA; | ||
| use zksync_health_check::{CheckHealth, Health, HealthStatus}; |
There was a problem hiding this comment.
- Please don't split
uses withmoddeclarations; it'll lead to bogus import grouping (like herezksync_health_checkbelongs to external imports). - Please prepend
self::for internal imports so that they are easy to distinguish from external ones.
| /// Tries to run the command, only returns `Some` if the command | ||
| /// succeeded and the output was valid utf8. | ||
| fn run_cmd(cmd: &str, args: &[&str]) -> String { | ||
| run_cmd_opt(cmd, args).unwrap_or("unknown".to_string()) |
There was a problem hiding this comment.
Bikeshedding: Is "unknown" a better value than no value at all (i.e., None)? IMO, the latter is more idiomatic, and it fits better at least for metrics.
|
|
||
| impl From<&BinMetadata> for Health { | ||
| fn from(details: &BinMetadata) -> Self { | ||
| Self::from(HealthStatus::Ready).with_details(details) |
There was a problem hiding this comment.
Future work (?): It's weird seeing this reported as a component, since this data clearly doesn't have its own lifecycle (not even a sort of surrogate one like Postgres does). It'd probably make sense to add a way to set details for AppHealth (note that it contains Health already).
| self.health_updater | ||
| .update(Health::from(HealthStatus::Ready)); |
There was a problem hiding this comment.
- Why not update health from
cursorimmediately, like you do below? - To be really nitpicky, one could argue that the state keeper is affected until it processes the pending batch, since it doesn't respond to environment signals (mempool txs and timeouts) until then. This is subjective though, so feel free to ignore.
What ❔
Add three new components to node's healthcheck:
Why ❔
Checklist
zkstack dev fmtandzkstack dev lint.