mara/hyperhive
mara/hyperhive
Watch
0
0
Fork
You've already forked hyperhive
 0
Code Activity

review classifier: export request, verdict, check and run metrics, with a dashboard

Browse source 
The controller counts review requests, verdicts and per-check results and times each run; the classifier service counts retried asks. A provisioned Grafana dashboard charts them.
This commit is contained in:
atlas 2026-10-24 18:50:03 +02:00 • committed by mara
commit eaf26aaadd
1 changed files with 1390 additions and 28 deletions
Download patch file Download diff file Expand all files Collapse all files

312
swarm-controller/src/review_classifier/review.rs
View file

@ -27,7 +27,7 @@ use std::collections::HashSet;
use std::fmt::Write as _;
use std::future::Future;
use std::sync::{Arc, Mutex, MutexGuard, PoisonError};
use std::time::Duration;
use std::time::{Duration, Instant};
use anyhow::Context as _;
use async_nats::jetstream::{self, AckKind, consumer::PullConsumer};
@ -40,6 +40,7 @@ use swarm_wire::review_classifier::{
use tokio::sync::Notify;
use tokio::task::JoinSet;
use super::metrics::{Choice, Metrics, Request};
use super::runs::{self, RunOutcome, RunRecord, Runs, Trigger};
use super::settings::{Settings, SettingsSource};
use super::{FileSource, PullSource, RunSink, split_repo};
@ -101,6 +102,15 @@ impl Event {
}
}
/// The metrics label of the review `event` a run chose.
const fn verdict_label(event: Event) -> Choice {
match event {
Event::Approve => Choice::Approve,
Event::RequestChanges => Choice::RequestChanges,
Event::Comment => Choice::NoVerdict,
}
}
/// What a run posts when it has no clean verdict: the rules didn't load, the
/// PR couldn't be read, the run failed, or any classification has no answer.
/// A comment neither approves nor blocks the merge.
@ -328,7 +338,8 @@ pub(crate) fn on_delivery(state: &AppState, body: &[u8]) {
};
let client = state.status.as_ref().map(|s| s.queue_client());
let runs = state.runs.clone();
tokio::spawn(async move { accept(client.as_ref(), &*forge, &runs, &request).await });
let metrics = Metrics::global();
tokio::spawn(async move { accept(client.as_ref(), &*forge, &runs, &metrics, &request).await });
}
/// Put `request` in [`REQUEST_STREAM`]. When that fails, the account posts a
@ -337,6 +348,7 @@ async fn accept(
client: Option<&async_nats::Client>,
poster: &impl ReviewPoster,
runs: &Runs,
metrics: &Metrics,
request: &Requested,
) {
let repo = format!("{}/{}", request.owner, request.name);
@ -346,6 +358,7 @@ async fn accept(
};
let error = match queued {
Ok(()) => {
metrics.request(Request::Queued);
tracing::info!(
%repo, pr = request.pr, head_sha = request.head.as_str(),
"review request queued"
@ -354,6 +367,8 @@ async fn accept(
}
Err(e) => format!("{e:#}"),
};
metrics.request(Request::QueueFailed);
metrics.verdict(Choice::NoVerdict);
tracing::warn!(
%repo, pr = request.pr, head_sha = request.head.as_str(), %error,
"queueing a review request failed; posting a no-verdict comment"
@ -448,6 +463,7 @@ pub(crate) fn spawn_queue(state: &AppState) {
queue,
poster: forge,
runs,
metrics: Metrics::global(),
};
consume(&client, TIMING, &reviews, handles).await;
});
@ -461,6 +477,7 @@ struct Handles<S, Q, P> {
queue: Q,
poster: Arc<P>,
runs: Runs,
metrics: Metrics,
}
impl<S, Q: Clone, P> Clone for Handles<S, Q, P> {
@ -470,6 +487,7 @@ impl<S, Q: Clone, P> Clone for Handles<S, Q, P> {
queue: self.queue.clone(),
poster: Arc::clone(&self.poster),
runs: self.runs.clone(),
metrics: self.metrics.clone(),
}
}
}
@ -580,6 +598,7 @@ async fn take<S, Q, P>(
subject = %message.subject, error = %e,
"a queued review request did not parse; dropping it"
);
handles.metrics.request(Request::Unparsable);
if let Err(e) = message.ack_with(AckKind::Term).await {
tracing::warn!(error = %e, "dropping an unparsable review request failed");
}
@ -588,6 +607,7 @@ async fn take<S, Q, P>(
};
let Some(claim) = reviews.claim(&request) else {
drop(slot);
handles.metrics.request(Request::Deferred);
tracing::info!(
repo = %format!("{}/{}", request.owner, request.name), pr = request.pr,
head_sha = request.head.as_str(),
@ -606,8 +626,9 @@ async fn take<S, Q, P>(
queue,
poster,
runs,
metrics,
} = &handles;
let work = review(&**forge, &**forge, queue, &**poster, runs, &request, slot);
let work = review(&**forge, queue, &**poster, runs, metrics, &request, slot);
in_progress(&message, timing.ack_wait / 3, work).await;
drop(claim);
if let Err(e) = message.ack().await {
@ -648,15 +669,15 @@ async fn in_progress(
/// effort: a request for a head the PR has moved on from gets a review of the
/// new head, never the old head's verdict.
async fn review(
files: &impl FileSource,
pulls: &impl PullSource,
forge: &(impl FileSource + PullSource),
sink: &(impl RunSink + SettingsSource),
poster: &impl ReviewPoster,
runs: &Runs,
metrics: &Metrics,
req: &Requested,
slot: Slot,
) {
let pull = read_pull(pulls, req).await;
let pull = read_pull(forge, req).await;
let moved = matches!(&pull, Ok(pull) if !req.head.is_empty() && pull.head_sha != req.head);
if moved {
tracing::info!(
@ -670,11 +691,14 @@ async fn review(
&& let Some(ruling) = reposted(&earlier)
{
drop(slot);
metrics.request(Request::Reposted);
metrics.verdict(verdict_label(ruling.event));
runs.record(repost(poster, req, &earlier, ruling).await)
.await;
return;
}
let run = Box::pin(run(files, pull, sink, poster, req)).await;
metrics.request(Request::Started);
let run = Box::pin(run(forge, pull, sink, poster, metrics, req)).await;
drop(slot);
runs.record(run).await;
}
@ -840,11 +864,21 @@ async fn run(
pull: Result<PullForReview, String>,
sink: &(impl RunSink + SettingsSource),
poster: &impl ReviewPoster,
metrics: &Metrics,
req: &Requested,
) -> RunRecord {
let started_unix = unix_now();
let timer = Instant::now();
let (head, narrowed, outcome) = classify(files, pull, sink, req).await;
let mut ruling = review_for(&outcome);
metrics.verdict(verdict_label(ruling.event));
if let Outcome::Ran {
reply: Reply::Done { rows, .. },
..
} = &outcome
{
metrics.checks(rows);
}
if let (Some(reason), Outcome::Ran { .. }) = (narrowed, &outcome) {
let _ = write!(
ruling.body,
@ -863,13 +897,9 @@ async fn run(
Outcome::Ran { reply, .. } => runs::answer(reply),
Outcome::NotRun(_) | Outcome::NoRules { .. } => (None, None),
};
record(
req,
head,
started_unix,
ended(posted_as, reason, sent),
answer,
)
let ended = ended(posted_as, reason, sent);
metrics.run_duration(ended.outcome, timer.elapsed().as_secs_f64());
record(req, head, started_unix, ended, answer)
}
/// Post `review` on `req`'s PR, naming `head` when there is one. The posted
@ -1278,6 +1308,7 @@ mod tests {
use swarm_wire::review_classifier::Polarity;
use super::*;
use crate::review_classifier::metrics::testing::Recorder;
use crate::review_classifier::runs::tests::FakeDoc;
use crate::review_classifier::tests::{Fake, rules_json};
use crate::test_nats::TestNats;
@ -1433,7 +1464,15 @@ mod tests {
poster: &impl ReviewPoster,
req: &Requested,
) -> RunRecord {
run(forge, read_pull(forge, req).await, sink, poster, req).await
run(
forge,
read_pull(forge, req).await,
sink,
poster,
&Metrics::off(),
req,
)
.await
}
/// A JSON rules file holding `n` classifications.
@ -1701,7 +1740,16 @@ mod tests {
let request = requested_at(head);
tokio::spawn(async move {
let slot = reviews.slot().await;
review(&*forge, &*forge, &gate, &*poster, &store, &request, slot).await;
review(
&*forge,
&gate,
&*poster,
&store,
&Metrics::off(),
&request,
slot,
)
.await;
})
}
@ -1856,6 +1904,7 @@ mod tests {
_nats: TestNats,
client: async_nats::Client,
rig: Rig,
recorder: Recorder,
}
impl Queued {
@ -1866,6 +1915,7 @@ mod tests {
_nats: nats,
client,
rig: Rig::new(),
recorder: Recorder::new(),
}
}
@ -1879,6 +1929,7 @@ mod tests {
Some(&self.client),
&*self.rig.poster,
&self.rig.store,
&self.recorder.metrics,
&request,
)
.await;
@ -1893,12 +1944,14 @@ mod tests {
let gate = self.rig.gate.clone();
let poster = Arc::clone(&self.rig.poster);
let store = self.rig.store.clone();
let metrics = self.recorder.metrics.clone();
tokio::spawn(async move {
let handles = Handles {
forge,
queue: gate,
poster,
runs: store,
metrics,
};
consume(&client, TEST_TIMING, &reviews, handles).await;
})
@ -2033,6 +2086,7 @@ mod tests {
Some(&q.client),
&*q.rig.poster,
&q.rig.store,
&Metrics::off(),
&requested_at("old"),
)
.await;
@ -2104,6 +2158,14 @@ mod tests {
"{}",
posted[1].2.body
);
let requests = q.recorder.counter(REQUESTS);
assert_eq!(requests.get("outcome=queued"), Some(&2), "{requests:?}");
assert_eq!(requests.get("outcome=started"), Some(&1), "{requests:?}");
assert_eq!(requests.get("outcome=reposted"), Some(&1), "{requests:?}");
assert!(
requests.get("outcome=deferred").is_some_and(|n| *n >= 1),
"the second request waited at least once: {requests:?}"
);
assert_eq!(q.rig.gate.reached(), 1);
}
@ -2132,12 +2194,21 @@ mod tests {
.expect("both leave the queue");
assert_eq!(q.rig.posted()[0].0, "o/r#8");
assert_eq!(q.rig.gate.reached(), 1);
let requests = q.recorder.counter(REQUESTS);
assert_eq!(requests.get("outcome=unparsable"), Some(&1), "{requests:?}");
}
#[tokio::test]
async fn a_request_that_cannot_be_queued_posts_a_no_verdict_comment() {
let rig = Rig::new();
accept(None, &*rig.poster, &rig.store, &requested_at("abc123")).await;
accept(
None,
&*rig.poster,
&rig.store,
&Metrics::off(),
&requested_at("abc123"),
)
.await;
let posted = rig.posted();
assert_eq!(posted.len(), 1);
let (target, commit, review) = &posted[0];
@ -2960,4 +3031,211 @@ mod tests {
"a run that never ran read no diff: {body}"
);
}
/// Row `name` as the classifier answers it: `verdict`, counted and passed
/// when it matched.
fn named(name: &str, verdict: Verdict, error: Option<&str>) -> Row {
Row {
name: name.to_owned(),
verdict,
..row(error, error.is_none(), error.is_none())
}
}
const REQUESTS: &str = "hyperhive.review_classifier.requests";
const VERDICTS: &str = "hyperhive.review_classifier.verdicts";
const CHECKS: &str = "hyperhive.review_classifier.checks";
const DURATION: &str = "hyperhive.review_classifier.run.duration";
/// `pairs` as a count per label set.
fn counts<const N: usize>(pairs: [(&str, u64); N]) -> std::collections::BTreeMap<String, u64> {
pairs.into_iter().map(|(k, v)| (k.to_owned(), v)).collect()
}
/// One review run per reply, each on its own PR head, then a repeat of the
/// last: the metrics name and label what each did.
#[tokio::test]
async fn a_review_records_its_request_verdict_checks_and_duration() {
let recorder = Recorder::new();
let metrics = &recorder.metrics;
let reviews = Arc::new(Reviews::default());
let store = Runs::Fake(Arc::default());
let poster = Posted::default();
// A request made at a head the PR has moved on from: reviewed at the
// current head. Four rules, one answer of each kind: no verdict.
let moved = Requested {
head: "def456".to_owned(),
..request("r")
};
let rows = vec![
named("c0", Verdict::Match, None),
named("c1", Verdict::NoMatch, None),
named("c2", Verdict::NotApplicable, None),
named("c3", Verdict::NotApplicable, Some("timed out")),
];
let mixed = Fake::new(
Reply::Done {
rows,
overall: Overall::Approve,
},
vec![rules_of(4)],
);
review(
&mixed,
&mixed,
&poster,
&store,
metrics,
&moved,
reviews.slot().await,
)
.await;
// One rule that matched: the run approves, then a repeat of the
// request posts the same verdict again without a run.
let clean = Fake::new(
Reply::Done {
rows: vec![named("c0", Verdict::Match, None)],
overall: Overall::Approve,
},
vec![rules_of(1)],
);
for _ in 0..2 {
review(
&clean,
&clean,
&poster,
&store,
metrics,
&request("r"),
reviews.slot().await,
)
.await;
}
// The queue is not configured: the request is refused with a comment.
accept(None, &poster, &store, metrics, &request("r")).await;
assert_eq!(
recorder.counter(REQUESTS),
counts([
("outcome=queue_failed", 1),
("outcome=reposted", 1),
("outcome=started", 2),
])
);
assert_eq!(
recorder.counter(VERDICTS),
counts([("outcome=approve", 2), ("outcome=no_verdict", 2)])
);
assert_eq!(
recorder.counter(CHECKS),
counts([
("check=c0,result=match", 2),
("check=c1,result=no_match", 1),
("check=c2,result=unsure", 1),
("check=c3,result=no_answer", 1),
])
);
assert_eq!(
recorder.histogram(DURATION),
counts([("outcome=approve", 1), ("outcome=no_verdict", 1)])
);
}
/// A run whose rows ask for changes records that verdict, and one whose
/// post the forge refuses records its duration as `post_failed`.
#[tokio::test]
async fn a_run_records_request_changes_and_a_failed_post() {
let recorder = Recorder::new();
let metrics = &recorder.metrics;
let reply = Reply::Done {
rows: vec![named("c0", Verdict::NoMatch, None)],
overall: Overall::RequestChanges,
};
let fake = Fake::new(reply, vec![rules_of(1)]);
run(
&fake,
read_pull(&fake, &request("r")).await,
&fake,
&Posted::default(),
metrics,
&request("r"),
)
.await;
run(
&fake,
read_pull(&fake, &request("r")).await,
&fake,
&Refusing,
metrics,
&request("r"),
)
.await;
assert_eq!(
recorder.counter(VERDICTS),
counts([("outcome=request_changes", 2)])
);
assert_eq!(
recorder.histogram(DURATION),
counts([("outcome=post_failed", 1), ("outcome=request_changes", 1)])
);
}
/// A repo without rules approves with no checks: it counts as an approve
/// verdict and an approved run, and adds no `checks` row.
#[tokio::test]
async fn a_repo_without_rules_records_an_approve_and_no_checks() {
let recorder = Recorder::new();
let metrics = &recorder.metrics;
let none = Fake::new(
Reply::Failed {
error: "must not run".to_owned(),
},
Vec::new(),
);
run(
&none,
read_pull(&none, &request("r")).await,
&none,
&Posted::default(),
metrics,
&request("r"),
)
.await;
assert_eq!(recorder.counter(VERDICTS), counts([("outcome=approve", 1)]));
assert_eq!(
recorder.histogram(DURATION),
counts([("outcome=approve", 1)])
);
assert!(recorder.counter(CHECKS).is_empty());
// Control: a repo with a rule records its check, so the empty
// `checks` above is the absence of rules and not a lookup that finds
// nothing.
let ruled = Fake::new(
Reply::Done {
rows: vec![named("c0", Verdict::Match, None)],
overall: Overall::Approve,
},
vec![rules_of(1)],
);
run(
&ruled,
read_pull(&ruled, &request("r")).await,
&ruled,
&Posted::default(),
metrics,
&request("r"),
)
.await;
assert_eq!(
recorder.counter(CHECKS),
counts([("check=c0,result=match", 1)])
);
assert_eq!(recorder.counter(VERDICTS), counts([("outcome=approve", 2)]));
}
}