-
Notifications
You must be signed in to change notification settings - Fork 1k
chore: initial statistics subsystem #9687
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
base: master
Are you sure you want to change the base?
chore: initial statistics subsystem #9687
Conversation
User @EclesioMeloJunior, please sign the CLA here. |
ApprovalVoting(Hash, CandidateHash, (ValidatorIndex, DelayTranche)), | ||
|
||
// Candidate received enough approval and now is approved | ||
CandidateApproved(CandidateHash, Hash), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this for ?
@@ -3085,12 +3088,29 @@ where | |||
candidate_hash, | |||
); | |||
|
|||
if let Some(v_idx) = validator_index { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mentioned in the other PR as well, but this just tells us we received a vote, but that doesn't mean we use it for approving the candidate, so we need to make sure the approvals we track and reward are only the approvals we use in tranches_to_approve.
…sdk into statistics-collector
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a first pass, overall looks like it is moving into the right direction, thank you!
Left you some comments of things that I think needs addressing.
|
||
// You should have received a copy of the GNU General Public License | ||
// along with Polkadot. If not, see <http://www.gnu.org/licenses/>. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add some documentation about what this module does.
// Check whether this is approved, while allowing a maximum | ||
// assignment tick of `now - APPROVAL_DELAY` - that is, that | ||
// all counted assignments are at least `APPROVAL_DELAY` ticks old. | ||
let is_approved = check.is_approved(tick_now.saturating_sub(APPROVAL_DELAY)); | ||
if status.last_no_shows != 0 { | ||
metrics.on_observed_no_shows(status.last_no_shows); | ||
sender | ||
.send_message( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of this are on the hot path and would block if the statistics subsystems's queue is full, so let's use just try_send_message
drop the message if the queue is full, also we shouldn't send a message for each vote, it should be just the aggregation once the candidate is approved.
} | ||
|
||
#[overseer::subsystem(StatisticsCollector, error = SubsystemError, prefix = self::overseer)] | ||
impl<Context> StatisticsCollectorSubsystem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to find a better name Statistics too generic for me, maybe ConsensusStatisticsCollector or ConsensusWorkStatsCollector
sender.send_message(StatisticsCollectorMessage::CandidateApproved( | ||
candidate_hash, | ||
block_hash, | ||
candidate_entry.approvals().clone().into(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
candidate_entry.approvals()
returns all the approvals that we received, but that does not mean we used all of them in approving the candidate, take a look at tranches_to_approve to understand how we use the approvals based on the current tick.
/// Candidate backing metrics. | ||
#[derive(Default, Clone)] | ||
pub struct Metrics; | ||
impl metrics::Metrics for Metrics { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will also need some metrics and logs to publish all this gathered stats.
Description
Statistics Collector
subsytemNext Steps