feat(resharding): added metrics #9528
Conversation
chain/chain/src/metrics.rs
Outdated
| Finished, | ||
| } | ||
|
|
||
| impl Into<i64> for ReshardingStatus { |
There was a problem hiding this comment.
nit: https://doc.rust-lang.org/std/convert/trait.From.html
One should always prefer implementing From over Into because implementing From automatically provides one with an implementation of Into thanks to the blanket implementation in the standard library.
chain/chain/src/resharding.rs
Outdated
| @@ -327,7 +354,7 @@ impl Chain { | |||
| fn initialize_flat_storage( | |||
| &self, | |||
| prev_hash: &CryptoHash, | |||
| child_shard_uids: &[&ShardUId], | |||
| child_shard_uids: &Vec<ShardUId>, | |||
There was a problem hiding this comment.
Didn't we have some convention saying we shouldn't be passing Vecs as params but only spans of vecs (the &[...])? @pugachAG
There was a problem hiding this comment.
Let me try, it does seem more "rusty".
chain/chain/src/resharding.rs
Outdated
|
|
||
| RESHARDING_STATUS | ||
| .with_label_values(&[&shard_uid.shard_id.to_string()]) | ||
| .set(ReshardingStatus::Finished.into()); |
There was a problem hiding this comment.
This should be ReshardingStatus::BuildingState
There was a problem hiding this comment.
We can add this to build_state_for_split_shards function as well if you'd like?
There was a problem hiding this comment.
For the labels why are we using shard_id instead of shard_uid?
There was a problem hiding this comment.
This should be ReshardingStatus::BuildingState
thanks copy pasta my bad
We can add this to build_state_for_split_shards function as well if you'd like?
I don't think we'll ever need it and we can always add it quickly.
For the labels why are we using shard_id instead of shard_uid?
It has more info in it and I feel better knowing for sure what shard layout version we're talking about.
There was a problem hiding this comment.
It has more info in it and I feel better knowing for sure what shard layout version we're talking about.
Yes, exactly, we should be using shard_uid, the code currently has shard_id.
There was a problem hiding this comment.
Ah thanks, total brain fart on my side, I was totally intending to use shard uids but somehow ended up with shard id. Will fix soon.
| @@ -305,7 +326,7 @@ impl Chain { | |||
| let block_header = self.get_block_header(sync_hash)?; | |||
| let prev_hash = block_header.prev_hash(); | |||
|
|
|||
| let child_shard_uids = state_roots.keys().collect_vec(); | |||
| let child_shard_uids = state_roots.keys().cloned().collect_vec(); | |||
There was a problem hiding this comment.
Why do we need the clone here?
There was a problem hiding this comment.
This variable needs to outlive the state_roots
- state_roots are consumed in line 333
- child_shard_uids are needed in line 341
It's very small so I'm not worried about perf.
| RESHARDING_BATCH_COUNT.with_label_values(&[shard_uid.to_string().as_str()]).inc(); | ||
| RESHARDING_BATCH_SIZE | ||
| .with_label_values(&[shard_uid.to_string().as_str()]) | ||
| .add(size as i64) |
There was a problem hiding this comment.
We can try returning size as i64 instead of u64 in get_trie_update_batch?
There was a problem hiding this comment.
Either way we need to cast at some point. Logically size is unsigned so I feel better keeping it as so. Casting here seems ok because it is actually when it stops being "size" and becomes "arbitrary numeric value".
* feat(resharding): added metrics * shard_uid * code review * shard_uid
* feat(resharding): added metrics * shard_uid * code review * shard_uid
Added resharding metrics to
a) track the status of resharding
b) track how much data (in batches count and batchs size) was written for each shard