From 8c862ac0b8b1d90854bbed8367927bd7e9456826 Mon Sep 17 00:00:00 2001 From: Daniel F B Morinigo Date: Tue, 19 May 2026 16:06:10 +0000 Subject: [PATCH] =?UTF-8?q?feat(roadmap-planner):=20W8=20=E2=80=94=20revie?= =?UTF-8?q?w=5Flatency=5Fp50=5Fhours=20per=20(reviewer,=20week)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit follow-up B3 (2026-05-19). Depends on W2 (this PR is based on feat/metrics-w2-bot-consolidation so the `first_human_review_at` column + `is_bot` tagging are available). Aggregator.Rebuild gains a new step that walks pr_reviews ⨝ pull_requests for the rebuild window. For each non-bot review whose submitted_at equals the PR's first_human_review_at (i.e., the row that defined the first-human touch), we compute latency in hours from PR creation. p50 per (reviewer_id, week_of_pr_created_at) goes into member_week_metrics.review_latency_p50_hours via an ON CONFLICT upsert that reuses the existing PK. Two implementation notes: - p50 is computed in Go using the existing percentile() helper — portable across SQLite + Postgres without percentile_cont(). - The week_start INSERT value is formatted as YYYY-MM-DD so the PK matches what the four sibling INSERTs above emit via dialect.WeekStart(); otherwise SQLite stores a separate row keyed on the time.Time driver-format and the latency falls off the team-overview read. Also adds: - scanTimeAny / parseDateString helpers — SQLite returns week_start as TEXT through the strftime expression; Postgres returns time.Time directly. The helper handles both. Tests: - TestAggregatorReviewLatency seeds 3 PRs with latencies 1/4/9h all reviewed by `alice`, plus a bot comment that must be ignored. Verifies p50 = 4.0. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 --- .../internal/contributions/aggregator.go | 113 +++++++++++++++++- .../internal/contributions/aggregator_test.go | 106 ++++++++++++++++ 2 files changed, 216 insertions(+), 3 deletions(-) diff --git a/roadmap-planner/backend/internal/contributions/aggregator.go b/roadmap-planner/backend/internal/contributions/aggregator.go index 48d6bbe0..eab249af 100644 --- a/roadmap-planner/backend/internal/contributions/aggregator.go +++ b/roadmap-planner/backend/internal/contributions/aggregator.go @@ -10,6 +10,7 @@ package contributions import ( "context" "fmt" + "sort" "strings" "time" @@ -271,9 +272,88 @@ func (a *Aggregator) Rebuild(ctx context.Context, from, to time.Time) error { return fmt.Errorf("aggregate jira: %w", err) } - // TODO(B3): review_latency_p50_hours — derive from - // pull_requests.first_review_at - pull_requests.created_at, p50 by - // (reviewer_id, week). p50 across reviews per author per week. + // Review latency p50, by reviewer × week (W8 2026-05-19). + // + // For each non-bot review row that matches `pr.first_human_review_at` + // (i.e., the first *human* touch on the PR), we record the latency + // in hours from the PR's `created_at`. The week is the week the PR + // was opened — the metric answers "when reviewer X picks up a PR, + // how long after creation does that first response take?" rather + // than "how long does the review process last", which keeps the + // number comparable across reviewers regardless of how long the PR + // sat before they happened to look at it. + // + // p50 is computed in Go (portable across SQLite + Postgres). The + // dataset per (reviewer, week) is small (single-digit reviews for + // most engineers per week), so the in-memory sort is cheap. + latSQL := rebind(fmt.Sprintf(` + SELECT + rv.reviewer_id AS member_id, + %s AS week_start, + %s AS latency_hours + FROM pr_reviews rv + JOIN pull_requests pr ON pr.id = rv.pr_id + WHERE rv.is_bot = 0 + AND rv.reviewer_id IS NOT NULL + AND rv.reviewer_id <> '' + AND pr.first_human_review_at IS NOT NULL + AND rv.submitted_at = pr.first_human_review_at + AND pr.created_at >= ? + AND pr.created_at < ?`, + dialect.WeekStart("pr.created_at"), + hoursBetween(dialect, "pr.first_human_review_at", "pr.created_at"), + )) + latArgs := []any{from, to} + latRows, err := db.QueryContext(ctx, latSQL, latArgs...) + if err != nil { + return fmt.Errorf("aggregate review latency: %w", err) + } + type latKey struct { + member string + week time.Time + } + pools := map[latKey][]float64{} + for latRows.Next() { + var memberID string + var weekStartRaw any + var hours float64 + if err := latRows.Scan(&memberID, &weekStartRaw, &hours); err != nil { + latRows.Close() + return fmt.Errorf("scan review latency: %w", err) + } + weekStart, err := scanTimeAny(weekStartRaw) + if err != nil { + latRows.Close() + return fmt.Errorf("parse review-latency week_start: %w", err) + } + if hours < 0 { + continue + } + pools[latKey{memberID, weekStart}] = append(pools[latKey{memberID, weekStart}], hours) + } + latRows.Close() + if err := latRows.Err(); err != nil { + return fmt.Errorf("iterate review latency: %w", err) + } + if len(pools) > 0 { + upsertSQL := rebind(` + INSERT INTO member_week_metrics (member_id, week_start, pillar_id, component, review_latency_p50_hours) + VALUES (?, ?, '', '', ?) + ON CONFLICT(member_id, week_start, pillar_id, component) DO UPDATE SET + review_latency_p50_hours = excluded.review_latency_p50_hours`) + for k, vs := range pools { + sort.Float64s(vs) + p50 := round1(percentile(vs, 0.50)) + // Bind week_start as the same `YYYY-MM-DD` string the other + // INSERTs in Rebuild emit via `dialect.WeekStart(...)`, so the + // composite primary key matches and the ON CONFLICT branch + // fires instead of creating a parallel row. + weekStr := k.week.UTC().Format("2006-01-02") + if _, err := db.ExecContext(ctx, upsertSQL, k.member, weekStr, p50); err != nil { + return fmt.Errorf("upsert review latency for %s/%s: %w", k.member, k.week, err) + } + } + } // W1 cleanup: every rebuild leaves the four INSERTs above blocked // on the allowlist, but historical rollup rows for now-denied @@ -363,6 +443,33 @@ func (a *Aggregator) RebuildRecent(ctx context.Context, days int) error { return a.Rebuild(ctx, MondayOf(now.AddDate(0, 0, -days)), MondayOf(now.AddDate(0, 0, 7))) } +// scanTimeAny converts a driver value emitted by `dialect.WeekStart(...)` +// to a time.Time. SQLite returns the strftime result as TEXT +// ("YYYY-MM-DD"); Postgres returns date_trunc as time.Time directly. +// Empty / nil inputs produce the zero time without erroring. +func scanTimeAny(v any) (time.Time, error) { + switch t := v.(type) { + case nil: + return time.Time{}, nil + case time.Time: + return t.UTC(), nil + case string: + return parseDateString(t) + case []byte: + return parseDateString(string(t)) + } + return time.Time{}, fmt.Errorf("unsupported time scan type %T", v) +} + +func parseDateString(s string) (time.Time, error) { + for _, layout := range []string{"2006-01-02", "2006-01-02 15:04:05", time.RFC3339, time.RFC3339Nano} { + if t, err := time.Parse(layout, s); err == nil { + return t.UTC(), nil + } + } + return time.Time{}, fmt.Errorf("week_start %q: no layout matched", s) +} + // MondayOf returns the Monday 00:00 UTC of t's week. func MondayOf(t time.Time) time.Time { wd := int(t.Weekday()) diff --git a/roadmap-planner/backend/internal/contributions/aggregator_test.go b/roadmap-planner/backend/internal/contributions/aggregator_test.go index 8b04f0da..1a3ad611 100644 --- a/roadmap-planner/backend/internal/contributions/aggregator_test.go +++ b/roadmap-planner/backend/internal/contributions/aggregator_test.go @@ -211,3 +211,109 @@ func TestAggregatorAllowlist(t *testing.T) { } } } + +// TestAggregatorReviewLatency exercises the W8 (2026-05-19) per-reviewer +// p50 of `first_human_review_at - created_at`. +// +// Seeds three PRs created in the same week, all reviewed by `alice` as +// the first human reviewer. Latencies (in hours): 1, 4, 9 — p50 = 4. +// One bot review is also seeded to confirm it's ignored (is_bot = 1). +func TestAggregatorReviewLatency(t *testing.T) { + ctx := context.Background() + dir := t.TempDir() + store, err := storage.OpenSQLite(filepath.Join(dir, "lat.db")) + if err != nil { + t.Fatalf("open: %v", err) + } + t.Cleanup(func() { _ = store.Close() }) + if err := store.Migrate(ctx); err != nil { + t.Fatalf("migrate: %v", err) + } + for _, m := range []storage.Member{ + {ID: "alice", DisplayName: "Alice", GitHubLogin: "alice", Active: true}, + {ID: "bot", DisplayName: "Bot", Active: true}, + } { + if err := store.UpsertMember(ctx, m); err != nil { + t.Fatalf("upsert %s: %v", m.ID, err) + } + } + + now := time.Now().UTC().Truncate(time.Hour) + created := MondayOf(now).Add(36 * time.Hour) + week := MondayOf(created) + prs := []storage.PullRequest{} + reviews := []storage.PRReview{} + for i, latHours := range []int{1, 4, 9} { + prID := "alaudadevops/x#" + itoa(i+1) + first := created.Add(time.Duration(latHours) * time.Hour) + prs = append(prs, storage.PullRequest{ + ID: prID, Source: "github", RepoID: "alaudadevops/x", Number: i + 1, + Title: "PR", State: "merged", + AuthorLogin: "danielfbm", + CreatedAt: created, + FirstReviewAt: &first, + FirstHumanReviewAt: &first, + FetchedAt: created, + }) + reviews = append(reviews, storage.PRReview{ + ID: prID + "/r1", PRID: prID, Source: "github", + ReviewerID: "alice", ReviewerLogin: "alice", State: "approved", + SubmittedAt: first, + }) + } + // Bot review on the first PR — should be ignored. + reviews = append(reviews, storage.PRReview{ + ID: "alaudadevops/x#1/r-bot", PRID: "alaudadevops/x#1", Source: "github", + ReviewerID: "bot", ReviewerLogin: "renovate", State: "commented", + SubmittedAt: created.Add(30 * time.Minute), IsBot: true, + }) + if err := store.UpsertPullRequests(ctx, prs); err != nil { + t.Fatalf("upsert prs: %v", err) + } + if err := store.UpsertPRReviews(ctx, reviews); err != nil { + t.Fatalf("upsert reviews: %v", err) + } + + agg := NewAggregator(store) + if err := agg.Rebuild(ctx, week.Add(-7*24*time.Hour), week.Add(14*24*time.Hour)); err != nil { + t.Fatalf("rebuild: %v", err) + } + + rows, err := store.MemberWeekMetrics(ctx, storage.MemberWeekQuery{ + From: week.Add(-7 * 24 * time.Hour), + To: week.Add(14 * 24 * time.Hour), + MemberIDs: []string{"alice"}, + }) + if err != nil { + t.Fatalf("read: %v", err) + } + if len(rows) != 1 { + t.Fatalf("expected 1 row, got %d: %+v", len(rows), rows) + } + r := rows[0] + if r.ReviewLatencyP50Hours == nil { + t.Fatalf("review_latency_p50_hours nil; want 4.0") + } + if *r.ReviewLatencyP50Hours != 4.0 { + t.Errorf("review_latency_p50_hours = %v, want 4.0", *r.ReviewLatencyP50Hours) + } +} + +func itoa(n int) string { + if n == 0 { + return "0" + } + neg := n < 0 + if neg { + n = -n + } + out := []byte{} + for n > 0 { + out = append([]byte{'0' + byte(n%10)}, out...) + n /= 10 + } + if neg { + out = append([]byte{'-'}, out...) + } + return string(out) +}