From 6aa2085eb917180cceace72921b2da4543c97275 Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Mon, 23 Feb 2026 11:00:44 -0500 Subject: [PATCH 01/15] perf: push filters into test report subqueries (~830x improvement) When collapse=false, TestsByNURPAndStandardDeviation builds a query that self-joins prow_test_report_7d_matview 3 times: 1. Outer query - gets the raw rows 2. pass_rates subquery - computes per-variant percentages 3. stats subquery - computes AVG/STDDEV across variants The name/variant filters were only applied to the outermost query. Subqueries 2 and 3 scanned all rows for the release to compute aggregates for every test, even when only a single test was requested. For release 4.22 with a name filter, this meant: | | Before (outer only) | After (pushed down) | |--------------------|-------------------------|---------------------| | Stats subquery | Seq Scan, 1.28M rows | Index Scan, 142 | | Estimated cost | 802,603 - 1,137,530 | 7.53 - 1,371 | | Speedup | - | ~830x | TestsByNURPAndStandardDeviation now accepts optional filter functions (variadic, backward-compatible) that are applied to both the stats and pass_rates subqueries. The filter is still also applied to the outer query, so results are identical. Co-Authored-By: Claude Opus 4.6 --- pkg/api/tests.go | 9 ++++++++- pkg/db/query/test_queries.go | 9 ++++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/pkg/api/tests.go b/pkg/api/tests.go index 415ee9e917..5f15c20f71 100644 --- a/pkg/api/tests.go +++ b/pkg/api/tests.go @@ -14,6 +14,7 @@ import ( "github.com/pkg/errors" log "github.com/sirupsen/logrus" "google.golang.org/api/iterator" + "gorm.io/gorm" apitype "github.com/openshift/sippy/pkg/apis/api" "github.com/openshift/sippy/pkg/apis/cache" @@ -466,7 +467,13 @@ func (spec *TestResultsSpec) buildTestsResultsPGGenerator(ctx context.Context, d if spec.Collapse { rawQuery = rawQuery.Select(`suite_name,name,jira_component,jira_component_id,` + query.QueryTestSummer).Group("suite_name,name,jira_component,jira_component_id") } else { - rawQuery = query.TestsByNURPAndStandardDeviation(dbc, spec.Release, matview) + var subqueryFilters []func(*gorm.DB) *gorm.DB + if rawFilter != nil { + subqueryFilters = append(subqueryFilters, func(db *gorm.DB) *gorm.DB { + return rawFilter.ToSQL(db, apitype.Test{}) + }) + } + rawQuery = query.TestsByNURPAndStandardDeviation(dbc, spec.Release, matview, subqueryFilters...) variantSelect = "suite_name, variants," + "delta_from_working_average, working_average, working_standard_deviation, " + "delta_from_passing_average, passing_average, passing_standard_deviation, " + diff --git a/pkg/db/query/test_queries.go b/pkg/db/query/test_queries.go index 83c1acd89e..bd8028e792 100644 --- a/pkg/db/query/test_queries.go +++ b/pkg/db/query/test_queries.go @@ -244,7 +244,7 @@ func LoadBugsForTest(dbc *db.DB, testName string, filterClosed bool) ([]models.B // flake_average shows the average flake percentage among all variants. // flake_standard_deviation shows the standard deviation of the flake percentage among variants. The number reflects how much flake percentage differs among variants. // delta_from_flake_average shows how much each variant differs from the flake_average. This can be used to identify outliers. -func TestsByNURPAndStandardDeviation(dbc *db.DB, release, table string) *gorm.DB { +func TestsByNURPAndStandardDeviation(dbc *db.DB, release, table string, subqueryFilters ...func(*gorm.DB) *gorm.DB) *gorm.DB { // 1. Create a virtual stats table. There is a single row for each test. stats := dbc.DB.Table(table). Select(` @@ -264,6 +264,13 @@ func TestsByNURPAndStandardDeviation(dbc *db.DB, release, table string) *gorm.DB Select(`id as test_id, suite_name as pass_rate_suite_name, variants as pass_rate_variants, `+QueryTestPercentages). Where(`release = ?`, release) + // Apply any provided filters to the subqueries so they can leverage indexes + // instead of scanning the entire matview for the release. + for _, f := range subqueryFilters { + stats = f(stats) + passRates = f(passRates) + } + // 3. Join the tables to produce test report. Each row represent one variant of a test and contains all stats, both unique to the specific variant and average across all variants. return dbc.DB. Table(table). From ee1ea29c3f4bd56263f3a42692db67da457316da Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Mon, 23 Feb 2026 11:15:19 -0500 Subject: [PATCH 02/15] fix: exclude variant filters from stats subquery Variant-specific filters (e.g., NOT has entry "never-stable") must not be pushed into the stats subquery, which computes AVG/STDDEV across all variants for a test. Filtering out variants there would skew the delta_from_*_average and standard deviation calculations. Split SubqueryFilter into a struct with a VariantOnly flag and an isVariantFilter helper. At the call site, the rawFilter is further split: name filters go to both stats and passRates subqueries (safe, just narrows to the matching test), while variant filters go only to passRates (preserving cross-variant stats semantics). Co-Authored-By: Claude Opus 4.6 --- pkg/api/tests.go | 21 +++++++++++++++++---- pkg/db/query/test_queries.go | 28 +++++++++++++++++++++++----- 2 files changed, 40 insertions(+), 9 deletions(-) diff --git a/pkg/api/tests.go b/pkg/api/tests.go index 5f15c20f71..a7ab17fcab 100644 --- a/pkg/api/tests.go +++ b/pkg/api/tests.go @@ -467,11 +467,24 @@ func (spec *TestResultsSpec) buildTestsResultsPGGenerator(ctx context.Context, d if spec.Collapse { rawQuery = rawQuery.Select(`suite_name,name,jira_component,jira_component_id,` + query.QueryTestSummer).Group("suite_name,name,jira_component,jira_component_id") } else { - var subqueryFilters []func(*gorm.DB) *gorm.DB + var subqueryFilters []query.SubqueryFilter if rawFilter != nil { - subqueryFilters = append(subqueryFilters, func(db *gorm.DB) *gorm.DB { - return rawFilter.ToSQL(db, apitype.Test{}) - }) + variantFilter, nameFilter := rawFilter.Split([]string{"variants"}) + if len(nameFilter.Items) > 0 { + subqueryFilters = append(subqueryFilters, query.SubqueryFilter{ + Apply: func(db *gorm.DB) *gorm.DB { + return nameFilter.ToSQL(db, apitype.Test{}) + }, + }) + } + if len(variantFilter.Items) > 0 { + subqueryFilters = append(subqueryFilters, query.SubqueryFilter{ + Apply: func(db *gorm.DB) *gorm.DB { + return variantFilter.ToSQL(db, apitype.Test{}) + }, + VariantOnly: true, + }) + } } rawQuery = query.TestsByNURPAndStandardDeviation(dbc, spec.Release, matview, subqueryFilters...) variantSelect = "suite_name, variants," + diff --git a/pkg/db/query/test_queries.go b/pkg/db/query/test_queries.go index bd8028e792..d3f948a939 100644 --- a/pkg/db/query/test_queries.go +++ b/pkg/db/query/test_queries.go @@ -244,7 +244,20 @@ func LoadBugsForTest(dbc *db.DB, testName string, filterClosed bool) ([]models.B // flake_average shows the average flake percentage among all variants. // flake_standard_deviation shows the standard deviation of the flake percentage among variants. The number reflects how much flake percentage differs among variants. // delta_from_flake_average shows how much each variant differs from the flake_average. This can be used to identify outliers. -func TestsByNURPAndStandardDeviation(dbc *db.DB, release, table string, subqueryFilters ...func(*gorm.DB) *gorm.DB) *gorm.DB { +// SubqueryFilter wraps a filter function with metadata about what it targets. +// Variant-only filters are applied only to passRates, not to stats, because +// the stats subquery computes cross-variant averages and standard deviations +// that should not be skewed by variant exclusions. +type SubqueryFilter struct { + Apply func(*gorm.DB) *gorm.DB + VariantOnly bool +} + +func isVariantFilter(f SubqueryFilter) bool { + return f.VariantOnly +} + +func TestsByNURPAndStandardDeviation(dbc *db.DB, release, table string, subqueryFilters ...SubqueryFilter) *gorm.DB { // 1. Create a virtual stats table. There is a single row for each test. stats := dbc.DB.Table(table). Select(` @@ -264,11 +277,16 @@ func TestsByNURPAndStandardDeviation(dbc *db.DB, release, table string, subquery Select(`id as test_id, suite_name as pass_rate_suite_name, variants as pass_rate_variants, `+QueryTestPercentages). Where(`release = ?`, release) - // Apply any provided filters to the subqueries so they can leverage indexes - // instead of scanning the entire matview for the release. + // Apply filters to the subqueries so they can leverage indexes instead of + // scanning the entire matview for the release. Variant-specific filters + // only apply to passRates to preserve cross-variant stats semantics. for _, f := range subqueryFilters { - stats = f(stats) - passRates = f(passRates) + if isVariantFilter(f) { + passRates = f.Apply(passRates) + } else { + stats = f.Apply(stats) + passRates = f.Apply(passRates) + } } // 3. Join the tables to produce test report. Each row represent one variant of a test and contains all stats, both unique to the specific variant and average across all variants. From ab588f478dbf2294e70ad3471d4990cb3bae778c Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Wed, 6 May 2026 17:34:25 +0000 Subject: [PATCH 03/15] perf: add server-side pagination for tests API The /api/tests endpoint previously returned all matching rows (up to 50k+ for uncollapsed views), causing the frontend to barely load. This adds server-side pagination following the existing pattern used by the job runs endpoint. When perPage/page query parameters are present, the backend now: - Applies ORDER BY, COUNT, LIMIT, and OFFSET at the SQL level - Returns a PaginationResult envelope with rows, total_rows, page_size, page - Bypasses the cache (paginated queries are fast with LIMIT/OFFSET) When pagination params are absent, existing behavior is preserved for backward compatibility. Frontend changes switch the DataGrid to paginationMode="server" and send perPage/page params, following the JobRunsTable pattern. Co-Authored-By: Claude Opus 4.6 --- pkg/api/tests.go | 61 +++++++++++++++++++++++++++++---- pkg/sippyserver/server.go | 10 ++++-- sippy-ng/src/tests/TestTable.js | 43 ++++++++++++++++++----- 3 files changed, 96 insertions(+), 18 deletions(-) diff --git a/pkg/api/tests.go b/pkg/api/tests.go index a7ab17fcab..e304f7e79a 100644 --- a/pkg/api/tests.go +++ b/pkg/api/tests.go @@ -11,6 +11,7 @@ import ( "time" "cloud.google.com/go/bigquery" + "github.com/lib/pq" "github.com/pkg/errors" log "github.com/sirupsen/logrus" "google.golang.org/api/iterator" @@ -340,12 +341,32 @@ func PrintTestsJSONFromDB( w http.ResponseWriter, req *http.Request, dbc *db.DB, cacheClient cache.Cache, release string, + pagination *apitype.Pagination, ) { spec, ok := makeTestsResultsSpec(w, req, release) if !ok { return } + if pagination != nil { + spec.Pagination = pagination + spec.SortField = param.SafeRead(req, "sortField") + spec.Sort = apitype.Sort(param.SafeRead(req, "sort")) + + result, errs := spec.buildTestsResultsPGGenerator(req.Context(), dbc, spec.matview()) + if errs != nil { + RespondWithJSON(http.StatusInternalServerError, w, map[string]interface{}{"code": http.StatusInternalServerError, "message": fmt.Sprintf("Error building test report: %v", errs)}) + return + } + RespondWithJSON(http.StatusOK, w, apitype.PaginationResult{ + Rows: result.TestsAPIResult, + TotalRows: result.TotalRows, + PageSize: pagination.PerPage, + Page: pagination.Page, + }) + return + } + result, err := spec.buildTestsResultsFromPostgres(req.Context(), dbc, cacheClient) if err != nil { RespondWithJSON(http.StatusInternalServerError, w, map[string]interface{}{"code": http.StatusInternalServerError, "message": "Error building job report:" + err.Error()}) @@ -418,19 +439,28 @@ type TestResultsSpec struct { Release, Period string Collapse, IncludeOverall bool Filter *filter.Filter + Pagination *apitype.Pagination + SortField string + Sort apitype.Sort } + +func (spec *TestResultsSpec) matview() string { + if spec.Period == "twoDay" { + return testReport2dMatView + } + return testReport7dMatView +} + type testResults struct { TestsAPIResult - Test *apitype.Test + Test *apitype.Test + TotalRows int64 } const testResultsCacheDuration = time.Hour func (spec *TestResultsSpec) buildTestsResultsFromPostgres(ctx context.Context, dbc *db.DB, cacheClient cache.Cache) (testResults, error) { - matview := testReport7dMatView - if spec.Period == "twoDay" { - matview = testReport2dMatView - } + matview := spec.matview() generator := func(ctx context.Context) (testResults, []error) { return spec.buildTestsResultsPGGenerator(ctx, dbc, matview) @@ -508,6 +538,24 @@ func (spec *TestResultsSpec) buildTestsResultsPGGenerator(ctx context.Context, d finalResults = processedFilter.ToSQL(finalResults, apitype.Test{}) } + if spec.Pagination != nil { + sortField := spec.SortField + if sortField == "" { + sortField = "current_pass_percentage" + } + sort := spec.Sort + if sort == "" { + sort = apitype.SortAscending + } + finalResults = finalResults.Order(fmt.Sprintf("%s %s NULLS LAST", pq.QuoteIdentifier(sortField), sort)) + + var rowCount int64 + finalResults.Count(&rowCount) + result.TotalRows = rowCount + + finalResults = finalResults.Limit(spec.Pagination.PerPage).Offset(spec.Pagination.Page * spec.Pagination.PerPage) + } + frr := finalResults.Scan(&testReports) if frr.Error != nil { log.WithError(finalResults.Error).Error("error querying test reports") @@ -516,9 +564,8 @@ func (spec *TestResultsSpec) buildTestsResultsPGGenerator(ctx context.Context, d return } - // Produce a special "overall" test that has a summary of all the selected tests. var overallTest *apitype.Test - if spec.IncludeOverall { + if spec.IncludeOverall && spec.Pagination == nil { finalResults := dbc.DB.Table("(?) as final_results", finalResults) finalResults = finalResults.Select(query.QueryTestSummer) summaryResult := dbc.DB.Table("(?) as overall", finalResults).Select(query.QueryTestSummarizer) diff --git a/pkg/sippyserver/server.go b/pkg/sippyserver/server.go index 3165f605c3..212f611ff9 100644 --- a/pkg/sippyserver/server.go +++ b/pkg/sippyserver/server.go @@ -1133,9 +1133,15 @@ func (s *Server) jsonJobBugsFromDB(w http.ResponseWriter, req *http.Request) { func (s *Server) jsonTestsReportFromDB(w http.ResponseWriter, req *http.Request) { release := s.getParamOrFail(w, req, "release") - if release != "" { - api.PrintTestsJSONFromDB(w, req, s.db, s.cache, release) + if release == "" { + return + } + pagination, err := getPaginationParams(req) + if err != nil { + api.RespondWithJSON(http.StatusBadRequest, w, map[string]interface{}{"code": http.StatusBadRequest, "message": "invalid pagination params: " + err.Error()}) + return } + api.PrintTestsJSONFromDB(w, req, s.db, s.cache, release, pagination) } func (s *Server) jsonTestsReportFromBigQuery(w http.ResponseWriter, req *http.Request) { diff --git a/sippy-ng/src/tests/TestTable.js b/sippy-ng/src/tests/TestTable.js index 2455f853a7..9b7a531ee2 100644 --- a/sippy-ng/src/tests/TestTable.js +++ b/sippy-ng/src/tests/TestTable.js @@ -128,7 +128,9 @@ function TestTable(props) { const [fetchError, setFetchError] = React.useState('') const [isLoaded, setLoaded] = React.useState(false) const [isSearching, setSearching] = React.useState(false) - const [rows, setRows] = React.useState([]) + const [apiResult, setApiResult] = React.useState({ rows: [], total_rows: 0 }) + const [page = 0, setPage] = useQueryParam('page', NumberParam) + const [pageFlip, setPageFlip] = React.useState(false) const [period = props.period, setPeriod] = useQueryParam( 'period', @@ -917,6 +919,7 @@ function TestTable(props) { gridView.setView(v) setSort(gridView.view.sort) setSortField(gridView.view.sortField) + setPage(0) } const fetchData = () => { @@ -940,6 +943,8 @@ function TestTable(props) { queryString += '&sortField=' + safeEncodeURIComponent(sortField) queryString += '&sort=' + safeEncodeURIComponent(sort) + queryString += '&perPage=' + safeEncodeURIComponent(pageSize) + queryString += '&page=' + safeEncodeURIComponent(page) queryString += '&collapse=' + safeEncodeURIComponent(props.collapse) @@ -957,12 +962,13 @@ function TestTable(props) { }) .then((json) => { if (json != null) { - setRows(json) + setApiResult(json) } else { - setRows([]) + setApiResult({ rows: [], total_rows: 0 }) } setLoaded(true) setSearching(false) + setPageFlip(false) }) .catch((error) => { setFetchError( @@ -976,7 +982,7 @@ function TestTable(props) { useEffect(() => { if (prevLocation.current !== location) { - setRows([]) + setApiResult({ rows: [], total_rows: 0 }) setLoaded(false) } @@ -986,7 +992,7 @@ function TestTable(props) { fetchData() } else { // Mark as loaded so we don't show loading spinner, and clear any stale data - setRows([]) + setApiResult({ rows: [], total_rows: 0 }) setLoaded(true) setSearching(false) } @@ -997,6 +1003,8 @@ function TestTable(props) { filterModel, sort, sortField, + page, + pageSize, props.collapse, props.briefTable, view, @@ -1004,6 +1012,7 @@ function TestTable(props) { const requestSearch = (searchValue) => { setSearching(true) + setPage(0) const currentFilters = filterModel currentFilters.items = currentFilters.items.filter( (f) => f.columnField !== 'name' @@ -1041,6 +1050,7 @@ function TestTable(props) { currentFilters.push(item) } }) + setPage(0) setFilterModel({ items: currentFilters, linkOperator: filterModel.linkOperator || 'and', @@ -1059,6 +1069,13 @@ function TestTable(props) { if (sortField !== model[0].field) { setSortField(model[0].field) } + + setPage(0) + } + + const changePage = (newPage) => { + setPageFlip(true) + setPage(newPage) } return ( @@ -1071,15 +1088,23 @@ function TestTable(props) { )} (props.collapse ? 100 : 'auto')} disableColumnFilter={props.briefTable} + pagination + paginationMode="server" + page={page} pageSize={pageSize} - onPageSizeChange={(newPageSize) => setPageSize(newPageSize)} + onPageChange={(newPage) => changePage(newPage)} + onPageSizeChange={(newPageSize) => { + setPageSize(newPageSize) + setPage(0) + }} rowsPerPageOptions={props.rowsPerPageOptions} checkboxSelection={false} filterMode="server" @@ -1129,7 +1154,7 @@ function TestTable(props) { filterModel: filterModel, setFilterModel: setFilterModel, downloadDataFunc: () => { - return rows + return apiResult.rows || [] }, downloadFilePrefix: 'tests', }, From 899f339f4ea0fc4c3940dd9b3070bf9e3d679546 Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Wed, 6 May 2026 17:54:25 +0000 Subject: [PATCH 04/15] fix: address review panel findings for pagination - Validate sort direction against allowlist (asc/desc) instead of interpolating raw user input into ORDER BY clause - Add bounds validation for perPage (1-1000) and page (>= 0) in getPaginationParams to prevent DoS via unbounded queries - Check COUNT query error instead of silently ignoring it - Fix BigQuery path to return PaginationResult envelope when pagination params are present (frontend expects this format) - Move COUNT before ORDER BY to avoid unnecessary overhead - Inline isVariantFilter (trivial single-field access) - Separate SubqueryFilter doc comment from function doc block - Update API docs in pkg/api/README.md with new pagination params and response format - Add unit tests for getPaginationParams bounds validation Co-Authored-By: Claude Opus 4.6 --- pkg/api/README.md | 19 +++++- pkg/api/tests.go | 23 +++++-- pkg/db/query/test_queries.go | 7 +-- pkg/sippyserver/parameters.go | 9 +++ pkg/sippyserver/parameters_test.go | 97 ++++++++++++++++++++++++++++++ pkg/sippyserver/server.go | 10 ++- 6 files changed, 151 insertions(+), 14 deletions(-) create mode 100644 pkg/sippyserver/parameters_test.go diff --git a/pkg/api/README.md b/pkg/api/README.md index 6482f1bb18..36d2cdbd54 100644 --- a/pkg/api/README.md +++ b/pkg/api/README.md @@ -408,10 +408,25 @@ Endpoint: `/api/tests` | filter | Filter | Filters the results by the specified value. | See filtering | | sortField| Field name | Sort by this field | | | sort | asc / desc | Sort type, ascending or descending | "asc" or "desc" | -| limit | Integer | The maximum amount of results to return | N/A | +| limit | Integer | The maximum amount of results to return (legacy, use perPage instead) | N/A | +| perPage | Integer | Number of results per page (enables server-side pagination) | 1-1000 | +| page | Integer | Zero-indexed page number (requires perPage) | 0+ | + +When `perPage` is provided, the response is wrapped in a pagination envelope: + +```json +{ + "rows": [ ... ], + "total_rows": 2847, + "page_size": 25, + "page": 0 +} +``` + +When `perPage` is omitted, the legacy array response format is returned for backward compatibility.
-Example response +Example response (legacy, without pagination) ```json [ diff --git a/pkg/api/tests.go b/pkg/api/tests.go index e304f7e79a..e292a134b4 100644 --- a/pkg/api/tests.go +++ b/pkg/api/tests.go @@ -381,7 +381,7 @@ func PrintTestsJSONFromDB( RespondWithJSON(http.StatusOK, w, testsResult) } -func PrintTestsJSONFromBigQuery(release string, w http.ResponseWriter, req *http.Request, bqc *bq.Client) { +func PrintTestsJSONFromBigQuery(release string, w http.ResponseWriter, req *http.Request, bqc *bq.Client, pagination *apitype.Pagination) { spec, ok := makeTestsResultsSpec(w, req, release) if !ok { return @@ -398,6 +398,16 @@ func PrintTestsJSONFromBigQuery(release string, w http.ResponseWriter, req *http testsResult = append([]apitype.TestBQ{*result.Test}, testsResult...) } + if pagination != nil { + RespondWithJSON(http.StatusOK, w, apitype.PaginationResult{ + Rows: testsResult, + TotalRows: int64(len(testsResult)), + PageSize: pagination.PerPage, + Page: pagination.Page, + }) + return + } + RespondWithJSON(http.StatusOK, w, testsResult) } @@ -543,16 +553,19 @@ func (spec *TestResultsSpec) buildTestsResultsPGGenerator(ctx context.Context, d if sortField == "" { sortField = "current_pass_percentage" } - sort := spec.Sort - if sort == "" { + sort := apitype.SortDescending + if spec.Sort == apitype.SortAscending { sort = apitype.SortAscending } - finalResults = finalResults.Order(fmt.Sprintf("%s %s NULLS LAST", pq.QuoteIdentifier(sortField), sort)) var rowCount int64 - finalResults.Count(&rowCount) + if err := finalResults.Count(&rowCount).Error; err != nil { + errs = append(errs, fmt.Errorf("count query failed: %w", err)) + return + } result.TotalRows = rowCount + finalResults = finalResults.Order(fmt.Sprintf("%s %s NULLS LAST", pq.QuoteIdentifier(sortField), sort)) finalResults = finalResults.Limit(spec.Pagination.PerPage).Offset(spec.Pagination.Page * spec.Pagination.PerPage) } diff --git a/pkg/db/query/test_queries.go b/pkg/db/query/test_queries.go index d3f948a939..89a39e4776 100644 --- a/pkg/db/query/test_queries.go +++ b/pkg/db/query/test_queries.go @@ -244,6 +244,7 @@ func LoadBugsForTest(dbc *db.DB, testName string, filterClosed bool) ([]models.B // flake_average shows the average flake percentage among all variants. // flake_standard_deviation shows the standard deviation of the flake percentage among variants. The number reflects how much flake percentage differs among variants. // delta_from_flake_average shows how much each variant differs from the flake_average. This can be used to identify outliers. + // SubqueryFilter wraps a filter function with metadata about what it targets. // Variant-only filters are applied only to passRates, not to stats, because // the stats subquery computes cross-variant averages and standard deviations @@ -253,10 +254,6 @@ type SubqueryFilter struct { VariantOnly bool } -func isVariantFilter(f SubqueryFilter) bool { - return f.VariantOnly -} - func TestsByNURPAndStandardDeviation(dbc *db.DB, release, table string, subqueryFilters ...SubqueryFilter) *gorm.DB { // 1. Create a virtual stats table. There is a single row for each test. stats := dbc.DB.Table(table). @@ -281,7 +278,7 @@ func TestsByNURPAndStandardDeviation(dbc *db.DB, release, table string, subquery // scanning the entire matview for the release. Variant-specific filters // only apply to passRates to preserve cross-variant stats semantics. for _, f := range subqueryFilters { - if isVariantFilter(f) { + if f.VariantOnly { passRates = f.Apply(passRates) } else { stats = f.Apply(stats) diff --git a/pkg/sippyserver/parameters.go b/pkg/sippyserver/parameters.go index 6f420c7a74..1e66713420 100644 --- a/pkg/sippyserver/parameters.go +++ b/pkg/sippyserver/parameters.go @@ -1,6 +1,7 @@ package sippyserver import ( + "fmt" "net/http" "strconv" "time" @@ -74,6 +75,8 @@ func getLimitParam(req *http.Request) int { return limit } +const maxPerPage = 1000 + func getPaginationParams(req *http.Request) (*apitype.Pagination, error) { perPage := req.URL.Query().Get("perPage") page := req.URL.Query().Get("page") @@ -82,6 +85,9 @@ func getPaginationParams(req *http.Request) (*apitype.Pagination, error) { if err != nil { return nil, err } + if perPageInt <= 0 || perPageInt > maxPerPage { + return nil, fmt.Errorf("perPage must be between 1 and %d", maxPerPage) + } pageNo := 0 if page != "" { @@ -89,6 +95,9 @@ func getPaginationParams(req *http.Request) (*apitype.Pagination, error) { if err != nil { return nil, err } + if pageNo < 0 { + return nil, fmt.Errorf("page must be non-negative") + } } return &apitype.Pagination{ diff --git a/pkg/sippyserver/parameters_test.go b/pkg/sippyserver/parameters_test.go new file mode 100644 index 0000000000..416e187320 --- /dev/null +++ b/pkg/sippyserver/parameters_test.go @@ -0,0 +1,97 @@ +package sippyserver + +import ( + "net/http" + "net/url" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestGetPaginationParams(t *testing.T) { + tests := []struct { + name string + query url.Values + wantNil bool + wantErr bool + wantPage int + wantPer int + }{ + { + name: "no params returns nil", + query: url.Values{}, + wantNil: true, + }, + { + name: "valid perPage and page", + query: url.Values{"perPage": {"25"}, "page": {"2"}}, + wantPer: 25, + wantPage: 2, + }, + { + name: "perPage without page defaults to page 0", + query: url.Values{"perPage": {"50"}}, + wantPer: 50, + wantPage: 0, + }, + { + name: "perPage=0 returns error", + query: url.Values{"perPage": {"0"}}, + wantErr: true, + }, + { + name: "negative perPage returns error", + query: url.Values{"perPage": {"-1"}}, + wantErr: true, + }, + { + name: "negative page returns error", + query: url.Values{"perPage": {"25"}, "page": {"-1"}}, + wantErr: true, + }, + { + name: "perPage exceeds max returns error", + query: url.Values{"perPage": {"10000"}}, + wantErr: true, + }, + { + name: "non-numeric perPage returns error", + query: url.Values{"perPage": {"abc"}}, + wantErr: true, + }, + { + name: "non-numeric page returns error", + query: url.Values{"perPage": {"25"}, "page": {"abc"}}, + wantErr: true, + }, + { + name: "max perPage is accepted", + query: url.Values{"perPage": {"1000"}}, + wantPer: 1000, + wantPage: 0, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + req := &http.Request{URL: &url.URL{RawQuery: tc.query.Encode()}} + result, err := getPaginationParams(req) + + if tc.wantErr { + assert.Error(t, err) + return + } + require.NoError(t, err) + + if tc.wantNil { + assert.Nil(t, result) + return + } + + require.NotNil(t, result) + assert.Equal(t, tc.wantPer, result.PerPage) + assert.Equal(t, tc.wantPage, result.Page) + }) + } +} diff --git a/pkg/sippyserver/server.go b/pkg/sippyserver/server.go index 212f611ff9..50cf333126 100644 --- a/pkg/sippyserver/server.go +++ b/pkg/sippyserver/server.go @@ -1151,9 +1151,15 @@ func (s *Server) jsonTestsReportFromBigQuery(w http.ResponseWriter, req *http.Re return } release := s.getParamOrFail(w, req, "release") - if release != "" { - api.PrintTestsJSONFromBigQuery(release, w, req, s.bigQueryClient) + if release == "" { + return + } + pagination, err := getPaginationParams(req) + if err != nil { + api.RespondWithJSON(http.StatusBadRequest, w, map[string]interface{}{"code": http.StatusBadRequest, "message": "invalid pagination params: " + err.Error()}) + return } + api.PrintTestsJSONFromBigQuery(release, w, req, s.bigQueryClient, pagination) } func (s *Server) jsonTestDetailsReportFromDB(w http.ResponseWriter, req *http.Request) { From 5ef2468d52aaa6d65a95b957f30efd7a30e70440 Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Wed, 6 May 2026 18:05:01 +0000 Subject: [PATCH 05/15] fix: gofmt parameters_test.go struct alignment Co-Authored-By: Claude Opus 4.6 --- pkg/sippyserver/parameters_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/sippyserver/parameters_test.go b/pkg/sippyserver/parameters_test.go index 416e187320..1f708a90be 100644 --- a/pkg/sippyserver/parameters_test.go +++ b/pkg/sippyserver/parameters_test.go @@ -11,12 +11,12 @@ import ( func TestGetPaginationParams(t *testing.T) { tests := []struct { - name string - query url.Values - wantNil bool - wantErr bool - wantPage int - wantPer int + name string + query url.Values + wantNil bool + wantErr bool + wantPage int + wantPer int }{ { name: "no params returns nil", From 50f24889ea56e56f041308e1d3a5b3029cb71734 Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Wed, 6 May 2026 18:23:22 +0000 Subject: [PATCH 06/15] fix: BigQuery pagination returns total row count, not page count The BQ path was setting TotalRows to len(testsResult) after limit, which gave the page count instead of the dataset total. Now captures the total before applying pagination slice. Co-Authored-By: Claude Opus 4.6 --- pkg/api/tests.go | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/pkg/api/tests.go b/pkg/api/tests.go index e292a134b4..f6cedcd0f2 100644 --- a/pkg/api/tests.go +++ b/pkg/api/tests.go @@ -393,21 +393,32 @@ func PrintTestsJSONFromBigQuery(release string, w http.ResponseWriter, req *http return } - testsResult := result.TestsAPIResultBQ.sort(req).limit(req) + sorted := result.TestsAPIResultBQ.sort(req) if result.Test != nil { - testsResult = append([]apitype.TestBQ{*result.Test}, testsResult...) + sorted = append([]apitype.TestBQ{*result.Test}, sorted...) } if pagination != nil { + totalRows := int64(len(sorted)) + start := pagination.Page * pagination.PerPage + end := start + pagination.PerPage + if start > int(totalRows) { + start = int(totalRows) + } + if end > int(totalRows) { + end = int(totalRows) + } RespondWithJSON(http.StatusOK, w, apitype.PaginationResult{ - Rows: testsResult, - TotalRows: int64(len(testsResult)), + Rows: sorted[start:end], + TotalRows: totalRows, PageSize: pagination.PerPage, Page: pagination.Page, }) return } + testsResult := sorted.limit(req) + RespondWithJSON(http.StatusOK, w, testsResult) } From 87f82f3bfc00a2f8fc1d1d42af2b010ea96cb003 Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Wed, 6 May 2026 14:43:11 -0400 Subject: [PATCH 07/15] perf: cache test results and paginate in memory The paginated tests API path was bypassing the cache and running the expensive three-layer nested subquery on every page/sort change. Now both paginated and non-paginated paths share the same cached result set (1 hour TTL), with sorting and pagination applied in memory. The collapsed result set is ~5k rows, making in-memory operations trivial. This also removes the separate COUNT(*) query that was doubling the DB work per paginated request. Co-Authored-By: Claude Opus 4.6 --- pkg/api/tests.go | 66 +++++++++++++++--------------------------------- 1 file changed, 21 insertions(+), 45 deletions(-) diff --git a/pkg/api/tests.go b/pkg/api/tests.go index f6cedcd0f2..cef4c1a89c 100644 --- a/pkg/api/tests.go +++ b/pkg/api/tests.go @@ -11,7 +11,6 @@ import ( "time" "cloud.google.com/go/bigquery" - "github.com/lib/pq" "github.com/pkg/errors" log "github.com/sirupsen/logrus" "google.golang.org/api/iterator" @@ -348,32 +347,34 @@ func PrintTestsJSONFromDB( return } + result, err := spec.buildTestsResultsFromPostgres(req.Context(), dbc, cacheClient) + if err != nil { + RespondWithJSON(http.StatusInternalServerError, w, map[string]interface{}{"code": http.StatusInternalServerError, "message": "Error building job report:" + err.Error()}) + return + } + + sorted := result.TestsAPIResult.sort(req) + if pagination != nil { - spec.Pagination = pagination - spec.SortField = param.SafeRead(req, "sortField") - spec.Sort = apitype.Sort(param.SafeRead(req, "sort")) - - result, errs := spec.buildTestsResultsPGGenerator(req.Context(), dbc, spec.matview()) - if errs != nil { - RespondWithJSON(http.StatusInternalServerError, w, map[string]interface{}{"code": http.StatusInternalServerError, "message": fmt.Sprintf("Error building test report: %v", errs)}) - return + totalRows := int64(len(sorted)) + start := pagination.Page * pagination.PerPage + end := start + pagination.PerPage + if start > int(totalRows) { + start = int(totalRows) + } + if end > int(totalRows) { + end = int(totalRows) } RespondWithJSON(http.StatusOK, w, apitype.PaginationResult{ - Rows: result.TestsAPIResult, - TotalRows: result.TotalRows, + Rows: sorted[start:end], + TotalRows: totalRows, PageSize: pagination.PerPage, Page: pagination.Page, }) return } - result, err := spec.buildTestsResultsFromPostgres(req.Context(), dbc, cacheClient) - if err != nil { - RespondWithJSON(http.StatusInternalServerError, w, map[string]interface{}{"code": http.StatusInternalServerError, "message": "Error building job report:" + err.Error()}) - return - } - - testsResult := result.TestsAPIResult.sort(req).limit(req) + testsResult := sorted.limit(req) if result.Test != nil { testsResult = append([]apitype.Test{*result.Test}, testsResult...) } @@ -460,9 +461,6 @@ type TestResultsSpec struct { Release, Period string Collapse, IncludeOverall bool Filter *filter.Filter - Pagination *apitype.Pagination - SortField string - Sort apitype.Sort } func (spec *TestResultsSpec) matview() string { @@ -474,8 +472,7 @@ func (spec *TestResultsSpec) matview() string { type testResults struct { TestsAPIResult - Test *apitype.Test - TotalRows int64 + Test *apitype.Test } const testResultsCacheDuration = time.Hour @@ -559,27 +556,6 @@ func (spec *TestResultsSpec) buildTestsResultsPGGenerator(ctx context.Context, d finalResults = processedFilter.ToSQL(finalResults, apitype.Test{}) } - if spec.Pagination != nil { - sortField := spec.SortField - if sortField == "" { - sortField = "current_pass_percentage" - } - sort := apitype.SortDescending - if spec.Sort == apitype.SortAscending { - sort = apitype.SortAscending - } - - var rowCount int64 - if err := finalResults.Count(&rowCount).Error; err != nil { - errs = append(errs, fmt.Errorf("count query failed: %w", err)) - return - } - result.TotalRows = rowCount - - finalResults = finalResults.Order(fmt.Sprintf("%s %s NULLS LAST", pq.QuoteIdentifier(sortField), sort)) - finalResults = finalResults.Limit(spec.Pagination.PerPage).Offset(spec.Pagination.Page * spec.Pagination.PerPage) - } - frr := finalResults.Scan(&testReports) if frr.Error != nil { log.WithError(finalResults.Error).Error("error querying test reports") @@ -589,7 +565,7 @@ func (spec *TestResultsSpec) buildTestsResultsPGGenerator(ctx context.Context, d } var overallTest *apitype.Test - if spec.IncludeOverall && spec.Pagination == nil { + if spec.IncludeOverall { finalResults := dbc.DB.Table("(?) as final_results", finalResults) finalResults = finalResults.Select(query.QueryTestSummer) summaryResult := dbc.DB.Table("(?) as overall", finalResults).Select(query.QueryTestSummarizer) From cfb3754ef69ea75e065a3f0c384a4a3af3c2e52f Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Wed, 6 May 2026 14:57:38 -0400 Subject: [PATCH 08/15] perf: cache unfiltered test results with cache primer support Cache the full unfiltered test result set and apply filters in memory. This means any filter, sort, or page change is served instantly from cache without hitting the database. The cache TTL is increased to 4 hours to match the cache primer schedule. Also adds a test-report-cache data loader that can be used with the cache primer cronjob (--loader=test-report-cache) to warm the test results cache for all releases on both default and twoDay periods. Co-Authored-By: Claude Opus 4.6 --- cmd/sippy/load.go | 11 ++++ pkg/api/tests.go | 38 ++++++++++++-- .../testreportcacheloader.go | 52 +++++++++++++++++++ 3 files changed, 97 insertions(+), 4 deletions(-) create mode 100644 pkg/dataloader/testreportcacheloader/testreportcacheloader.go diff --git a/cmd/sippy/load.go b/cmd/sippy/load.go index 49e8346f69..c5236d43d3 100644 --- a/cmd/sippy/load.go +++ b/cmd/sippy/load.go @@ -39,6 +39,7 @@ import ( "github.com/openshift/sippy/pkg/dataloader/prowloader/github" "github.com/openshift/sippy/pkg/dataloader/releaseloader" "github.com/openshift/sippy/pkg/dataloader/testownershiploader" + "github.com/openshift/sippy/pkg/dataloader/testreportcacheloader" "github.com/openshift/sippy/pkg/db" "github.com/openshift/sippy/pkg/flags" "github.com/openshift/sippy/pkg/github/commenter" @@ -318,6 +319,16 @@ func NewLoadCommand() *cobra.Command { loaders = append(loaders, fgLoader) } + if l == "test-report-cache" { + if dbErr != nil { + return dbErr + } + if cacheErr != nil { + return errors.Wrap(cacheErr, "couldn't get cache client") + } + loaders = append(loaders, testreportcacheloader.New(dbc, cacheClient)) + } + } // Run loaders with the metrics wrapper diff --git a/pkg/api/tests.go b/pkg/api/tests.go index cef4c1a89c..f1613fca3d 100644 --- a/pkg/api/tests.go +++ b/pkg/api/tests.go @@ -227,6 +227,19 @@ func GetTestDurationsFromDB(dbc *db.DB, release, test string, filters *filter.Fi type TestsAPIResult []apitype.Test +func (tests TestsAPIResult) filter(f *filter.Filter) TestsAPIResult { + if f == nil || len(f.Items) == 0 { + return tests + } + var result TestsAPIResult + for _, t := range tests { + if match, err := f.Filter(t); err == nil && match { + result = append(result, t) + } + } + return result +} + func (tests TestsAPIResult) sort(req *http.Request) TestsAPIResult { sortField := param.SafeRead(req, "sortField") sort := param.SafeRead(req, "sort") @@ -347,13 +360,19 @@ func PrintTestsJSONFromDB( return } + // Cache the unfiltered result set so that filter/sort/page changes + // are served from cache without hitting the database again. + requestFilter := spec.Filter + spec.Filter = nil + result, err := spec.buildTestsResultsFromPostgres(req.Context(), dbc, cacheClient) if err != nil { RespondWithJSON(http.StatusInternalServerError, w, map[string]interface{}{"code": http.StatusInternalServerError, "message": "Error building job report:" + err.Error()}) return } - sorted := result.TestsAPIResult.sort(req) + filtered := result.TestsAPIResult.filter(requestFilter) + sorted := filtered.sort(req) if pagination != nil { totalRows := int64(len(sorted)) @@ -475,7 +494,18 @@ type testResults struct { Test *apitype.Test } -const testResultsCacheDuration = time.Hour +const TestResultsCacheDuration = 4 * time.Hour + +// PrimeTestResultsCache warms the cache for the given release/period/collapse combination. +func PrimeTestResultsCache(ctx context.Context, dbc *db.DB, cacheClient cache.Cache, release, period string, collapse bool) error { + spec := TestResultsSpec{ + Release: release, + Period: period, + Collapse: collapse, + } + _, err := spec.buildTestsResultsFromPostgres(ctx, dbc, cacheClient) + return err +} func (spec *TestResultsSpec) buildTestsResultsFromPostgres(ctx context.Context, dbc *db.DB, cacheClient cache.Cache) (testResults, error) { matview := spec.matview() @@ -485,7 +515,7 @@ func (spec *TestResultsSpec) buildTestsResultsFromPostgres(ctx context.Context, } result, errs := GetDataFromCacheOrMatview(ctx, cacheClient, NewCacheSpec(spec, "PostgresTestsResults~", nil), - matview, testResultsCacheDuration, + matview, TestResultsCacheDuration, generator, testResults{}, ) @@ -600,7 +630,7 @@ func (spec *TestResultsSpec) buildTestsResultsFromBigQuery(ctx context.Context, result, errs := GetDataFromCacheOrGenerate[testResultsBQ]( ctx, bqc.Cache, - cache.RequestOptions{Expiry: testResultsCacheDuration}, + cache.RequestOptions{Expiry: TestResultsCacheDuration}, NewCacheSpec(spec, "BigQueryTestsResults~", nil), generator, testResultsBQ{}) diff --git a/pkg/dataloader/testreportcacheloader/testreportcacheloader.go b/pkg/dataloader/testreportcacheloader/testreportcacheloader.go new file mode 100644 index 0000000000..d7d131d3d1 --- /dev/null +++ b/pkg/dataloader/testreportcacheloader/testreportcacheloader.go @@ -0,0 +1,52 @@ +package testreportcacheloader + +import ( + "context" + + log "github.com/sirupsen/logrus" + + "github.com/openshift/sippy/pkg/api" + "github.com/openshift/sippy/pkg/apis/cache" + "github.com/openshift/sippy/pkg/db" +) + +type testReportCacheLoader struct { + dbc *db.DB + cacheClient cache.Cache + errs []error +} + +func New(dbc *db.DB, cacheClient cache.Cache) *testReportCacheLoader { + return &testReportCacheLoader{ + dbc: dbc, + cacheClient: cacheClient, + } +} + +func (l *testReportCacheLoader) Name() string { + return "test-report-cache" +} + +func (l *testReportCacheLoader) Load() { + ctx := context.Background() + + var releases []string + if err := l.dbc.DB.Raw("SELECT DISTINCT release FROM prow_test_report_7d_matview").Scan(&releases).Error; err != nil { + l.errs = append(l.errs, err) + return + } + + for _, release := range releases { + for _, period := range []string{"default", "twoDay"} { + log.Infof("Priming test report cache for release=%s period=%s collapse=true", release, period) + if err := api.PrimeTestResultsCache(ctx, l.dbc, l.cacheClient, release, period, true); err != nil { + log.WithError(err).Errorf("Failed to prime test report cache for release=%s period=%s collapse=true", release, period) + l.errs = append(l.errs, err) + } + } + } +} + +func (l *testReportCacheLoader) Errors() []error { + return l.errs +} From ae7d76f56999f8d12fc7ad157afcdd4266a9f652 Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Wed, 6 May 2026 19:56:52 +0000 Subject: [PATCH 09/15] perf: prime test report cache for both collapsed and non-collapsed results Only prime cache for OCP development releases (no GA date, has payloadTags capability) to avoid wasting time on GA/OKD releases. Co-Authored-By: Claude Opus 4.6 --- cmd/sippy/load.go | 2 +- .../testreportcacheloader.go | 39 +++++++- .../testreportcacheloader_test.go | 93 +++++++++++++++++++ 3 files changed, 128 insertions(+), 6 deletions(-) create mode 100644 pkg/dataloader/testreportcacheloader/testreportcacheloader_test.go diff --git a/cmd/sippy/load.go b/cmd/sippy/load.go index c5236d43d3..04a3ab8ed5 100644 --- a/cmd/sippy/load.go +++ b/cmd/sippy/load.go @@ -326,7 +326,7 @@ func NewLoadCommand() *cobra.Command { if cacheErr != nil { return errors.Wrap(cacheErr, "couldn't get cache client") } - loaders = append(loaders, testreportcacheloader.New(dbc, cacheClient)) + loaders = append(loaders, testreportcacheloader.New(dbc, cacheClient, releaseConfigs)) } } diff --git a/pkg/dataloader/testreportcacheloader/testreportcacheloader.go b/pkg/dataloader/testreportcacheloader/testreportcacheloader.go index d7d131d3d1..630e3a250d 100644 --- a/pkg/dataloader/testreportcacheloader/testreportcacheloader.go +++ b/pkg/dataloader/testreportcacheloader/testreportcacheloader.go @@ -7,19 +7,22 @@ import ( "github.com/openshift/sippy/pkg/api" "github.com/openshift/sippy/pkg/apis/cache" + v1 "github.com/openshift/sippy/pkg/apis/sippy/v1" "github.com/openshift/sippy/pkg/db" ) type testReportCacheLoader struct { dbc *db.DB cacheClient cache.Cache + releases []v1.Release errs []error } -func New(dbc *db.DB, cacheClient cache.Cache) *testReportCacheLoader { +func New(dbc *db.DB, cacheClient cache.Cache, releases []v1.Release) *testReportCacheLoader { return &testReportCacheLoader{ dbc: dbc, cacheClient: cacheClient, + releases: releases, } } @@ -36,17 +39,43 @@ func (l *testReportCacheLoader) Load() { return } + devReleases := l.developmentReleases() for _, release := range releases { + if !devReleases[release] { + log.Infof("Skipping test report cache for non-development release=%s", release) + continue + } for _, period := range []string{"default", "twoDay"} { - log.Infof("Priming test report cache for release=%s period=%s collapse=true", release, period) - if err := api.PrimeTestResultsCache(ctx, l.dbc, l.cacheClient, release, period, true); err != nil { - log.WithError(err).Errorf("Failed to prime test report cache for release=%s period=%s collapse=true", release, period) - l.errs = append(l.errs, err) + for _, collapse := range []bool{true, false} { + log.Infof("Priming test report cache for release=%s period=%s collapse=%v", release, period, collapse) + if err := api.PrimeTestResultsCache(ctx, l.dbc, l.cacheClient, release, period, collapse); err != nil { + log.WithError(err).Errorf("Failed to prime test report cache for release=%s period=%s collapse=%v", release, period, collapse) + l.errs = append(l.errs, err) + } } } } } +// developmentReleases returns a set of release names that are OCP development +// releases (have payloadTags capability and no GA date). +func (l *testReportCacheLoader) developmentReleases() map[string]bool { + devReleases := make(map[string]bool) + for _, r := range l.releases { + if r.Product != "OCP" { + continue + } + if r.GADate != nil { + continue + } + if !r.Capabilities[v1.PayloadTagsCap] { + continue + } + devReleases[r.Release] = true + } + return devReleases +} + func (l *testReportCacheLoader) Errors() []error { return l.errs } diff --git a/pkg/dataloader/testreportcacheloader/testreportcacheloader_test.go b/pkg/dataloader/testreportcacheloader/testreportcacheloader_test.go new file mode 100644 index 0000000000..b66734b873 --- /dev/null +++ b/pkg/dataloader/testreportcacheloader/testreportcacheloader_test.go @@ -0,0 +1,93 @@ +package testreportcacheloader + +import ( + "testing" + "time" + + v1 "github.com/openshift/sippy/pkg/apis/sippy/v1" + "github.com/stretchr/testify/assert" +) + +func TestDevelopmentReleases(t *testing.T) { + gaDate := time.Date(2024, 10, 1, 0, 0, 0, 0, time.UTC) + + tests := []struct { + name string + releases []v1.Release + want map[string]bool + }{ + { + name: "filters to dev OCP releases only", + releases: []v1.Release{ + { + Release: "4.18", + Product: "OCP", + Capabilities: map[v1.ReleaseCapability]bool{v1.PayloadTagsCap: true}, + }, + { + Release: "4.17", + Product: "OCP", + GADate: &gaDate, + Capabilities: map[v1.ReleaseCapability]bool{v1.PayloadTagsCap: true}, + }, + { + Release: "Presubmits", + Product: "OCP", + Capabilities: map[v1.ReleaseCapability]bool{v1.SippyClassicCap: true}, + }, + { + Release: "4.19", + Product: "OCP", + Capabilities: map[v1.ReleaseCapability]bool{v1.PayloadTagsCap: true, v1.SippyClassicCap: true}, + }, + }, + want: map[string]bool{ + "4.18": true, + "4.19": true, + }, + }, + { + name: "excludes OKD releases", + releases: []v1.Release{ + { + Release: "4.18", + Product: "OCP", + Capabilities: map[v1.ReleaseCapability]bool{v1.PayloadTagsCap: true}, + }, + { + Release: "4.18-okd", + Product: "OKD", + Capabilities: map[v1.ReleaseCapability]bool{v1.PayloadTagsCap: true}, + }, + }, + want: map[string]bool{ + "4.18": true, + }, + }, + { + name: "empty releases", + releases: nil, + want: map[string]bool{}, + }, + { + name: "all GA releases returns empty", + releases: []v1.Release{ + { + Release: "4.17", + Product: "OCP", + GADate: &gaDate, + Capabilities: map[v1.ReleaseCapability]bool{v1.PayloadTagsCap: true}, + }, + }, + want: map[string]bool{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + l := &testReportCacheLoader{releases: tt.releases} + got := l.developmentReleases() + assert.Equal(t, tt.want, got) + }) + } +} From ef59bb6d311a55fdd3642d0e0b1f19cb4e0663a4 Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Wed, 6 May 2026 20:06:19 +0000 Subject: [PATCH 10/15] perf: force refresh cache in test report primer and add benchmark script PrimeTestResultsCache now bypasses the cache read path entirely, always regenerating from the database and writing the fresh result. Previously it went through GetDataFromCacheOrMatview which would return stale cached data if the matview hadn't been refreshed yet. Also adds hack/bench-test-api.sh for comparing prod vs local API response times across various query patterns. Co-Authored-By: Claude Opus 4.6 --- hack/bench-test-api.sh | 65 ++++++++++++++++++++++++++++++++++++++++++ pkg/api/tests.go | 24 ++++++++++++++-- 2 files changed, 86 insertions(+), 3 deletions(-) create mode 100755 hack/bench-test-api.sh diff --git a/hack/bench-test-api.sh b/hack/bench-test-api.sh new file mode 100755 index 0000000000..15fa7c1602 --- /dev/null +++ b/hack/bench-test-api.sh @@ -0,0 +1,65 @@ +#!/usr/bin/env bash +# +# Benchmark the /api/tests endpoint across production and local Sippy instances. +# Usage: ./hack/bench-test-api.sh [local_base_url] +# local_base_url defaults to http://localhost:8080 + +set -euo pipefail + +LOCAL="${1:-http://localhost:8080}" +PROD="https://sippy.dptools.openshift.org" +RELEASE="4.19" +RUNS=3 + +# ANSI colors +bold='\033[1m' +reset='\033[0m' +green='\033[32m' +yellow='\033[33m' + +# Each entry: "label|query_params" +queries=( + "collapsed (default)|release=${RELEASE}&collapse=true" + "uncollapsed|release=${RELEASE}&collapse=false" + "collapsed + twoDay|release=${RELEASE}&collapse=true&period=twoDay" + "uncollapsed + twoDay|release=${RELEASE}&collapse=false&period=twoDay" + "collapsed + filter name contains sig-node|release=${RELEASE}&collapse=true&filter=$(python3 -c 'import urllib.parse,json; print(urllib.parse.quote(json.dumps({"items":[{"columnField":"name","operatorValue":"contains","value":"sig-node"}]})))')" + "uncollapsed + filter name contains sig-node|release=${RELEASE}&collapse=false&filter=$(python3 -c 'import urllib.parse,json; print(urllib.parse.quote(json.dumps({"items":[{"columnField":"name","operatorValue":"contains","value":"sig-node"}]})))')" + "collapsed + filter runs > 14|release=${RELEASE}&collapse=true&filter=$(python3 -c 'import urllib.parse,json; print(urllib.parse.quote(json.dumps({"items":[{"columnField":"current_runs","operatorValue":">","value":"14"}]})))')" + "uncollapsed + paginated page 0|release=${RELEASE}&collapse=false&perPage=25&page=0" +) + +time_url() { + local url="$1" + local total=0 + local times=() + for i in $(seq 1 "$RUNS"); do + local t + t=$(curl -so /dev/null -w '%{time_total}' "$url" 2>/dev/null) + times+=("$t") + total=$(echo "$total + $t" | bc) + done + local avg + avg=$(echo "scale=3; $total / $RUNS" | bc) + echo "$avg" +} + +printf "${bold}%-50s %12s %12s %12s${reset}\n" "Query" "Prod (s)" "Local (s)" "Speedup" +printf "%-50s %12s %12s %12s\n" "-----" "--------" "---------" "-------" + +for entry in "${queries[@]}"; do + IFS='|' read -r label params <<< "$entry" + prod_url="${PROD}/api/tests?${params}" + local_url="${LOCAL}/api/tests?${params}" + + prod_time=$(time_url "$prod_url") + local_time=$(time_url "$local_url") + + if (( $(echo "$local_time > 0" | bc -l) )); then + speedup=$(echo "scale=1; $prod_time / $local_time" | bc 2>/dev/null || echo "n/a") + else + speedup="n/a" + fi + + printf "%-50s %11.3fs %11.3fs %11sx\n" "$label" "$prod_time" "$local_time" "$speedup" +done diff --git a/pkg/api/tests.go b/pkg/api/tests.go index f1613fca3d..fa391fa6da 100644 --- a/pkg/api/tests.go +++ b/pkg/api/tests.go @@ -496,15 +496,33 @@ type testResults struct { const TestResultsCacheDuration = 4 * time.Hour -// PrimeTestResultsCache warms the cache for the given release/period/collapse combination. +// PrimeTestResultsCache unconditionally regenerates and caches results for the +// given release/period/collapse combination, replacing any existing entry. func PrimeTestResultsCache(ctx context.Context, dbc *db.DB, cacheClient cache.Cache, release, period string, collapse bool) error { spec := TestResultsSpec{ Release: release, Period: period, Collapse: collapse, } - _, err := spec.buildTestsResultsFromPostgres(ctx, dbc, cacheClient) - return err + matview := spec.matview() + result, errs := spec.buildTestsResultsPGGenerator(ctx, dbc, matview) + if len(errs) > 0 { + return fmt.Errorf("error(s) generating test results: %v", errs) + } + cacheSpec := NewCacheSpec(spec, "PostgresTestsResults~", nil) + cacheKey, err := cacheSpec.GetCacheKey() + if err != nil { + return fmt.Errorf("error getting cache key: %v", err) + } + cacheVal := struct { + Val testResults + Timestamp time.Time + }{ + Val: result, + Timestamp: time.Now().UTC(), + } + CacheSet(ctx, cacheClient, cacheVal, cacheKey, TestResultsCacheDuration) + return nil } func (spec *TestResultsSpec) buildTestsResultsFromPostgres(ctx context.Context, dbc *db.DB, cacheClient cache.Cache) (testResults, error) { From fe688043643628e3e66380d0eee122c6af5b972e Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Wed, 6 May 2026 20:06:43 +0000 Subject: [PATCH 11/15] chore: remove benchmark script The production API is too slow to complete within curl's timeout, making the comparison benchmark impractical. Co-Authored-By: Claude Opus 4.6 --- hack/bench-test-api.sh | 65 ------------------------------------------ 1 file changed, 65 deletions(-) delete mode 100755 hack/bench-test-api.sh diff --git a/hack/bench-test-api.sh b/hack/bench-test-api.sh deleted file mode 100755 index 15fa7c1602..0000000000 --- a/hack/bench-test-api.sh +++ /dev/null @@ -1,65 +0,0 @@ -#!/usr/bin/env bash -# -# Benchmark the /api/tests endpoint across production and local Sippy instances. -# Usage: ./hack/bench-test-api.sh [local_base_url] -# local_base_url defaults to http://localhost:8080 - -set -euo pipefail - -LOCAL="${1:-http://localhost:8080}" -PROD="https://sippy.dptools.openshift.org" -RELEASE="4.19" -RUNS=3 - -# ANSI colors -bold='\033[1m' -reset='\033[0m' -green='\033[32m' -yellow='\033[33m' - -# Each entry: "label|query_params" -queries=( - "collapsed (default)|release=${RELEASE}&collapse=true" - "uncollapsed|release=${RELEASE}&collapse=false" - "collapsed + twoDay|release=${RELEASE}&collapse=true&period=twoDay" - "uncollapsed + twoDay|release=${RELEASE}&collapse=false&period=twoDay" - "collapsed + filter name contains sig-node|release=${RELEASE}&collapse=true&filter=$(python3 -c 'import urllib.parse,json; print(urllib.parse.quote(json.dumps({"items":[{"columnField":"name","operatorValue":"contains","value":"sig-node"}]})))')" - "uncollapsed + filter name contains sig-node|release=${RELEASE}&collapse=false&filter=$(python3 -c 'import urllib.parse,json; print(urllib.parse.quote(json.dumps({"items":[{"columnField":"name","operatorValue":"contains","value":"sig-node"}]})))')" - "collapsed + filter runs > 14|release=${RELEASE}&collapse=true&filter=$(python3 -c 'import urllib.parse,json; print(urllib.parse.quote(json.dumps({"items":[{"columnField":"current_runs","operatorValue":">","value":"14"}]})))')" - "uncollapsed + paginated page 0|release=${RELEASE}&collapse=false&perPage=25&page=0" -) - -time_url() { - local url="$1" - local total=0 - local times=() - for i in $(seq 1 "$RUNS"); do - local t - t=$(curl -so /dev/null -w '%{time_total}' "$url" 2>/dev/null) - times+=("$t") - total=$(echo "$total + $t" | bc) - done - local avg - avg=$(echo "scale=3; $total / $RUNS" | bc) - echo "$avg" -} - -printf "${bold}%-50s %12s %12s %12s${reset}\n" "Query" "Prod (s)" "Local (s)" "Speedup" -printf "%-50s %12s %12s %12s\n" "-----" "--------" "---------" "-------" - -for entry in "${queries[@]}"; do - IFS='|' read -r label params <<< "$entry" - prod_url="${PROD}/api/tests?${params}" - local_url="${LOCAL}/api/tests?${params}" - - prod_time=$(time_url "$prod_url") - local_time=$(time_url "$local_url") - - if (( $(echo "$local_time > 0" | bc -l) )); then - speedup=$(echo "scale=1; $prod_time / $local_time" | bc 2>/dev/null || echo "n/a") - else - speedup="n/a" - fi - - printf "%-50s %11.3fs %11.3fs %11sx\n" "$label" "$prod_time" "$local_time" "$speedup" -done From 118186f99170d4b8138a01086d57b8be7d250382 Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Wed, 6 May 2026 20:07:39 +0000 Subject: [PATCH 12/15] perf: extend test results cache TTL to 5 hours The cache primer runs every 4 hours, so a 4-hour TTL risked cache expiry on the boundary before the next primer run. Extending to 5 hours ensures primed entries never expire between runs. Co-Authored-By: Claude Opus 4.6 --- pkg/api/tests.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/api/tests.go b/pkg/api/tests.go index fa391fa6da..7de3a3eccf 100644 --- a/pkg/api/tests.go +++ b/pkg/api/tests.go @@ -494,7 +494,7 @@ type testResults struct { Test *apitype.Test } -const TestResultsCacheDuration = 4 * time.Hour +const TestResultsCacheDuration = 5 * time.Hour // PrimeTestResultsCache unconditionally regenerates and caches results for the // given release/period/collapse combination, replacing any existing entry. From 73a73c24d114bcfb84a404c05f98c4f5f35b828e Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Wed, 6 May 2026 20:18:51 +0000 Subject: [PATCH 13/15] fix: set IncludeOverall in cache primer to match API defaults The API defaults IncludeOverall to true when collapse is false, but the primer was leaving it as false. This caused a cache key mismatch, resulting in cache misses for uncollapsed requests. Co-Authored-By: Claude Opus 4.6 --- pkg/api/tests.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/pkg/api/tests.go b/pkg/api/tests.go index 7de3a3eccf..c838cdd724 100644 --- a/pkg/api/tests.go +++ b/pkg/api/tests.go @@ -500,9 +500,10 @@ const TestResultsCacheDuration = 5 * time.Hour // given release/period/collapse combination, replacing any existing entry. func PrimeTestResultsCache(ctx context.Context, dbc *db.DB, cacheClient cache.Cache, release, period string, collapse bool) error { spec := TestResultsSpec{ - Release: release, - Period: period, - Collapse: collapse, + Release: release, + Period: period, + Collapse: collapse, + IncludeOverall: !collapse, } matview := spec.matview() result, errs := spec.buildTestsResultsPGGenerator(ctx, dbc, matview) From 037d1f37bd2004fcfa0235a972f140fb317adc88 Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Wed, 6 May 2026 20:19:23 +0000 Subject: [PATCH 14/15] fix: nil check cache client in PrimeTestResultsCache The old code path through GetDataFromCacheOrMatview handled nil cache gracefully, but the direct write path panicked on nil. Return an error instead. Co-Authored-By: Claude Opus 4.6 --- pkg/api/tests.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/api/tests.go b/pkg/api/tests.go index c838cdd724..1059de9350 100644 --- a/pkg/api/tests.go +++ b/pkg/api/tests.go @@ -505,6 +505,9 @@ func PrimeTestResultsCache(ctx context.Context, dbc *db.DB, cacheClient cache.Ca Collapse: collapse, IncludeOverall: !collapse, } + if cacheClient == nil { + return fmt.Errorf("cache client is required for priming") + } matview := spec.matview() result, errs := spec.buildTestsResultsPGGenerator(ctx, dbc, matview) if len(errs) > 0 { From bd15f5cb26b062b738f614bbe5bcdc4bf29721c7 Mon Sep 17 00:00:00 2001 From: Stephen Benjamin Date: Wed, 6 May 2026 20:47:14 +0000 Subject: [PATCH 15/15] perf: add opt-in gzip compression for cache entries Add WithCompression() option to CacheSet that gzip-compresses the JSON before writing to Redis. Both cache read paths auto-detect gzip via magic header bytes and decompress transparently. Only the test results cache primer uses compression for now, as the uncollapsed result set is ~1GB of JSON. All other callers are unchanged. Co-Authored-By: Claude Opus 4.6 --- pkg/api/cache.go | 98 ++++++++++++++++++++++++++++++++++++++++++------ pkg/api/tests.go | 2 +- 2 files changed, 87 insertions(+), 13 deletions(-) diff --git a/pkg/api/cache.go b/pkg/api/cache.go index 20cf2ab5c7..1c9d177b0b 100644 --- a/pkg/api/cache.go +++ b/pkg/api/cache.go @@ -1,9 +1,12 @@ package api import ( + "bytes" + "compress/gzip" "context" "encoding/json" "fmt" + "io" "reflect" "strings" "time" @@ -15,6 +18,17 @@ import ( var defaultCacheDuration = 8 * time.Hour +func formatBytes(b int) string { + switch { + case b >= 1<<20: + return fmt.Sprintf("%.1f MB", float64(b)/float64(1<<20)) + case b >= 1<<10: + return fmt.Sprintf("%.1f KB", float64(b)/float64(1<<10)) + default: + return fmt.Sprintf("%d B", b) + } +} + // CacheSpec specifies caching parameters for an individual query type CacheSpec struct { // function to turn the specified key struct into cacheable bytes @@ -94,8 +108,12 @@ func GetDataFromCacheOrGenerate[T any]( "key": string(cacheKey), "type": reflect.TypeOf(defaultVal).String(), }).Infof("cache hit") + decompressed, err := cacheDecompress(res) + if err != nil { + return defaultVal, []error{errors.WithMessagef(err, "failed to decompress cached item. cacheKey=%+v", cacheKey)} + } var cr T - if err := json.Unmarshal(res, &cr); err != nil { + if err := json.Unmarshal(decompressed, &cr); err != nil { return defaultVal, []error{errors.WithMessagef(err, "failed to unmarshal cached item. cacheKey=%+v", cacheKey)} } return cr, nil @@ -118,20 +136,73 @@ func GetDataFromCacheOrGenerate[T any]( return generateFn(ctx) } -func CacheSet[T any](ctx context.Context, c cache.Cache, result T, cacheKey []byte, cacheDuration time.Duration) { +// gzipMagic is prepended to compressed cache entries so reads can detect them. +var gzipMagic = []byte{0x1f, 0x8b} + +func CacheSet[T any](ctx context.Context, c cache.Cache, result T, cacheKey []byte, cacheDuration time.Duration, opts ...CacheSetOption) { + o := cacheSetOptions{} + for _, opt := range opts { + opt(&o) + } + cr, err := json.Marshal(result) - if err == nil { - if err := c.Set(ctx, string(cacheKey), cr, cacheDuration); err != nil { - if strings.Contains(err.Error(), "connection refused") { - logrus.WithError(err).Fatalf("redis URL specified but got connection refused, exiting due to cost issues in this configuration") - } - logrus.WithError(err).Warningf("couldn't persist new item to cache") - } else { - logrus.Debugf("cache set for cache key: %s", string(cacheKey)) + if err != nil { + logrus.WithError(err).Errorf("Failed to marshall cache item") + return + } + + stored := cr + if o.compress { + var buf bytes.Buffer + gz := gzip.NewWriter(&buf) + if _, err := gz.Write(cr); err != nil { + logrus.WithError(err).Errorf("Failed to gzip cache item") + return + } + if err := gz.Close(); err != nil { + logrus.WithError(err).Errorf("Failed to close gzip writer") + return } + stored = buf.Bytes() + logrus.Infof("cache set: key=%s raw=%s compressed=%s", string(cacheKey), formatBytes(len(cr)), formatBytes(len(stored))) } else { - logrus.WithError(err).Errorf("Failed to marshall cache item: %v", result) + logrus.Infof("cache set: key=%s size=%s", string(cacheKey), formatBytes(len(cr))) + } + + if err := c.Set(ctx, string(cacheKey), stored, cacheDuration); err != nil { + if strings.Contains(err.Error(), "connection refused") { + logrus.WithError(err).Fatalf("redis URL specified but got connection refused, exiting due to cost issues in this configuration") + } + logrus.WithError(err).Warningf("couldn't persist new item to cache, size=%s", formatBytes(len(stored))) + } +} + +type cacheSetOptions struct { + compress bool +} + +// CacheSetOption configures optional CacheSet behavior. +type CacheSetOption func(*cacheSetOptions) + +// WithCompression enables gzip compression for the cache entry. +func WithCompression() CacheSetOption { + return func(o *cacheSetOptions) { + o.compress = true + } +} + +// cacheDecompress returns decompressed bytes if the input is gzip-compressed, +// otherwise returns the input unchanged. +func cacheDecompress(data []byte) ([]byte, error) { + if len(data) < 2 || data[0] != gzipMagic[0] || data[1] != gzipMagic[1] { + return data, nil + } + gz, err := gzip.NewReader(bytes.NewReader(data)) + if err != nil { + return nil, fmt.Errorf("failed to create gzip reader: %w", err) } + defer gz.Close() + return io.ReadAll(gz) } func CalculateRoundedCacheDuration(cacheOptions cache.RequestOptions) time.Duration { @@ -198,7 +269,10 @@ func GetDataFromCacheOrMatview[T any](ctx context.Context, "type": reflect.TypeOf(defaultVal).String(), }).Debugf("cache hit") - if err := json.Unmarshal(cached, &cacheVal); err != nil { + decompressed, decompErr := cacheDecompress(cached) + if decompErr != nil { + logrus.WithError(decompErr).Warnf("failed to decompress cached item. cacheKey=%+v", cacheKey) + } else if err := json.Unmarshal(decompressed, &cacheVal); err != nil { logrus.WithError(err).Warnf("failed to unmarshal cached item. cacheKey=%+v", cacheKey) // fall through to generate the data instead } else { diff --git a/pkg/api/tests.go b/pkg/api/tests.go index 1059de9350..7e9c64e21c 100644 --- a/pkg/api/tests.go +++ b/pkg/api/tests.go @@ -525,7 +525,7 @@ func PrimeTestResultsCache(ctx context.Context, dbc *db.DB, cacheClient cache.Ca Val: result, Timestamp: time.Now().UTC(), } - CacheSet(ctx, cacheClient, cacheVal, cacheKey, TestResultsCacheDuration) + CacheSet(ctx, cacheClient, cacheVal, cacheKey, TestResultsCacheDuration, WithCompression()) return nil }