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

network-gossip: add metric for number of local messages #7871

Merged
8 commits merged into from
Jan 12, 2021

Conversation

andresilva
Copy link
Contributor

@andresilva andresilva commented Jan 11, 2021

This is useful to debug if any gossip validator might be leaking messages (i.e. by not expiring them).

Additionally this PR also increases the known messages cache size of network-gossip. This is to accommodate the increasing number of kusama validators. ~800 validators -> ~1600 GRANDPA messages per round -> we keep track of the last 2 completed rounds plus the current one -> ~4800 messages.

Setting this cache to 8192 should use about 256KB of memory (8192 * 32 bytes per block hash).

@andresilva andresilva added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jan 11, 2021
@andresilva andresilva requested review from tomaka and mxinden and removed request for tomaka January 11, 2021 17:31
Copy link
Contributor

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Great to see more metrics being exposed.

Additionally this PR also increases the known messages cache size of network-gossip. This is to accommodate the increasing number of kusama validators. ~800 validators -> ~1600 GRANDPA messages per round -> we keep track of the last 2 completed rounds plus the current one -> ~4800 messages.

Setting this cache to 8192 should use about 256KB of memory (8192 * 32 bytes per block hash).

This sounds reasonable to me.

client/network-gossip/src/state_machine.rs Outdated Show resolved Hide resolved
client/network-gossip/src/state_machine.rs Outdated Show resolved Hide resolved
client/finality-grandpa/src/communication/mod.rs Outdated Show resolved Hide resolved
client/network-gossip/src/state_machine.rs Show resolved Hide resolved
@andresilva
Copy link
Contributor Author

@mxinden Thanks, I implemented all your suggestions.

@andresilva
Copy link
Contributor Author

andresilva commented Jan 11, 2021

I think this metric would be a good addition to the network dashboard, in particular adding a current messages count as: registered - expired.

@mxinden
Copy link
Contributor

mxinden commented Jan 11, 2021

I think this metric would be a good addition to the network dashboard, in particular adding a current messages count as: registered - expired.

Sounds good. There is already a grandpa section. Thus far each of us just edited the dashboard. Feel free to do the same, just please describe your changes in the change-title that is prompted when you hit save.

@andresilva
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Jan 12, 2021

Trying merge.

@ghost ghost merged commit b2a5019 into master Jan 12, 2021
@ghost ghost deleted the andre/gossip-messages-metric branch January 12, 2021 15:04
drahnr pushed a commit that referenced this pull request Jan 13, 2021
* network-gossip: add metric for number of local messages

* grandpa: fix GossipEngine missing metrics registry parameter

* network-gossip: increase known messages cache size

* network-gossip: fix tests

* grandpa: remove unnecessary clone

Co-authored-by: Max Inden <mail@max-inden.de>

* network-gossip: count registered and expired messages separately

* network-gossip: add comment on known messages cache size

* network-gossip: extend comment with cache size in memory

Co-authored-by: Max Inden <mail@max-inden.de>
drahnr added a commit that referenced this pull request Jan 13, 2021
* make helper error types generics

* avoid From<io::Error> dep in runner helper logic

* slip of the pen, bump futures to 0.3.9

* more generics

* generic var spaces

Co-authored-by: Andronik Ordian <write@reusable.software>

* network-gossip: add metric for number of local messages (#7871)

* network-gossip: add metric for number of local messages

* grandpa: fix GossipEngine missing metrics registry parameter

* network-gossip: increase known messages cache size

* network-gossip: fix tests

* grandpa: remove unnecessary clone

Co-authored-by: Max Inden <mail@max-inden.de>

* network-gossip: count registered and expired messages separately

* network-gossip: add comment on known messages cache size

* network-gossip: extend comment with cache size in memory

Co-authored-by: Max Inden <mail@max-inden.de>

* Clean-up pass in network/src/protocol.rs (#7889)

* Remove statistics system

* Remove ContextData struct

* Remove next_request_id

* Some TryFrom nit-picking

* Use constants for peer sets

* contracts: Don't read the previous value when overwriting a storage item (#7879)

* Add `len` function that can return the length of a storage item efficiently

* Make use of the new len function in contracts

* Fix benchmarks

* cargo run --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_contracts --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/contracts/src/weights.rs --template=./.maintain/frame-weight-template.hbs

* Remove unused imports

Co-authored-by: Parity Benchmarking Bot <admin@parity.io>

* Fix clear prefix check to avoid erasing child trie roots. (#7848)

* Fix clear prefix check to avoid erasing child trie roots.

* Renaming and extend existing test with check.

* last nitpicks.

* use follow paths to std standarad components

* line width

Co-authored-by: Bernhard Schuster <bernhard@parity.io>
Co-authored-by: Andronik Ordian <write@reusable.software>
Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com>
Co-authored-by: Max Inden <mail@max-inden.de>
Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com>
Co-authored-by: Alexander Theißen <alex.theissen@me.com>
Co-authored-by: Parity Benchmarking Bot <admin@parity.io>
Co-authored-by: cheme <emericchevalier.pro@gmail.com>
This pull request was closed.
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. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants