Skip to content
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

metrics: add separate statesync and blocksync metrics #9682

Merged
merged 6 commits into from
Nov 10, 2022
Merged

Conversation

cmwaters
Copy link
Contributor

@cmwaters cmwaters commented Nov 9, 2022

Statesync and blocksync metrics were previously housed within the consensus metrics. There were some comments that this was messy. Metrics should reside in the component that controls them. This PR separates them out into their respective packages.

@cmwaters cmwaters requested a review from a team November 9, 2022 14:11
Copy link
Contributor

@thanethomson thanethomson left a comment

Choose a reason for hiding this comment

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

There were some comments that this was messy.

Were these comments in synchronous discussions, or are they in an issue somewhere?

Also, this changes the metric labels from block_syncing -> blocksync_syncing and state_syncing -> statesync_syncing, right?

Do we anticipate that there'll be more metrics added in the blocksync_ and statesync_ groups in the v0.38 release? If not, then I'd advocate for keeping the metric labels the same. Every such change introduces friction into an upgrade, and I'd say we should keep upgrade friction at a minimum.

@thanethomson thanethomson added the C:metrics Component: Metrics label Nov 9, 2022
@thanethomson
Copy link
Contributor

If not, then I'd advocate for keeping the metric labels the same.

Actually, I take this back. If we break this once then we can roll out additional metrics in an additive, non-breaking way in patch releases. Please add a changelog and/or upgrading guide entry for this though?

@cmwaters
Copy link
Contributor Author

Were these comments in synchronous discussions, or are they in an issue somewhere?

Some synchronous but mostly in code:

// FIXME We need to update metrics here, since other reactors don't have access to them.

// FIXME Very ugly to have these metrics bleed through here.

Do we anticipate that there'll be more metrics added in the blocksync_ and statesync_ groups in the v0.38 release?

I can imagine that there would eventually be more metrics. Tracking the amount of snapshots available in the network or the latest height blocksynced etc.

Please add a changelog and/or upgrading guide entry for this though?

Sure

statesync/reactor.go Outdated Show resolved Hide resolved
@cmwaters cmwaters closed this Nov 10, 2022
@cmwaters cmwaters reopened this Nov 10, 2022
@cmwaters cmwaters merged commit 99a7ac8 into main Nov 10, 2022
@cmwaters cmwaters deleted the cal/add-metrics branch November 10, 2022 18:13
@cmwaters cmwaters mentioned this pull request Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:metrics Component: Metrics
Projects
Status: Done/Merged
Development

Successfully merging this pull request may close these issues.

None yet

3 participants