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

Adding Dispute Participation Metrics #6838

Merged
merged 15 commits into from
Mar 11, 2023
Merged

Conversation

BradleyOlson64
Copy link
Contributor

Per issue #4393

We should have metrics on the following:

  1. How long does the actual participation task take
  2. How long does it take from queuing a participation to the participation task finishing
  3. Participation queue sizes (best effort & priority)

@BradleyOlson64 BradleyOlson64 self-assigned this Mar 7, 2023
@BradleyOlson64 BradleyOlson64 added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Mar 7, 2023
@BradleyOlson64 BradleyOlson64 linked an issue Mar 7, 2023 that may be closed by this pull request
node/core/dispute-coordinator/src/metrics.rs Outdated Show resolved Hide resolved
Comment on lines 63 to 66
/// Timer handle for each participation request. Stored to measure full request
/// completion time. Optimally these would have been stored in the participation
/// request itself, but HistogramTimer doesn't implement the Clone trait.
request_timers: BTreeMap<CandidateComparator, Option<prometheus::HistogramTimer>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

HistogramTimer doesn't implement the Clone trait.

I think you can put it behind a smart pointer like Rc. Might simplify the code. 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

Rc doesn't work because we have multithreaded tokio runtime, we would need Arc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Placed the timer in ParticipationRequest. It does indeed simplify the changes 👍 Please check the refactor when you have a moment!

node/core/dispute-coordinator/src/metrics.rs Show resolved Hide resolved
Comment on lines 63 to 66
/// Timer handle for each participation request. Stored to measure full request
/// completion time. Optimally these would have been stored in the participation
/// request itself, but HistogramTimer doesn't implement the Clone trait.
request_timers: BTreeMap<CandidateComparator, Option<prometheus::HistogramTimer>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Rc doesn't work because we have multithreaded tokio runtime, we would need Arc.

