Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

client: only report block import to telemetry if new best#3548

Merged
andresilva merged 4 commits intomasterfrom
andre/telemetry-report-block-import-on-best
Sep 4, 2019
Merged

client: only report block import to telemetry if new best#3548
andresilva merged 4 commits intomasterfrom
andre/telemetry-report-block-import-on-best

Conversation

@andresilva
Copy link
Contributor

@andresilva andresilva commented Sep 4, 2019

Fixes paritytech/substrate-telemetry#175.

Right now we always report to telemetry when we import a new block. Telemetry assumes that this is the new best block which may not be true (we sync all forks we see). This leads the telemetry UI to show more "re-orgs" than actually exist, and also to always show the highest block number for a peer (instead of best). After this PR telemetry UI should always show the latest block notified by the peer (regardless of number), since that it will always be the best block.

@andresilva andresilva added A0-please_review Pull request needs code review. M4-core labels Sep 4, 2019
@andresilva andresilva requested a review from bkchr September 4, 2019 11:02
@andresilva
Copy link
Contributor Author

cc @maciejhirsz

Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@bkchr
Copy link
Member

bkchr commented Sep 4, 2019

So we will only see finalized blocks? And will not be able to see if finalization or block production is stalled?

Copy link
Contributor

@maciejhirsz maciejhirsz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as well.

@bkchr telemetry UI has both best block and finalized block, you might have to enable the relevant columns in the settings if you want to see both numbers / hashes for all nodes.

@bkchr
Copy link
Member

bkchr commented Sep 4, 2019

LGTM as well.

@bkchr telemetry UI has both best block and finalized block, you might have to enable the relevant columns in the settings if you want to see both numbers / hashes for all nodes.

Yeah I know, but I meant something different. However, I finally looked into the code and found the answer to my question.

@andresilva
Copy link
Contributor Author

andresilva commented Sep 4, 2019

@bkchr No, this means that we'll only report to telemetry when we import something that is a new best block (regardless of finality). E.g. with longest chain fork choice rule we will always report to telemetry every time we import a block whose number is higher than the previous best, but e.g. if we import two distinct blocks #10 only one of those will be considered to be a "best" block and therefore we won't report the other one to telemetry. It also means that whenever we import blocks from a different fork we only report to telemetry if we actually re-org (which makes sense to me).

@andresilva andresilva merged commit ca02bee into master Sep 4, 2019
@andresilva andresilva deleted the andre/telemetry-report-block-import-on-best branch September 4, 2019 16:14
Copy link
Contributor

@Demi-Marie Demi-Marie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

andresilva added a commit that referenced this pull request Sep 17, 2019
* client: only report block import to telemetry if new best

* grandpa: fix tests

* consensus: derive Default for ImportedAux

* network: fix test
Demi-Marie pushed a commit to Demi-Marie/substrate that referenced this pull request Sep 17, 2019
…#3548)

* client: only report block import to telemetry if new best

* grandpa: fix tests

* consensus: derive Default for ImportedAux

* network: fix test
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Pick "Best Block" only from validators

5 participants