Merge pull request #2220 from Jamesbarford/feat/completed-at-not-derived

Feat; add a duration column for benchmark_request's

Author: Jamesbarford

database/src/lib.rs

@@ -812,7 +812,10 @@ pub enum BenchmarkRequestStatus {
     WaitingForArtifacts,
     ArtifactsReady,
     InProgress,
-    Completed { completed_at: DateTime<Utc> },
+    Completed {
+        completed_at: DateTime<Utc>,
+        duration: Duration,
+    },
 }
 
 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<DateTime<Utc>>,
+        duration_ms: Option<i32>,
     ) -> anyhow::Result<Self> {
         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: Duration::from_millis(duration_ms.ok_or_else(|| {
+                    anyhow!("No completion duration for a completed BenchmarkRequestStatus")
+                })? as u64),
             }),
             _ => 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<String>)>,
+    /// A Vector of; completed requests with any associated errors
+    pub completed_requests: Vec<(BenchmarkRequest, Vec<String>)>,
+    /// In progress requests along with their associated jobs
     pub in_progress: Vec<(BenchmarkRequest, Vec<BenchmarkJob>)>,
 }

database/src/pool.rs

@@ -973,16 +973,38 @@ 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
                 .unwrap();
 
             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, .. } = req.status() else {
+                unreachable!();
+            };
+            assert!(duration >= Duration::from_millis(1000));
+
+            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;
@@ -1059,11 +1081,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()
             );
 

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<P> 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
+                        WHERE job_queue.request_tag = $2
+                    )
                 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,19 +2073,16 @@ where
             })
             .collect();
 
-        let completed_requests: Vec<(BenchmarkRequest, String, Vec<String>)> = self
+        let completed_requests: Vec<(BenchmarkRequest, Vec<String>)> = self
             .conn()
             .query(&completed_requests_query, &[])
             .await?
             .iter()
             .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<String>>(11),
+                    it.get::<_, Option<Vec<String>>>(11).unwrap_or_default(),
                 )
             })
             .collect();
@@ -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<DateTime<Utc>>>(9);
+    let duration_ms = row.get::<_, Option<i32>>(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 {