Redesign status page

[Kobzol]

This PR redesigns the status page so that we can see the history of runs, to have better visibility of historical benchmark errors, and also to have a more obvious estimation of when will a given PR finish.

This patch can be used to hardcode data for testing locally:

Patch

diff --git a/site/src/request_handlers/status_page.rs b/site/src/request_handlers/status_page.rs
index e1e2563e..f2ae5f04 100644
--- a/site/src/request_handlers/status_page.rs
+++ b/site/src/request_handlers/status_page.rs
@@ -1,18 +1,46 @@
+use database::{Commit, CommitType, Date};
 use std::str;
 use std::sync::Arc;
 
 use crate::api::status;
-use crate::api::status::FinishedRun;
+use crate::api::status::{CurrentState, FinishedRun};
 use crate::db::{ArtifactId, Lookup};
-use crate::load::SiteCtxt;
+use crate::load::{MissingReason, SiteCtxt};
+use crate::server::status::BenchmarkError;
 
 // How many historical (finished) runs should be returned from the status API.
 const FINISHED_RUN_COUNT: u64 = 5;
 
 pub async fn handle_status_page(ctxt: Arc<SiteCtxt>) -> status::Response {
-    let missing = ctxt.missing_commits().await;
-
-    // FIXME: no current builds
+    let mut missing = vec![
+        (
+            Commit {
+                sha: "abcdef".to_string(),
+                date: Date::ymd_hms(2023, 1, 1, 0, 0, 0),
+                r#type: CommitType::Try,
+            },
+            MissingReason::Try {
+                pr: 123,
+                parent_sha: "foobar".to_string(),
+                include: None,
+                exclude: None,
+                runs: None,
+            },
+        ),
+        (
+            Commit {
+                sha: "abcdef2".to_string(),
+                date: Date::ymd_hms(2023, 2, 1, 0, 0, 0),
+                r#type: CommitType::Master,
+            },
+            MissingReason::Master {
+                pr: 421,
+                parent_sha: "foobar2".to_string(),
+                is_try_parent: true,
+            },
+        ),
+    ]; //ctxt.missing_commits().await;
+       // FIXME: no current builds
     let conn = ctxt.conn().await;
     let current = if let Some(artifact) = conn.in_progress_artifacts().await.pop() {
         let steps = conn
@@ -35,8 +63,76 @@ pub async fn handle_status_page(ctxt: Arc<SiteCtxt>) -> status::Response {
         None
     };
 
-    // FIXME: load at least one master commit with errors, if no master commit is in last N commits?
-    // FIXME: cache this whole thing, or write a specific SQL query for it
+    // let current: Option<CurrentState> = None;
+    let current = Some(status::CurrentState {
+        artifact: ArtifactId::Commit(Commit {
+            sha: "abcdef123".to_string(),
+            date: Date::ymd_hms(2023, 09, 11, 18, 00, 00),
+            r#type: CommitType::Try,
+        }),
+        progress: vec![
+            status::Step {
+                step: "a".to_string(),
+                is_done: true,
+                expected_duration: 120,
+                current_progress: 80,
+            },
+            status::Step {
+                step: "b".to_string(),
+                is_done: true,
+                expected_duration: 90,
+                current_progress: 120,
+            },
+            status::Step {
+                step: "c".to_string(),
+                is_done: false,
+                expected_duration: 140,
+                current_progress: 40,
+            },
+            status::Step {
+                step: "runtime:a".to_string(),
+                is_done: false,
+                expected_duration: 3,
+                current_progress: 0,
+            },
+            status::Step {
+                step: "runtime:b".to_string(),
+                is_done: false,
+                expected_duration: 4,
+                current_progress: 0,
+            },
+            status::Step {
+                step: "rustc".to_string(),
+                is_done: false,
+                expected_duration: 1000,
+                current_progress: 0,
+            },
+            status::Step {
+                step: "d".to_string(),
+                is_done: false,
+                expected_duration: 120,
+                current_progress: 0,
+            },
+        ],
+    });
+
+    if let Some(ref current) = current {
+        if let ArtifactId::Commit(ref commit) = current.artifact {
+            missing.push((
+                commit.clone(),
+                MissingReason::InProgress(Some(Box::new(MissingReason::Try {
+                    pr: 1240,
+                    parent_sha: "foo".to_string(),
+                    include: None,
+                    exclude: None,
+                    runs: None,
+                }))),
+            ));
+        }
+    }
+
+    /*// TODO: load at least one master commit with errors, if no master commit is in last N commits?
+    // TODO: cache this whole thing, or write a specific SQL query for it
     let mut finished_runs = Vec::new();
     let idx = ctxt.index.load();
 
@@ -64,7 +160,51 @@ pub async fn handle_status_page(ctxt: Arc<SiteCtxt>) -> status::Response {
             duration: collection.duration.as_secs(),
             finished_at: collection.end_time.timestamp() as u64,
         });
-    }
+    }*/
+    let mut finished_runs = vec![
+        FinishedRun {
+            artifact: ArtifactId::Commit(Commit {
+                sha: "abcdef".to_string(),
+                date: Date::ymd_hms(2023, 09, 05, 01, 02, 03),
+                r#type: CommitType::Try,
+            }),
+            pr: Some(128),
+            errors: vec![],
+            duration: 1234,
+            finished_at: 1694447945,
+        },
+        FinishedRun {
+            artifact: ArtifactId::Commit(Commit {
+                sha: "foobar".to_string(),
+                date: Date::ymd_hms(2023, 09, 04, 01, 02, 03),
+                r#type: CommitType::Master,
+            }),
+            pr: Some(256),
+            errors: vec![
+                BenchmarkError {
+                    name: "foo".to_string(),
+                    error: "ERROR".to_string(),
+                },
+                BenchmarkError {
+                    name: "bar".to_string(),
+                    error: "ERROR 2".to_string(),
+                },
+            ],
+            duration: 1462,
+            finished_at: 1694437945,
+        },
+        FinishedRun {
+            artifact: ArtifactId::Commit(Commit {
+                sha: "foobar2".to_string(),
+                date: Date::ymd_hms(2023, 09, 03, 01, 02, 03),
+                r#type: CommitType::Master,
+            }),
+            pr: Some(100),
+            errors: vec![],
+            duration: 1302,
+            finished_at: 1694427945,
+        },
+    ];
 
     status::Response {
         finished_runs,

[Mark-Simulacrum]

It's hard to tell based on the image - but it looks like the new page might be quite wide? Is it wider than the current page?

I often check in from a phone if there's reports of slowness or similar and the current page is pretty readable without sideways scrolling for me.

Not a blocker of course, scrolling isn't bad and I'm not sure optimizing for phones makes much sense anyway, but figured I'd raise it here.

[Kobzol]

It uses a flex layout, so on mobile the timeline will be displayed below the current collection.That being said, the timeline table could be optimized for mobile more, now it requires a bit of horizontal scrolling.

[Mark-Simulacrum]

Sounds great. I think a bit of scrolling is ok but we can reassess once things merge.

[klensy]

Don't think that this can close #1713, as source of difference not found, but step for simpler debugging.

Maybe rolling window of bench logs/artifacts can be kept, so if anything funny happens again, we can check logs? (not related to current pr, just some thoughts)

[Kobzol]

Good point. We have logs, but I'm not sure if the current ones are very helpful. FWIW, I think that the long duration is caused by DB, it has been behaving quite erratically recently. We could add some timings to the DB operations to check if that is indeed the issue.

[lqd]

The frontend triggers errors on startup for me so I can't really test, unfortunately.

[lqd]

I managed to look at the frontend with the patch, and we may need to iterate a bit after this lands in case the production data has surprising effects (e.g. actual lengths impacting layout and scrolling).

The PR number and sha columns should be aligned and not centered. (The date in the Run end column could also maybe be turned into a fixed width, or the alignment there will be randomly shifting as well).

I didn't look closely at the queries, or API changes. It looked fine at a quick glance. Maybe Mark or Ryan can take a look?

I didn't notice it at first but it seems the timeline reverses the queue order compared to a regular ordering by position. If that's the case, I'm not sure we gain much by mixing the future collections with the past collections in a single list, with the current collection duplicated between the two. Compared to, say, having 2 tables: the queue in ASC order, and history of recent collections in DESC order. That feels clearer to me, but I don't mind If others prefer it.

[Kobzol]

The PR number and sha columns should be aligned and not centered. (The date in the Run end column could also maybe be turned into a fixed width, or the alignment there will be randomly shifting as well).

Right aligned PR and SHA (they will be almost always the same width on rust-lang/rust, but it's still better to align).

The run end data was previously formatted with the local "locale", which I don't like that much, but I didn't want to change that. I'm not sure how to make it fixed width when the format is not fixed.

I didn't notice it at first but it seems the timeline reverses the queue order compared to a regular ordering by position. If that's the case, I'm not sure we gain much by mixing the future collections with the past collections in a single list, with the current collection duplicated between the two. Compared to, say, having 2 tables: the queue in ASC order, and history of recent collections in DESC order. That feels clearer to me, but I don't mind If others prefer it.

Yeah it's currently basically ordered by start/end time, from newest (which can be in the future) to the oldest. The interpretation is that the next PR will "fall down" into the marked row after the current collection ends. Having two tables could also be reasonable. I just wanted to avoid the previous situation where a currently running PR would basically be in a sort of a random position within the list/table.

[Kobzol]

CC @Mark-Simulacrum if you could please take a look at the DB changes (and possibly comment on the new design - is a unified table with future runs and history OK, or would you prefer two separate tables?).

[Mark-Simulacrum]

Broadly happy for it to be a unified table. I think we can always edit if we don't like it, so I'm inclined to move ahead when ready.