@@ -221,12 +221,12 @@ impl metrics::Metrics for Metrics {
)?,
registry,
)?,
priority_queue_size: prometheus::register(
participation_priority_queue_size: prometheus::register(
prometheus::Gauge::new("polkadot_parachain_dispute_priority_queue_size",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Should the metric names be updated to match the new variable names? e.g. "polkadot_parachain_dispute_participation_priority_queue_size"

Comment on lines 919 to 925
let request_timer = Arc::new(self.metrics.time_participation_pipeline());
let r = self
.participation
.queue_participation(
ctx,
priority,
ParticipationRequest::new(new_state.candidate_receipt().clone(), session),
ParticipationRequest::new(new_state.candidate_receipt().clone(), session, request_timer),
Copy link
Contributor

Choose a reason for hiding this comment

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

My first reaction was maybe request_timer can be initialized in ParticipationRequest::new. But after looking at it, doing it like this instead of hiding it inside ParticipationRequest::new makes it more explicit that we are starting a timer here. 👍

Comment on lines 140 to 142
if &self.candidate_receipt == other.candidate_receipt() &&
&self.candidate_hash == other.candidate_hash() &&
self.session == other.session()
Copy link
Contributor

@mrcnski mrcnski Mar 10, 2023

Choose a reason for hiding this comment

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

I would destructure self into its fields, so that if a field is ever added in the future we don't forget to add it to the comparison here. It would look something like this:

let ParticipationRequest { candidate_receipt, candidate_hash, session, _request_timer } = self;
/* do the comparison */

Credit to @eskimor for teaching me this!

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Also, instead of if cond { return true; } false, you can simplify it to just cond. :) I think clippy should warn about this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Credit to @eskimor for teaching me this!

+1 :)

// eq. This helper checks whether all other fields are equal,
// which is sufficient.
#[cfg(test)]
pub fn functionally_equal(&self, other: ParticipationRequest) -> bool {
Copy link
Contributor

@mrcnski mrcnski Mar 10, 2023

Choose a reason for hiding this comment

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

nit: I had an internal struggle about this, but I think we should just impl eq on this type. I believe that already implies functional equality. I read the docs for PartialEq, and there is no restriction mentioned about having to include every field in the equality comparison.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting! I hadn't thought of it that way. But it does make sense. I shall make changes.

Copy link
Contributor

@mrcnski mrcnski left a comment

Choose a reason for hiding this comment

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

Looks good! Left some nits, but nothing blocking.

@@ -235,7 +241,8 @@ impl Participation {
recent_head: Hash,
) -> FatalResult<()> {
while self.running_participations.len() < MAX_PARALLEL_PARTICIPATIONS {
if let Some(req) = self.queue.dequeue() {
let maybe_req = self.queue.dequeue();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Why we need this variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! That was an artifact from a discarded implementation.

// which is sufficient.
#[cfg(test)]
pub fn functionally_equal(&self, other: ParticipationRequest) -> bool {
if &self.candidate_receipt == other.candidate_receipt() &&
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd implement this by destructuring (unless it's not possible due to something I'm not seeing right now). This way we'll get a compilation error if a new field is added to the struct.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also can't we implement std::cmp::PartialEq::eq for ParticipationRequest so that we have got a bit nicer looking syntax?

@tdimitrov
Copy link
Contributor

Good job! Just some small remarks from me.

@tdimitrov
Copy link
Contributor

tdimitrov commented Mar 10, 2023

I just saw that @mrcnski has already left comments similar to mine. Sorry for the double posting :/

@BradleyOlson64 BradleyOlson64 removed the request for review from eskimor March 11, 2023 00:00
@BradleyOlson64
Copy link
Contributor Author

bot merge

ordian added a commit that referenced this pull request Mar 16, 2023
* master: (50 commits)
  Issue 4393: Correcting Unnecessary Use of Arc (#6882)
  Companion for #13287  (#6655)
  timestamp ci job logs (#6890)
  Release parachain host API v4 (#6885)
  polkadot companion: #13128 (Pools commission) (#6264)
  companion for #13555 (#6842)
  Bump libgit2-sys from 0.14.1+1.5.0 to 0.14.2+1.5.1 (#6600)
  Bump bumpalo from 3.8.0 to 3.12.0 (#6599)
  Bump git2 from 0.16.0 to 0.16.1 (#6601)
  Council as SpendOrigin (#6877)
  PVF: Document that preparation cannot lead to disputes (#6873)
  Sync versions with current release (0.9.39) (#6875)
  Companion for paritytech/substrate#13592 (#6869)
  Update orchestra to the recent version (#6854)
  Delete unused Cargo.lock (#6870)
  Remove use of Store trait (#6835)
  Companion for Substrate #13564 (#6845)
  Adding Dispute Participation Metrics (#6838)
  Update `substrate` to 48e7cb1 (#6851)
  Move PVF timeouts to executor environment parameters (#6823)
  ...
ordian added a commit that referenced this pull request Mar 21, 2023
…slashing-client

* ao-past-session-slashing-runtime: (23 commits)
  Issue 4393: Correcting Unnecessary Use of Arc (#6882)
  Companion for #13287  (#6655)
  timestamp ci job logs (#6890)
  Release parachain host API v4 (#6885)
  polkadot companion: #13128 (Pools commission) (#6264)
  companion for #13555 (#6842)
  Bump libgit2-sys from 0.14.1+1.5.0 to 0.14.2+1.5.1 (#6600)
  Bump bumpalo from 3.8.0 to 3.12.0 (#6599)
  Bump git2 from 0.16.0 to 0.16.1 (#6601)
  Council as SpendOrigin (#6877)
  PVF: Document that preparation cannot lead to disputes (#6873)
  Sync versions with current release (0.9.39) (#6875)
  Companion for paritytech/substrate#13592 (#6869)
  Update orchestra to the recent version (#6854)
  Delete unused Cargo.lock (#6870)
  Remove use of Store trait (#6835)
  Companion for Substrate #13564 (#6845)
  Adding Dispute Participation Metrics (#6838)
  Update `substrate` to 48e7cb1 (#6851)
  Move PVF timeouts to executor environment parameters (#6823)
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants