diff --git a/doc/howto/QUICKSTART.md b/doc/howto/QUICKSTART.md index d22099a1..53228bf4 100644 --- a/doc/howto/QUICKSTART.md +++ b/doc/howto/QUICKSTART.md @@ -105,7 +105,16 @@ make land-list SINCE=24h LIMIT=200 # a wider window make land-watch # follow them until they settle ``` -Both draw the same table `make demo-requests` does — the demo tool and the CLI share it — but against whatever the queue already holds, so watching a queue no longer means adding to it. They do not carry the same information, though: `list` is a one-shot read of the queue's receipts and does not fetch histories, so its `STAGE` column is always `…`, while `watch` follows the history API and fills the trail in as each request moves. +Both draw the same table `make demo-requests` does — the demo tool and the CLI share it — but against whatever the queue already holds, so watching a queue no longer means adding to it. They do not carry the same information, though: `list` is a one-shot read of the queue's receipts and does not fetch a history per request, so its `STAGE` column shows where each request is and not how it got there, while `watch` follows the history API and fills the whole trail in as each one moves. + +That trail carries more than positions. A request records events while it sits at one — a build starting or finishing, a passed path waiting on a dependency — and those are shown against the status they happened under, with repeats counted: + +``` +accepted → started → validating → validated → batching → batched → +speculating [building ×8, built ×8, waiting] → speculated → landing → landed +``` + +Eight builds means the batch was speculating down eight paths at once, and `waiting` means one of them passed and then sat on a dependency that had not resolved. A request that sailed through reads `speculating [building, built]` instead — the same position, a very different amount of work behind it. `land-watch` fixes its set when it starts and exits non-zero if any request in that set finishes anywhere other than `landed`, which makes it usable from a script. A request accepted after the watch begins is not picked up: a watch that grew as the queue did would never finish. diff --git a/submitqueue/client/view.go b/submitqueue/client/view.go index 554461fc..66accdcf 100644 --- a/submitqueue/client/view.go +++ b/submitqueue/client/view.go @@ -116,13 +116,19 @@ func (rw *Row) elapsed() string { return fmt.Sprintf("%ds", int(end.Sub(rw.Submitted).Seconds())) } -// stage is the path the request has taken, as the gateway recorded it. The -// waiting marker covers the gap between acceptance and the first recorded -// event, so an accepted request is never shown as though nothing happened. +// stage is the path the request has taken, as the gateway recorded it. +// +// A one-shot listing reads the queue's receipts and does not fetch a history +// per request, so it has the position each one holds but not how it got there. +// That is worth showing on its own: a column of "…" says nothing about a queue +// whose rows are mostly `speculating`. func (rw *Row) stage() string { if len(rw.Trail) > 0 { return strings.Join(rw.Trail, " → ") } + if rw.Status != "" { + return rw.Status + } if rw.SQID != "" { return "…" } @@ -133,34 +139,110 @@ func (rw *Row) stage() string { // a caller following requests as they move uses a Tracker instead, which owns // the rows and redraws them. func Draw(rows []*Row, status string) { - newRenderer().draw(rows, status) + r := newRenderer() + // Nothing will be drawn over this, so a table taller than the window simply + // scrolls — which is what a reader of a long listing wants, and why the + // height limit a redrawing watch lives under does not apply here. + r.oneShot = true + r.draw(rows, status) } // digest reduces a request's recorded history to the trail worth showing, the // status it currently holds, and the error the latest event carried. A status // recorded more than once in a row is one step in the trail, not several. +// +// The history holds two kinds of entry. A status is a position the request +// reached, and those are the trail's spine. An event is something that happened +// while it sat at one — a build starting, a passed path waiting on a dependency +// — and never changes the position, so each is shown against the status it +// occurred under rather than as a step of its own: +// +// batched → speculating [building ×2, built] → speculated +// +// Repeats are counted rather than listed. A batch runs one build per +// speculation path, so a request that speculated widely records `building` many +// times, and a trail that spelled each one out would say less than the count +// does while pushing the rest of the row off the line. func digest(events []*pb.HistoryEvent) (trail []string, status, note string) { if len(events) == 0 { return nil, "", "" } + + // Events seen since the last status, in first-seen order with their counts, + // so they can be attached once the step they belong to is complete. + var pending []string + var last string + counts := make(map[string]int) + + flush := func() { + if len(trail) == 0 || len(pending) == 0 { + pending, counts = nil, make(map[string]int) + return + } + trail[len(trail)-1] += " [" + strings.Join(annotate(pending, counts), ", ") + "]" + pending, counts = nil, make(map[string]int) + } + for _, e := range events { - if e == nil || e.Status == "" { + if e == nil { continue } - if len(trail) > 0 && trail[len(trail)-1] == e.Status { + if e.Status == "" { + if e.Event == "" { + continue + } + if counts[e.Event] == 0 { + pending = append(pending, e.Event) + } + counts[e.Event]++ continue } + // A status repeated back-to-back is one step, but anything recorded + // against it in between still belongs to that step. + if e.Status == last { + continue + } + flush() trail = append(trail, e.Status) + last = e.Status } + flush() + if last := events[len(events)-1]; last != nil { status, note = last.Status, last.LastError } - if status == "" && len(trail) > 0 { - status = trail[len(trail)-1] + // The last entry may be an event, which leaves the request where it was. + if status == "" { + status = currentStatus(events) } return trail, status, note } +// annotate renders each event with its count, dropping the count when it +// happened once. +func annotate(order []string, counts map[string]int) []string { + out := make([]string, 0, len(order)) + for _, event := range order { + if counts[event] > 1 { + out = append(out, fmt.Sprintf("%s ×%d", event, counts[event])) + continue + } + out = append(out, event) + } + return out +} + +// currentStatus is the last position the request reached, ignoring anything +// recorded while it sat there. +func currentStatus(events []*pb.HistoryEvent) string { + for i := len(events) - 1; i >= 0; i-- { + if e := events[i]; e != nil && e.Status != "" { + return e.Status + } + } + return "" +} + // outcome is the one-line verdict shown under the finished table. func outcome(rows []*Row) string { landed := 0 @@ -203,17 +285,29 @@ func summarize(rows []*Row) error { // the number of lines it *emitted* — so one wrapped line desyncs every redraw // after it. Everything wide is therefore wrapped deliberately, into lines the // renderer counts itself. +// +// A frame may not be taller than the window either, for the same reason in the +// other axis. Drawing more lines than the window holds scrolls it, and the +// cursor cannot then move back above the first row — every subsequent redraw +// starts from the wrong place and repaints the whole screen instead of the +// table. A watch of more requests than fit therefore shows as many as do and +// says how many it is not showing. type renderer struct { inPlace bool - // width is the terminal's width, re-read before every draw. A watch runs for - // minutes and a window can be resized inside them, so this is not a property - // the process can sample once — see resize. - width int + // oneShot marks a renderer that draws once and returns, so its frame is + // free to be taller than the window: no later frame has to line up with it. + oneShot bool + + // width and height are the terminal's, re-read before every draw. A watch + // runs for minutes and a window can be resized inside them, so neither is a + // property the process can sample once — see resize. + width int + height int - // size reports the terminal's width and whether it could be read. Held as a - // field so a test can drive a resize without a terminal. - size func() (int, bool) + // size reports the terminal's width and height and whether they could be + // read. Held as a field so a test can drive a resize without a terminal. + size func() (int, int, bool) wRequest int wChanges int @@ -231,7 +325,7 @@ type renderer struct { } func newRenderer() *renderer { - width, sized := terminalSize() + width, height, sized := terminalSize() return &renderer{ // Redrawing in place requires knowing the width to wrap to. A terminal // that will not report its size is therefore treated as a log: emitting @@ -240,6 +334,7 @@ func newRenderer() *renderer { // the lines that appeared, so one of those desyncs every frame after it. inPlace: sized, width: width, + height: height, size: terminalSize, wRequest: len("REQUEST"), wChanges: len("CHANGES"), @@ -256,12 +351,12 @@ func newRenderer() *renderer { // Anything that is not a sized terminal — a pipe, a file, a CI log — falls back // to a fixed width, since there is no width to discover and a log wants a // stable one anyway. -func terminalSize() (int, bool) { - w, _, err := term.GetSize(int(os.Stdout.Fd())) +func terminalSize() (int, int, bool) { + w, h, err := term.GetSize(int(os.Stdout.Fd())) if err != nil || w <= 0 { - return defaultLineWidth, false + return defaultLineWidth, 0, false } - return w, true + return w, h, true } // lineWidth is the width to render to. It tolerates a renderer built without @@ -289,14 +384,22 @@ func (r *renderer) resize() { if !r.inPlace || r.size == nil { return } - if w, sized := r.size(); sized { - r.width = w + if w, h, sized := r.size(); sized { + r.width, r.height = w, h } } func (r *renderer) draw(rows []*Row, status string) { r.resize() - body := r.body(rows) + // Widths come from every row, not just the drawn ones, so a column does not + // resize as rows come and go from view. + r.fit(rows) + + visible, hidden := r.visibleRows(rows) + body := r.body(visible) + if hidden > 0 { + body = append(body, fmt.Sprintf(" … %d settled request(s) not shown; the window is too short", hidden)) + } if !r.inPlace { sig := signature(rows) @@ -325,6 +428,74 @@ func (r *renderer) draw(rows []*Row, status string) { r.drawn = true } +// frameOverhead is what a frame spends on lines other than the table: the blank +// line and the status line below it, plus the row the cursor rests on, which +// has to stay inside the window or the next redraw starts a line too low. +const frameOverhead = 3 + +// visibleRows is the rows that fit in the window, and how many were left out. +// +// Settled requests are dropped first. They have stopped changing, so leaving +// them out costs a reader nothing `land-list` will not tell them, whereas +// dropping the ones still moving would hide the only part of the table that is +// doing anything. Within each group the original order is kept, so rows do not +// jump around between redraws. +func (r *renderer) visibleRows(rows []*Row) ([]*Row, int) { + // headerLines is the column header and its rule. + const headerLines = 2 + + // Redirected output is a log, not a window: it scrolls, nothing is + // overwritten, and a reader wants every row. So does a one-shot listing, + // which is drawn once and scrolled back through rather than redrawn. + if !r.inPlace || r.oneShot || r.height <= 0 { + return rows, 0 + } + budget := r.height - frameOverhead - headerLines + if budget < 1 { + return nil, len(rows) + } + + heights := make([]int, len(rows)) + total := 0 + for i, rw := range rows { + heights[i] = len(r.rowLines(rw)) + len(r.noteLines(rw)) + total += heights[i] + } + if total <= budget { + return rows, 0 + } + // One line goes to the note saying what is not shown. + budget-- + + // Keep the unsettled first, then settled, then restore the original order, + // so what survives is the moving part of the table without being reordered. + keep := make([]bool, len(rows)) + used := 0 + for _, settled := range []bool{false, true} { + for i, rw := range rows { + if rw.Done != settled || keep[i] { + continue + } + if used+heights[i] > budget { + continue + } + keep[i] = true + used += heights[i] + } + } + + visible := make([]*Row, 0, len(rows)) + hidden := 0 + for i, rw := range rows { + if keep[i] { + visible = append(visible, rw) + continue + } + hidden++ + } + return visible, hidden +} + // body renders the header and one line per row. func (r *renderer) body(rows []*Row) []string { r.fit(rows) diff --git a/submitqueue/client/view_test.go b/submitqueue/client/view_test.go index 688eef50..19223407 100644 --- a/submitqueue/client/view_test.go +++ b/submitqueue/client/view_test.go @@ -81,11 +81,80 @@ func TestDigest(t *testing.T) { wantNote: "merge conflict", }, { - name: "events without a status do not become steps", + name: "entries carrying neither a status nor an event are ignored", events: []*pb.HistoryEvent{{Status: ""}, {Status: "accepted"}, {Status: ""}}, wantTrail: []string{"accepted"}, wantStatus: "accepted", }, + { + // An event is something that happened while the request sat at a + // position, so it belongs to that step rather than being one. + name: "an event is shown against the status it happened under", + events: []*pb.HistoryEvent{ + {Status: "batched"}, + {Status: "speculating"}, + {Event: "building"}, + {Event: "built"}, + {Status: "speculated"}, + }, + wantTrail: []string{"batched", "speculating [building, built]", "speculated"}, + wantStatus: "speculated", + }, + { + // One build per speculation path, so a request that speculated + // widely records this many times over. + name: "repeats are counted rather than listed", + events: []*pb.HistoryEvent{ + {Status: "speculating"}, + {Event: "building"}, {Event: "building"}, {Event: "building"}, + {Event: "built"}, + }, + wantTrail: []string{"speculating [building ×3, built]"}, + wantStatus: "speculating", + }, + { + name: "an event does not move the request off its status", + events: []*pb.HistoryEvent{ + {Status: "speculating"}, {Event: "waiting"}, + }, + wantTrail: []string{"speculating [waiting]"}, + wantStatus: "speculating", + }, + { + name: "each status collects only the events recorded under it", + events: []*pb.HistoryEvent{ + {Status: "speculating"}, {Event: "building"}, + {Status: "speculated"}, {Event: "invalidated"}, + {Status: "speculating"}, {Event: "building"}, {Event: "built"}, + }, + wantTrail: []string{ + "speculating [building]", "speculated [invalidated]", "speculating [building, built]", + }, + wantStatus: "speculating", + }, + { + name: "a status repeated around its own events is still one step", + events: []*pb.HistoryEvent{ + {Status: "speculating"}, {Event: "building"}, {Status: "speculating"}, {Event: "built"}, + }, + wantTrail: []string{"speculating [building, built]"}, + wantStatus: "speculating", + }, + { + name: "an event before any status has nothing to attach to", + events: []*pb.HistoryEvent{{Event: "building"}, {Status: "accepted"}}, + wantTrail: []string{"accepted"}, + wantStatus: "accepted", + }, + { + name: "an error carried by an event is still reported", + events: []*pb.HistoryEvent{ + {Status: "speculating"}, {Event: "building", LastError: "runner unreachable"}, + }, + wantTrail: []string{"speculating [building]"}, + wantStatus: "speculating", + wantNote: "runner unreachable", + }, } for _, tt := range tests { @@ -165,6 +234,23 @@ func TestRowStage(t *testing.T) { row: Row{SQID: "demo-queue/17", Trail: []string{"accepted", "started", "landed"}}, want: "accepted → started → landed", }, + { + // A listing reads receipts rather than histories, so it knows where + // a request is without knowing how it got there. That still beats + // a column of nothing. + name: "the position it holds, when the trail was never fetched", + row: Row{SQID: "demo-queue/17", Status: "speculating"}, + want: "speculating", + }, + { + name: "a fetched trail is preferred over the bare position", + row: Row{ + SQID: "demo-queue/17", + Status: "landed", + Trail: []string{"accepted", "landed"}, + }, + want: "accepted → landed", + }, } for _, tt := range tests { @@ -310,7 +396,7 @@ func TestDrawFollowsAResize(t *testing.T) { r := newRenderer() r.inPlace = true r.width = width - r.size = func() (int, bool) { return width, true } + r.size = func() (int, int, bool) { return width, 40, true } rows := []*Row{{SQID: "demo-queue/72", Submitted: time.Now(), Trail: longTrail}} @@ -337,7 +423,7 @@ func TestDrawKeepsTheLastWidthWhenTheTerminalStopsAnswering(t *testing.T) { r := newRenderer() r.inPlace = true r.width = 94 - r.size = func() (int, bool) { return defaultLineWidth, false } + r.size = func() (int, int, bool) { return defaultLineWidth, 0, false } r.resize() assert.Equal(t, 94, r.width, "an unanswered probe leaves the last known width alone") @@ -350,7 +436,7 @@ func TestDrawKeepsTheLastWidthWhenTheTerminalStopsAnswering(t *testing.T) { func TestNewRendererNeedsASizeToRedrawInPlace(t *testing.T) { // Test stdout is not a sized terminal, which is exactly the case at issue. r := newRenderer() - width, sized := terminalSize() + width, _, sized := terminalSize() require.False(t, sized, "test stdout is not expected to be a sized terminal") assert.Equal(t, defaultLineWidth, width) assert.False(t, r.inPlace, "without a known width the renderer must not wrap and redraw") @@ -800,6 +886,104 @@ func TestRowLinesPipedStayOnOneLine(t *testing.T) { assert.NotContains(t, lines[0], "…") } +// A frame taller than the window scrolls it, and the cursor cannot then move +// back above the first row — so the next redraw starts from the wrong place and +// repaints the screen instead of the table. Every frame must fit. +func TestDrawNeverExceedsTheWindowHeight(t *testing.T) { + const height = 20 + + r := newRenderer() + r.inPlace = true + r.width = 200 + r.height = height + r.size = func() (int, int, bool) { return 200, height, true } + + rows := make([]*Row, 0, 60) + for i := range 60 { + rows = append(rows, &Row{ + SQID: fmt.Sprintf("demo-queue/%d", i+1), + Submitted: time.Now(), + Trail: []string{"accepted", "started", "speculating"}, + }) + } + + out := captureStdout(t, func() { r.draw(rows, "watching") }) + lines := strings.Split(strings.TrimSuffix(out, "\n"), "\n") + assert.LessOrEqual(t, len(lines), height, + "a frame of %d lines in a %d-line window scrolls it and desyncs every redraw after", len(lines), height) + assert.Equal(t, len(lines), r.lastLines, + "the cursor moves back by lastLines, so it has to be what was emitted") +} + +// The rows worth keeping when they cannot all be kept are the ones still doing +// something; a settled row is over and `land-list` still has it. +func TestDrawKeepsMovingRowsWhenTheWindowIsShort(t *testing.T) { + const height = 14 + + r := newRenderer() + r.inPlace = true + r.width = 200 + r.height = height + r.size = func() (int, int, bool) { return 200, height, true } + + rows := make([]*Row, 0, 30) + for i := range 30 { + rows = append(rows, &Row{ + SQID: fmt.Sprintf("demo-queue/%d", i+1), + Submitted: time.Now(), + Status: "landed", + Trail: []string{"accepted", "landed"}, + Done: true, + }) + } + moving := &Row{ + SQID: "demo-queue/moving", + Submitted: time.Now(), + Status: "speculating", + Trail: []string{"accepted", "speculating"}, + } + rows = append(rows, moving) + + out := captureStdout(t, func() { r.draw(rows, "watching") }) + assert.Contains(t, out, "demo-queue/moving", "the unsettled row must survive the trim") + assert.Contains(t, out, "not shown", "and the reader must be told the table is partial") +} + +// A one-shot listing is scrolled back through, not redrawn over, so hiding rows +// to fit the window would lose them for no reason. +func TestDrawOneShotKeepsEveryRow(t *testing.T) { + rows := make([]*Row, 0, 40) + for i := range 40 { + rows = append(rows, &Row{ + SQID: fmt.Sprintf("demo-queue/%d", i+1), + Submitted: time.Now(), + Status: "landed", + Done: true, + }) + } + + out := captureStdout(t, func() { Draw(rows, "40 request(s)") }) + assert.Contains(t, out, "demo-queue/40", "a listing must not drop rows to fit the window") + assert.NotContains(t, out, "not shown") +} + +// Redirected output is a log: it scrolls, nothing is overwritten, and a reader +// wants every row however many there are. +func TestDrawPipedKeepsEveryRow(t *testing.T) { + r := newRenderer() + r.inPlace = false + r.height = 10 + + rows := make([]*Row, 0, 40) + for i := range 40 { + rows = append(rows, &Row{SQID: fmt.Sprintf("demo-queue/%d", i+1), Submitted: time.Now(), Done: true}) + } + + out := captureStdout(t, func() { r.draw(rows, "watching") }) + assert.Contains(t, out, "demo-queue/40") + assert.NotContains(t, out, "not shown") +} + func TestStageWidthHasAFloor(t *testing.T) { r := newRenderer() r.inPlace = true