-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Collation metrics: exclude drops of fork-based collations to improve metrics accuracy #9319
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
Conversation
|
/cmd prdoc --audience node_dev --bump patch |
polkadot/node/network/collator-protocol/src/collator_side/mod.rs
Outdated
Show resolved
Hide resolved
polkadot/node/network/collator-protocol/src/collator_side/mod.rs
Outdated
Show resolved
Hide resolved
Sajjon
left a comment
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.
Did not get it all, but found some wrong doc comment use.
polkadot/node/network/collator-protocol/src/collator_side/metrics.rs
Outdated
Show resolved
Hide resolved
polkadot/node/network/collator-protocol/src/collator_side/metrics.rs
Outdated
Show resolved
Hide resolved
polkadot/node/network/collator-protocol/src/collator_side/metrics.rs
Outdated
Show resolved
Hide resolved
sandreim
left a comment
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.
Thanks @AndreiEres
| /// Returns true if the collation was included in a block before (or in) last finalized. | ||
| pub fn is_possibly_finalized(&self, last_finalized: BlockNumber) -> bool { | ||
| self.included_at | ||
| .map(|included_at| included_at <= last_finalized) |
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.
If we see some colation included it doesn't really mean it is finalized, because we don't even know if it is on the fork that is getting finalized. However, this should still work fine, because eventually the collation should be backed/included eventually as it was clearly backed offchain. Only issue is that the relay parent expires.
Please add some docs about the limitations of the measurement we are doing.
Sajjon
left a comment
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.
Lgtm!
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-9319-to-stable2503
git worktree add --checkout .worktree/backport-9319-to-stable2503 backport-9319-to-stable2503
cd .worktree/backport-9319-to-stable2503
git reset --hard HEAD^
git cherry-pick -x d51c532f07c7e3307718b95f5a1f8859e14949a0
git push --force-with-lease |
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-9319-to-stable2506
git worktree add --checkout .worktree/backport-9319-to-stable2506 backport-9319-to-stable2506
cd .worktree/backport-9319-to-stable2506
git reset --hard HEAD^
git cherry-pick -x d51c532f07c7e3307718b95f5a1f8859e14949a0
git push --force-with-lease |
…metrics accuracy (#9319) # Description The polkadot_parachain_collation_expired metric is an indicator for parachain block confidence. However, this metric has a critical issue: not every drop should be counted. Lookahead collators intentionally build collations on a relay chain block and its forks, so the drop of fork-based collations is an expected behaviour. If we count them, the drop metrics show a picture that is worse than in reality. To improve tracking accuracy, we should exclude legit drops The minor issue is also present in the expiry mechanism. It doesn't take into account that collation was moved to a different stage, e.g., from "fetched" to "backed", and can write a drop of fetched collation. To solve this issue we should: - Track relay parent finalization. - Record expiration metrics only when relay parent was finalized. - Exclude drops of fork-based collation from the metrics. - Send metrics only for collations that either finalized or dropped. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> (cherry picked from commit d51c532)
|
Successfully created backport PR for |
…metrics accuracy (#9319) # Description The polkadot_parachain_collation_expired metric is an indicator for parachain block confidence. However, this metric has a critical issue: not every drop should be counted. Lookahead collators intentionally build collations on a relay chain block and its forks, so the drop of fork-based collations is an expected behaviour. If we count them, the drop metrics show a picture that is worse than in reality. To improve tracking accuracy, we should exclude legit drops The minor issue is also present in the expiry mechanism. It doesn't take into account that collation was moved to a different stage, e.g., from "fetched" to "backed", and can write a drop of fetched collation. To solve this issue we should: - Track relay parent finalization. - Record expiration metrics only when relay parent was finalized. - Exclude drops of fork-based collation from the metrics. - Send metrics only for collations that either finalized or dropped. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> (cherry picked from commit d51c532)
|
Successfully created backport PR for |
Backport #9319 into `stable2509` from AndreiEres. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot. <!-- # To be used by other automation, do not modify: original-pr-number: #${pull_number} --> Co-authored-by: Andrei Eres <[email protected]> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Egor_P <[email protected]>
…metrics accuracy (#9319) # Description The polkadot_parachain_collation_expired metric is an indicator for parachain block confidence. However, this metric has a critical issue: not every drop should be counted. Lookahead collators intentionally build collations on a relay chain block and its forks, so the drop of fork-based collations is an expected behaviour. If we count them, the drop metrics show a picture that is worse than in reality. To improve tracking accuracy, we should exclude legit drops The minor issue is also present in the expiry mechanism. It doesn't take into account that collation was moved to a different stage, e.g., from "fetched" to "backed", and can write a drop of fetched collation. To solve this issue we should: - Track relay parent finalization. - Record expiration metrics only when relay parent was finalized. - Exclude drops of fork-based collation from the metrics. - Send metrics only for collations that either finalized or dropped. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…metrics accuracy (#9319) # Description The polkadot_parachain_collation_expired metric is an indicator for parachain block confidence. However, this metric has a critical issue: not every drop should be counted. Lookahead collators intentionally build collations on a relay chain block and its forks, so the drop of fork-based collations is an expected behaviour. If we count them, the drop metrics show a picture that is worse than in reality. To improve tracking accuracy, we should exclude legit drops The minor issue is also present in the expiry mechanism. It doesn't take into account that collation was moved to a different stage, e.g., from "fetched" to "backed", and can write a drop of fetched collation. To solve this issue we should: - Track relay parent finalization. - Record expiration metrics only when relay parent was finalized. - Exclude drops of fork-based collation from the metrics. - Send metrics only for collations that either finalized or dropped. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Description
The polkadot_parachain_collation_expired metric is an indicator for parachain block confidence. However, this metric has a critical issue: not every drop should be counted.
Lookahead collators intentionally build collations on a relay chain block and its forks, so the drop of fork-based collations is an expected behaviour. If we count them, the drop metrics show a picture that is worse than in reality. To improve tracking accuracy, we should exclude legit drops
The minor issue is also present in the expiry mechanism. It doesn't take into account that collation was moved to a different stage, e.g., from "fetched" to "backed", and can write a drop of fetched collation.
To solve this issue we should: