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.

Sign up for GitHub

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

Jump to bottom

[Metrics] Track synced blocks. #3231

Merged
merged 3 commits into from
Apr 24, 2024
Merged

[Metrics] Track synced blocks. #3231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
999aa17
Add SYNCED metric
d0cd Apr 20, 2024
e94b8b0
Update SYNCED metric for validators
d0cd Apr 20, 2024
d9bd0db
Update metric to IS_SYNCED
d0cd Apr 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion node/metrics/src/names.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,15 @@

pub(super) const COUNTER_NAMES: [&str; 2] = [bft::LEADERS_ELECTED, consensus::STALE_UNCONFIRMED_TRANSMISSIONS];

pub(super) const GAUGE_NAMES: [&str; 25] = [
pub(super) const GAUGE_NAMES: [&str; 26] = [
bft::CONNECTED,
bft::CONNECTING,
bft::LAST_STORED_ROUND,
bft::PROPOSAL_ROUND,
bft::CERTIFIED_BATCHES,
bft::HEIGHT,
bft::LAST_COMMITTED_ROUND,
bft::IS_SYNCED,
blocks::SOLUTIONS,
blocks::TRANSACTIONS,
blocks::ACCEPTED_DEPLOY,
Expand Down Expand Up @@ -55,6 +56,7 @@ pub mod bft {
pub const CERTIFIED_BATCHES: &str = "snarkos_bft_primary_certified_batches";
pub const HEIGHT: &str = "snarkos_bft_height_total";
pub const LAST_COMMITTED_ROUND: &str = "snarkos_bft_last_committed_round";
pub const IS_SYNCED: &str = "snarkos_bft_is_synced";
}

pub mod blocks {
Expand Down
7 changes: 7 additions & 0 deletions node/sync/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ edition = "2021"

[features]
default = [ ]
metrics = [ "dep:metrics" ]
test = [ "snarkos-node-sync-locators/test" ]

[dependencies.anyhow]
Expand Down Expand Up @@ -47,6 +48,12 @@ path = "../bft/ledger-service"
version = "=2.2.7"
features = [ "ledger-write" ]

[dependencies.metrics]
package = "snarkos-node-metrics"
path = "../metrics"
version = "=2.2.7"
optional = true

[dependencies.snarkos-node-router]
path = "../router"
version = "=2.2.7"
Expand Down
3 changes: 3 additions & 0 deletions node/sync/src/block_sync.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,9 @@ impl<N: Network> BlockSync<N> {
let is_synced = num_blocks_behind <= max_blocks_behind;
// Update the sync status.
self.is_block_synced.store(is_synced, Ordering::SeqCst);
// Update the `IS_SYNCED` metric.
#[cfg(feature = "metrics")]
metrics::gauge(metrics::bft::IS_SYNCED, is_synced);
Copy link
Contributor

@miazn miazn Apr 22, 2024 •
edited

Choose a reason for hiding this comment

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

This looks good! One thing that might make a difference in recording is that metrics are "registered" before they are seen, which basically initializes all metrics in the names folder at 0. Its probably a trivial difference, as for this metric we initialize it and then update sync status pretty quickly, however just wanted to point out that it will be initialized at 0. If we only want to start recording the metric the first time this line of code is hit, the metric name should be removed from the names.rs file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it should be fine since is_block_synced is initialized with Default::default() == false. So we'll have the property that is_block_synced matches IS_SYNCED.

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesnt it update when its initializing the sync module? I think it should reflect the current state 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

@miazn - reading further, this does get triggered with the sync is initialized. You're right.

}

/// Inserts a block request for the given height.
Expand Down