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

[Merged by Bors] - Metrics for sync aggregate fullness #2439

Closed
wants to merge 3 commits into from
Closed

[Merged by Bors] - Metrics for sync aggregate fullness #2439

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fb084ef
Add metrics for sync committee fullness
paulhauner Jul 8, 2021
2726f82
Avoid pushing metrics during sync
paulhauner Jul 12, 2021
0dc02e5
Add BeaconBlockBody::sync_aggregate
paulhauner Jul 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions beacon_node/beacon_chain/src/beacon_chain.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1976,10 +1976,22 @@ impl<T: BeaconChainTypes> BeaconChain<T> {

drop(validator_monitor);

metrics::observe(
&metrics::OPERATIONS_PER_BLOCK_ATTESTATION,
block.body().attestations().len() as f64,
);
// Only present some metrics for blocks from the previous epoch or later.
//
// This helps avoid noise in the metrics during sync.
if block.slot().epoch(T::EthSpec::slots_per_epoch()) + 1 >= self.epoch()? {
metrics::observe(
&metrics::OPERATIONS_PER_BLOCK_ATTESTATION,
block.body().attestations().len() as f64,
);

if let Ok(block) = block.as_altair() {
Copy link
Member

Choose a reason for hiding this comment

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

This would be good as block.body().sync_aggregate() so that it works for all post-Altair block types, but I realise that the sync_aggregate method doesn't exist yet in unstable.

When we implement the next fork we should audit all uses of as_altair anyway, so I'm happy to merge without fixing this.

The definition of sync_aggregate is:

impl<'a, T: EthSpec> BeaconBlockBodyRef<'a, T> {
    /// Access the sync aggregate from the block's body, if one exists.
    pub fn sync_aggregate(self) -> Option<&'a SyncAggregate<T>> {
        match self {
            BeaconBlockBodyRef::Base(_) => None,
            BeaconBlockBodyRef::Altair(inner) => Some(&inner.sync_aggregate),
        }
    }
}

in beacon_block_body.rs

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, great idea. I hadn't considered this problem before. I've added this function in 0dc02e5. Thanks for laying it all out nicely for me 🙂

Copy link
Member

Choose a reason for hiding this comment

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

Noice 😎

metrics::set_gauge(
&metrics::BLOCK_SYNC_AGGREGATE_SET_BITS,
block.body.sync_aggregate.num_set_bits() as i64,
);
}
}

let db_write_timer = metrics::start_timer(&metrics::BLOCK_PROCESSING_DB_WRITE);

Expand Down
4 changes: 4 additions & 0 deletions beacon_node/beacon_chain/src/metrics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@ lazy_static! {
"beacon_block_processing_attestation_observation_seconds",
"Time spent hashing and remembering all the attestations in the block"
);
pub static ref BLOCK_SYNC_AGGREGATE_SET_BITS: Result<IntGauge> = try_create_int_gauge(
"block_sync_aggregate_set_bits",
"The number of true bits in the last sync aggregate in a block"
);

/*
* Block Production
Expand Down
5 changes: 5 additions & 0 deletions consensus/types/src/sync_aggregate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,9 @@ impl<T: EthSpec> SyncAggregate<T> {
sync_committee_signature: AggregateSignature::empty(),
}
}

/// Returns how many bits are `true` in `self.sync_committee_bits`.
pub fn num_set_bits(&self) -> usize {
self.sync_committee_bits.num_set_bits()
}
}