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

Add counter for unapproved candidates #7491

Merged
merged 9 commits into from
Aug 10, 2023
+29 −0
Merged

Add counter for unapproved candidates #7491

Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f16ad07
Add counter for unapproved candidates
AndreiEres Jul 11, 2023
74d2fd0
Update metrics
AndreiEres Jul 20, 2023
09fbc9d
Split metrics
AndreiEres Jul 25, 2023
2b72297
Remove depth metric
AndreiEres Jul 25, 2023
beb8ada
Print only the oldest unapproved candidates
AndreiEres Jul 26, 2023
eb27052
Update logging condition
AndreiEres Aug 2, 2023
384647b
Fix logging condition
AndreiEres Aug 8, 2023
6315e11
Update logging
AndreiEres Aug 8, 2023
1d0fd8f
Update node/core/approval-voting/src/lib.rs
AndreiEres Aug 8, 2023
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
32 changes: 32 additions & 0 deletions node/core/approval-voting/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ struct MetricsInner {
time_db_transaction: prometheus::Histogram,
time_recover_and_approve: prometheus::Histogram,
candidate_signatures_requests_total: prometheus::Counter<prometheus::U64>,
unapproved_candidates_in_unfinalized_chain: prometheus::GaugeVec<prometheus::U64>,
}

/// Approval Voting metrics.
Expand Down Expand Up @@ -246,6 +247,15 @@ impl Metrics {
fn time_recover_and_approve(&self) -> Option<metrics::prometheus::prometheus::HistogramTimer> {
self.0.as_ref().map(|metrics| metrics.time_recover_and_approve.start_timer())
}

fn on_unapproved_candidates_in_unfinalized_chain(&self, count: usize, depth: usize) {
if let Some(metrics) = &self.0 {
metrics
.unapproved_candidates_in_unfinalized_chain
.with_label_values(&[&count.to_string(), &depth.to_string()])
.set(count as u64);
}
}
}

impl metrics::Metrics for Metrics {
Expand Down Expand Up @@ -336,6 +346,16 @@ impl metrics::Metrics for Metrics {
)?,
registry,
)?,
unapproved_candidates_in_unfinalized_chain: prometheus::register(
prometheus::GaugeVec::new(
prometheus::Opts::new(
"polkadot_parachain_approval_unapproved_candidates_in_unfinalized_chain",
"Number of unapproved candidates in unfinalized chain",
),
&["count", "depth"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, this is not what I meant. There should be one label, and it's value can be count or depth. I think it's better to have 2 separate Gauge in this case, one measuring depth of first unapproved and the total count of unapproved candidates.

)?,
registry,
)?,
};

Ok(Metrics(Some(metrics)))
Expand Down Expand Up @@ -1298,6 +1318,7 @@ async fn handle_from_overseer<Context>(
lower_bound,
wakeups,
&mut approved_ancestor_span,
&metrics,
)
.await
{
Expand Down Expand Up @@ -1423,9 +1444,11 @@ async fn handle_approved_ancestor<Context>(
lower_bound: BlockNumber,
wakeups: &Wakeups,
span: &mut jaeger::Span,
metrics: &Metrics,
) -> SubsystemResult<Option<HighestApprovedAncestorBlock>> {
const MAX_TRACING_WINDOW: usize = 200;
const ABNORMAL_DEPTH_THRESHOLD: usize = 5;
const LOGGING_DEPTH_THRESHOLD: usize = 10;
let mut span = span
.child("handle-approved-ancestor")
.with_stage(jaeger::Stage::ApprovalChecking);
Expand Down Expand Up @@ -1534,6 +1557,15 @@ async fn handle_approved_ancestor<Context>(
unapproved.len(),
entry.candidates().len(),
);
if bits.len() > LOGGING_DEPTH_THRESHOLD {
Copy link
Contributor

Choose a reason for hiding this comment

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

I still think this would be excessive output. What's holding finality is basically the oldest unapproved candidates. Let's just print a the candidate hashes of the in the oldest unapproved block only.

gum::trace!(
target: LOG_TARGET,
"Unapproved blocks on depth {}: {:?}",
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 also add the relay chain block hash in the trace.

bits.len(),
unapproved
)
}
metrics.on_unapproved_candidates_in_unfinalized_chain(unapproved.len(), bits.len());
entry_span.add_uint_tag("unapproved-candidates", unapproved.len() as u64);
for candidate_hash in unapproved {
match db.load_candidate_entry(&candidate_hash)? {
Expand Down