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

client/finality-grandpa: Instrument until-imported queue #5438

Merged
merged 2 commits into from
Mar 31, 2020

Conversation

mxinden
Copy link
Contributor

@mxinden mxinden commented Mar 27, 2020

The UntilImported queue takes as input finality grandpa messages that
depend on blocks that are not yet imported and holds them back until
those blocks are imported.

This commit adds a basic metric, the amount of messages waiting in the
queue, to the module. For now this metric is only available for the
global UntilImported queue awaiting blocks for commit and catch-up
messages.

# HELP substrate_finality_grandpa_until_imported_waiting_messages_number Number of waiting finality grandpa messages within the until imported queue.                            
# TYPE substrate_finality_grandpa_until_imported_waiting_messages_number gauge                                                         
substrate_finality_grandpa_until_imported_waiting_messages_number 11

The `UntilImported` queue takes as input finality grandpa messages that
depend on blocks that are not yet imported and holds them back until
those blocks are imported.

This commit adds a basic metric, the amount of messages waiting in the
queue, to the module. For now this metric is only available for the
global `UntilImported` queue awaiting blocks for commit and catch-up
messages.
Copy link
Contributor

@expenses expenses left a comment

Choose a reason for hiding this comment

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

LGTM, one tiny nitpick.

Comment on lines +89 to +92
// When a queue is dropped it might still contain messages. In order for those to not distort the
// Prometheus metrics, the `Metric` struct cleans up after itself within its `Drop` implementation
// by subtracting the local_waiting_messages (the amount of messages left in the queue about to
// be dropped) from the global_waiting_messages gauge.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is very clever :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe a bit too clever. Hope to be able to get rid of this in the future.

client/finality-grandpa/src/until_imported.rs Outdated Show resolved Hide resolved
Co-Authored-By: Ashley <ashley.ruglys@gmail.com>
@mxinden
Copy link
Contributor Author

mxinden commented Mar 27, 2020

Thanks for the quick review @expenses.

Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

changes lgtm, but going forward it probably would make sense to split between global and round-level messages (once we start counting those)

@mxinden mxinden added A8-mergeoncegreen and removed A0-please_review Pull request needs code review. labels Mar 31, 2020
@gavofyork gavofyork merged commit 95c4e64 into paritytech:master Mar 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants