From 90e889e5f292c6114eeb8e367e0e6c19e06476a6 Mon Sep 17 00:00:00 2001 From: James Barford-Evans Date: Mon, 18 Aug 2025 11:01:10 +0100 Subject: [PATCH 1/6] Feat; add a duration column for benchmark_request's --- database/src/lib.rs | 13 +++++++++-- database/src/pool.rs | 4 ++-- database/src/pool/postgres.rs | 43 ++++++++++++++--------------------- 3 files changed, 30 insertions(+), 30 deletions(-) diff --git a/database/src/lib.rs b/database/src/lib.rs index a9cd5a280..c606eece3 100644 --- a/database/src/lib.rs +++ b/database/src/lib.rs @@ -812,7 +812,10 @@ pub enum BenchmarkRequestStatus { WaitingForArtifacts, ArtifactsReady, InProgress, - Completed { completed_at: DateTime }, + Completed { + completed_at: DateTime, + duration_ms: u32, + }, } const BENCHMARK_REQUEST_STATUS_WAITING_FOR_ARTIFACTS_STR: &str = "waiting_for_artifacts"; @@ -833,6 +836,7 @@ impl BenchmarkRequestStatus { pub(crate) fn from_str_and_completion_date( text: &str, completion_date: Option>, + duration_ms: Option, ) -> anyhow::Result { match text { BENCHMARK_REQUEST_STATUS_WAITING_FOR_ARTIFACTS_STR => Ok(Self::WaitingForArtifacts), @@ -842,6 +846,9 @@ impl BenchmarkRequestStatus { completed_at: completion_date.ok_or_else(|| { anyhow!("No completion date for a completed BenchmarkRequestStatus") })?, + duration_ms: duration_ms.ok_or_else(|| { + anyhow!("No completion duration for a completed BenchmarkRequestStatus") + })? as u32, }), _ => Err(anyhow!("Unknown BenchmarkRequestStatus `{text}`")), } @@ -1221,6 +1228,8 @@ impl CollectorConfig { /// status page #[derive(Debug, PartialEq)] pub struct PartialStatusPageData { - pub completed_requests: Vec<(BenchmarkRequest, String, Vec)>, + /// A Vector of; completed requests with any associated errors + pub completed_requests: Vec<(BenchmarkRequest, Vec)>, + /// In progress requests along with their associated jobs pub in_progress: Vec<(BenchmarkRequest, Vec)>, } diff --git a/database/src/pool.rs b/database/src/pool.rs index 3053cae79..26c89d1ef 100644 --- a/database/src/pool.rs +++ b/database/src/pool.rs @@ -1059,11 +1059,11 @@ mod tests { // can't really test duration // ensure errors are correct assert_eq!( - status_page_data.completed_requests[0].2[0], + status_page_data.completed_requests[0].1[0], "This is an error".to_string() ); assert_eq!( - status_page_data.completed_requests[0].2[1], + status_page_data.completed_requests[0].1[1], "This is another error".to_string() ); diff --git a/database/src/pool/postgres.rs b/database/src/pool/postgres.rs index 77141fe5e..9865bba06 100644 --- a/database/src/pool/postgres.rs +++ b/database/src/pool/postgres.rs @@ -381,6 +381,9 @@ static MIGRATIONS: &[&str] = &[ r#" ALTER TABLE benchmark_request ADD COLUMN commit_date TIMESTAMPTZ NULL; "#, + r#" + ALTER TABLE benchmark_request ADD COLUMN duration_ms INTEGER NULL; + "#, ]; #[async_trait::async_trait] @@ -667,7 +670,7 @@ impl PostgresConnection { } const BENCHMARK_REQUEST_COLUMNS: &str = - "tag, parent_sha, pr, commit_type, status, created_at, completed_at, backends, profiles, commit_date"; + "tag, parent_sha, pr, commit_type, status, created_at, completed_at, backends, profiles, commit_date, duration_ms"; #[async_trait::async_trait] impl

Connection for P @@ -1891,7 +1894,14 @@ where benchmark_request SET status = $1, - completed_at = NOW() + completed_at = NOW(), + duration_ms = ( + SELECT (MAX(EXTRACT('epoch' FROM job_queue.completed_at)) - + MIN(EXTRACT('epoch' FROM job_queue.started_at))) * 1000 AS duration_ms + FROM + job_queue + LEFT JOIN benchmark_request ON job_queue.request_tag = benchmark_request.tag + ) WHERE benchmark_request.tag = $2 AND benchmark_request.status != $1 @@ -2020,22 +2030,6 @@ where ORDER BY completed_at DESC LIMIT {max_completed_requests} - ), jobs AS ( - SELECT - completed.tag, - job_queue.started_at, - job_queue.completed_at - FROM - job_queue - LEFT JOIN completed ON job_queue.request_tag = completed.tag - ), stats AS ( - SELECT - tag, - MAX(jobs.completed_at - jobs.started_at) AS duration - FROM - jobs - GROUP BY - tag ), artifacts AS ( SELECT artifact.id, @@ -2056,11 +2050,9 @@ where ) SELECT completed.*, - stats.duration::TEXT, errors.errors AS errors FROM completed - LEFT JOIN stats ON stats.tag = completed.tag LEFT JOIN errors ON errors.tag = completed.tag; " ); @@ -2081,7 +2073,7 @@ where }) .collect(); - let completed_requests: Vec<(BenchmarkRequest, String, Vec)> = self + let completed_requests: Vec<(BenchmarkRequest, Vec)> = self .conn() .query(&completed_requests_query, &[]) .await? @@ -2089,9 +2081,6 @@ where .map(|it| { ( row_to_benchmark_request(it), - // Duration being a string feels odd, but we don't need to - // perform any computations on it - it.get::<_, String>(10), // The errors, if there are none this will be an empty vector it.get::<_, Vec>(11), ) @@ -2217,11 +2206,13 @@ fn row_to_benchmark_request(row: &Row) -> BenchmarkRequest { let backends = row.get::<_, String>(7); let profiles = row.get::<_, String>(8); let commit_date = row.get::<_, Option>>(9); + let duration_ms = row.get::<_, Option>(10); let pr = pr.map(|v| v as u32); - let status = BenchmarkRequestStatus::from_str_and_completion_date(status, completed_at) - .expect("Invalid BenchmarkRequestStatus data in the database"); + let status = + BenchmarkRequestStatus::from_str_and_completion_date(status, completed_at, duration_ms) + .expect("Invalid BenchmarkRequestStatus data in the database"); match commit_type { BENCHMARK_REQUEST_TRY_STR => BenchmarkRequest { From 9c90f18e731e031117116ea5e3729924506decf4 Mon Sep 17 00:00:00 2001 From: James Barford-Evans Date: Mon, 18 Aug 2025 13:25:40 +0100 Subject: [PATCH 2/6] PR Feedback; corrected query so duration_ms is only computed from the jobs associated with the request matching the tag --- database/src/pool/postgres.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/database/src/pool/postgres.rs b/database/src/pool/postgres.rs index 9865bba06..257e6cdef 100644 --- a/database/src/pool/postgres.rs +++ b/database/src/pool/postgres.rs @@ -1901,6 +1901,7 @@ where FROM job_queue LEFT JOIN benchmark_request ON job_queue.request_tag = benchmark_request.tag + WHERE benchmark_request.tag = $2 ) WHERE benchmark_request.tag = $2 From ecfc84ebb92865574b810eec8244ccebc0bbf3b0 Mon Sep 17 00:00:00 2001 From: James Barford-Evans Date: Mon, 18 Aug 2025 13:29:37 +0100 Subject: [PATCH 3/6] simplify query --- database/src/pool/postgres.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/database/src/pool/postgres.rs b/database/src/pool/postgres.rs index 257e6cdef..8d5afaa3d 100644 --- a/database/src/pool/postgres.rs +++ b/database/src/pool/postgres.rs @@ -1900,8 +1900,7 @@ where MIN(EXTRACT('epoch' FROM job_queue.started_at))) * 1000 AS duration_ms FROM job_queue - LEFT JOIN benchmark_request ON job_queue.request_tag = benchmark_request.tag - WHERE benchmark_request.tag = $2 + WHERE job_queue.request_tag = $2 ) WHERE benchmark_request.tag = $2 From 3fab560bce1ff7ab098248a027b9886b00d7d13c Mon Sep 17 00:00:00 2001 From: James Barford-Evans Date: Mon, 18 Aug 2025 14:25:23 +0100 Subject: [PATCH 4/6] modify test --- database/src/pool.rs | 23 +++++++++++++++++++++-- database/src/pool/postgres.rs | 3 ++- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/database/src/pool.rs b/database/src/pool.rs index 26c89d1ef..01b68b7ef 100644 --- a/database/src/pool.rs +++ b/database/src/pool.rs @@ -980,9 +980,28 @@ mod tests { db.mark_benchmark_request_as_completed(tag).await.unwrap(); - let completed = db.load_benchmark_request_index().await.unwrap(); + /* From the status page view we can see that the duration has been + * updated. Albeit that it will be a very short duration. */ + let status_page_view = db.get_status_page_data().await.unwrap(); + let req = &status_page_view + .completed_requests + .iter() + .find(|it| it.0.tag() == Some(tag)) + .unwrap() + .0; + + assert!(matches!( + req.status(), + BenchmarkRequestStatus::Completed { .. } + )); + let BenchmarkRequestStatus::Completed { duration_ms, .. } = req.status() else { + unreachable!(); + }; + assert!(duration_ms >= 1); + + let completed_index = db.load_benchmark_request_index().await.unwrap(); + assert!(completed_index.contains_tag("sha-1")); - assert!(completed.contains_tag("sha-1")); Ok(ctx) }) .await; diff --git a/database/src/pool/postgres.rs b/database/src/pool/postgres.rs index 8d5afaa3d..5aebb240e 100644 --- a/database/src/pool/postgres.rs +++ b/database/src/pool/postgres.rs @@ -2082,7 +2082,8 @@ where ( row_to_benchmark_request(it), // The errors, if there are none this will be an empty vector - it.get::<_, Vec>(11), + it.get::<_, Option>>(11) + .unwrap_or_else(|| vec![]), ) }) .collect(); From d21659f14fc03973131d9e1f965c17030599d6b6 Mon Sep 17 00:00:00 2001 From: James Barford-Evans Date: Mon, 18 Aug 2025 14:38:20 +0100 Subject: [PATCH 5/6] fix clippy lint for unwrap() --- database/src/pool/postgres.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/database/src/pool/postgres.rs b/database/src/pool/postgres.rs index 5aebb240e..a546657eb 100644 --- a/database/src/pool/postgres.rs +++ b/database/src/pool/postgres.rs @@ -2082,8 +2082,7 @@ where ( row_to_benchmark_request(it), // The errors, if there are none this will be an empty vector - it.get::<_, Option>>(11) - .unwrap_or_else(|| vec![]), + it.get::<_, Option>>(11).unwrap_or_default(), ) }) .collect(); From 43c0c4b7e8d7e10e6cea24cb92b37d2d86b2edd0 Mon Sep 17 00:00:00 2001 From: James Barford-Evans Date: Wed, 20 Aug 2025 08:30:46 +0100 Subject: [PATCH 6/6] PR feedback; use `Duration` type --- database/src/lib.rs | 6 +++--- database/src/pool.rs | 7 +++++-- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/database/src/lib.rs b/database/src/lib.rs index c606eece3..e6f918845 100644 --- a/database/src/lib.rs +++ b/database/src/lib.rs @@ -814,7 +814,7 @@ pub enum BenchmarkRequestStatus { InProgress, Completed { completed_at: DateTime, - duration_ms: u32, + duration: Duration, }, } @@ -846,9 +846,9 @@ impl BenchmarkRequestStatus { completed_at: completion_date.ok_or_else(|| { anyhow!("No completion date for a completed BenchmarkRequestStatus") })?, - duration_ms: duration_ms.ok_or_else(|| { + duration: Duration::from_millis(duration_ms.ok_or_else(|| { anyhow!("No completion duration for a completed BenchmarkRequestStatus") - })? as u32, + })? as u64), }), _ => Err(anyhow!("Unknown BenchmarkRequestStatus `{text}`")), } diff --git a/database/src/pool.rs b/database/src/pool.rs index 01b68b7ef..05d6b02b1 100644 --- a/database/src/pool.rs +++ b/database/src/pool.rs @@ -973,6 +973,9 @@ mod tests { assert_eq!(job.request_tag(), benchmark_request.tag().unwrap()); + /* Make the job take some amount of time */ + std::thread::sleep(Duration::from_millis(1000)); + /* Mark the job as complete */ db.mark_benchmark_job_as_completed(job.id(), BenchmarkJobConclusion::Success) .await @@ -994,10 +997,10 @@ mod tests { req.status(), BenchmarkRequestStatus::Completed { .. } )); - let BenchmarkRequestStatus::Completed { duration_ms, .. } = req.status() else { + let BenchmarkRequestStatus::Completed { duration, .. } = req.status() else { unreachable!(); }; - assert!(duration_ms >= 1); + assert!(duration >= Duration::from_millis(1000)); let completed_index = db.load_benchmark_request_index().await.unwrap(); assert!(completed_index.contains_tag("sha-1"));