TRT-2741: Phase 2 - Replace prow_test_report matviews with cumulative summary queries#3769
TRT-2741: Phase 2 - Replace prow_test_report matviews with cumulative summary queries#3769mstaeble wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
@mstaeble: This pull request references TRT-2741 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mstaeble The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Tip For best results, initiate chat on the files or code changes. No action needed here — this is an automated notice from the |
mstaeble
force-pushed
the
worktree-trt-2741-phase2
branch
from
ce0aff4 to
d09b3c0
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
WalkthroughTest reporting is migrated from report materialized views to cumulative-summary query builders. APIs, variant queries, benchmarks, feature-gate database wiring, cache usage, and materialized-view configuration are updated accordingly. ChangesCumulative reporting migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ReportAPI
participant TestReportQuery
participant CumulativeSummaries
participant ReportMetadata
ReportAPI->>TestReportQuery: build sample/base report query
TestReportQuery->>CumulativeSummaries: resolve dates and calculate prefix-sum differences
TestReportQuery->>ReportMetadata: join tests, suites, ownership, variants, and open bugs
TestReportQuery-->>ReportAPI: return report query
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 18 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (18 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/db/query/feature_gates.go (1)
78-87: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winWrap errors with context using
fmt.Errorfand%w.As per coding guidelines, do not return unwrapped errors. Please wrap them with contextual information to aid in debugging.
🐛 Proposed fix to wrap errors
q, err := filter.FilterableDBResult(table, filterOpts, api.FeatureGate{}) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to apply filter: %w", err) } results := make([]api.FeatureGate, 0) tx := q.Scan(&results) if tx.Error != nil { - return nil, tx.Error + return nil, fmt.Errorf("failed to scan feature gates: %w", tx.Error) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/db/query/feature_gates.go` around lines 78 - 87, Update the query flow around FilterableDBResult and q.Scan in the relevant feature-gate function to wrap both returned errors with contextual messages using fmt.Errorf and the %w verb, preserving the original errors for unwrapping while identifying whether filtering or scanning failed.Source: Coding guidelines
🧹 Nitpick comments (2)
pkg/db/query/cumulative_query.go (2)
51-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider unit tests for the new pure helper functions.
escapeLikeMetachars,PeriodsForReportType, andresolveDateRangesare pure/deterministic and easy to unit test (date-boundary math is especially error-prone). No test file was included in this review scope forcumulative_query.go.Also applies to: 120-142
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/db/query/cumulative_query.go` around lines 51 - 54, Add unit tests for the pure helpers escapeLikeMetachars, PeriodsForReportType, and resolveDateRanges in cumulative_query.go. Cover LIKE metacharacter escaping, each report type’s period behavior, and date-range boundary calculations including representative edge cases, while keeping tests deterministic and isolated from external dependencies.
49-254: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd inline comments for major query sections in the new builders.
testReportPreAgg,TestReportQuery, andTestReportQueryCollapsedhave function-level docstrings but no inline comments marking the purpose of individual JOINs/window-function blocks (e.g., whatm/sself-joins represent, why the window functions are partitioned as they are). As per path instructions,pkg/**/query/**code should have inline comments explaining each major query section (CTEs, JOINs, window functions, WHERE clauses).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/db/query/cumulative_query.go` around lines 49 - 254, Add concise inline comments throughout TestReportQuery, testReportPreAgg, and TestReportQueryCollapsed that identify each major query section, including percentage/window-function calculations, openBugs and metadata joins, m/s prefix-sum self-joins, variant filtering, aggregation, and date/release WHERE clauses. Keep comments adjacent to the corresponding builder calls or SELECT blocks and describe their purpose without changing query behavior.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/db/query/cumulative_query.go`:
- Around line 27-47: Update buildTestsJoinCondition so Prefixes values pass
through escapeLikeMetachars before appending the trailing wildcard, preserving
literal prefix characters while retaining prefix matching behavior. Leave
exact-name and substring handling unchanged.
---
Outside diff comments:
In `@pkg/db/query/feature_gates.go`:
- Around line 78-87: Update the query flow around FilterableDBResult and q.Scan
in the relevant feature-gate function to wrap both returned errors with
contextual messages using fmt.Errorf and the %w verb, preserving the original
errors for unwrapping while identifying whether filtering or scanning failed.
---
Nitpick comments:
In `@pkg/db/query/cumulative_query.go`:
- Around line 51-54: Add unit tests for the pure helpers escapeLikeMetachars,
PeriodsForReportType, and resolveDateRanges in cumulative_query.go. Cover LIKE
metacharacter escaping, each report type’s period behavior, and date-range
boundary calculations including representative edge cases, while keeping tests
deterministic and isolated from external dependencies.
- Around line 49-254: Add concise inline comments throughout TestReportQuery,
testReportPreAgg, and TestReportQueryCollapsed that identify each major query
section, including percentage/window-function calculations, openBugs and
metadata joins, m/s prefix-sum self-joins, variant filtering, aggregation, and
date/release WHERE clauses. Keep comments adjacent to the corresponding builder
calls or SELECT blocks and describe their purpose without changing query
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: b3e19b0b-750f-4e5a-8cd7-91a1412eabe1
📒 Files selected for processing (13)
pkg/api/cache.gopkg/api/cache_test.gopkg/api/install.gopkg/api/tests.gopkg/db/cumulativesummary/cumulative_summary.gopkg/db/query/cumulative_query.gopkg/db/query/feature_gates.gopkg/db/query/misc_queries.gopkg/db/query/test_queries.gopkg/db/views.gopkg/flags/postgres_benchmarking_test.gopkg/html/installhtml/util.gopkg/sippyserver/server.go
💤 Files with no reviewable changes (4)
- pkg/db/query/misc_queries.go
- pkg/api/cache.go
- pkg/api/cache_test.go
- pkg/db/views.go
mstaeble
force-pushed
the
worktree-trt-2741-phase2
branch
from
d09b3c0 to
cc82cf3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/db/query/feature_gates.go (1)
13-90: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd regression coverage for
GetFeatureGatesFromDB. The new prefix-sum active-test check and thebyTag/byInstallunion should be covered so the query behavior stays stable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/db/query/feature_gates.go` around lines 13 - 90, Add regression tests for GetFeatureGatesFromDB covering both active-test detection via prefix_sum_runs and the combined byTag/byInstall query paths. Use fixtures that distinguish tests with actual recent runs from carried-forward rows, and verify feature-gate and install-test results are returned correctly.Source: Coding guidelines
🧹 Nitpick comments (2)
pkg/db/query/feature_gates.go (2)
33-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd inline comments for the byTag/byInstall query blocks.
As per path instructions,
pkg/**/query/**query-building code should have inline comments explaining each major query section (JOINs, WHERE filters, etc.).byTag'sCROSS JOIN LATERAL regexp_matches(...)and the reusedactiveTestExistsfilter in both blocks would benefit from a short comment on intent (e.g., why(m.match)[2]is the gate name, why the same active-test filter is applied to both tag- and install-derived subsets).✏️ Example inline comments
+ // Extract [FeatureGate:x] / [OCPFeatureGate:x] tag names, keeping only tests that + // actually ran (not merely carried forward) in the lookup window. byTag := dbc.DB.Table("tests t"). Select("t.name, ? AS release, (m.match)[2] AS gate_name", release). Joins(`CROSS JOIN LATERAL regexp_matches(t.name, '\[(FeatureGate|OCPFeatureGate):([^\]]+)\]', 'g') AS m(match)`). Where("t.name LIKE ?", "%FeatureGate:%"). Where(activeTestExists, lookupStart, lookupEnd, release) + // Install tests map to the "Install" feature gate via a NULL gate_name sentinel, + // matched later in fgQuery's join. byInstall := dbc.DB.Table("tests t"). Select("t.name, ? AS release, NULL::text AS gate_name", release). Where("t.name LIKE ?", "%install should succeed%"). Where(activeTestExists, lookupStart, lookupEnd, release)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/db/query/feature_gates.go` around lines 33 - 42, Add concise inline comments around the byTag and byInstall query blocks explaining the gate-name extraction from the lateral regexp match, the purpose of the CROSS JOIN LATERAL, and why both derived subsets reuse activeTestExists with the lookup bounds. Keep the query logic unchanged.Source: Path instructions
23-42: 🚀 Performance & Scalability | 🔵 TrivialVerify indexing supports the new correlated
EXISTSagainsttest_cumulative_summaries.
byTag/byInstalleach do a leading-wildcardLIKEscan oftests, then run theactiveTestExistscorrelated subquery (a self-join ontest_cumulative_summaries) per matching row. Postgres can turnEXISTSinto a semi-join, but that still depends on an index covering(test_id, date, release, prow_job_id, suite_id)ontest_cumulative_summariesto avoid a nested-loop scan per candidate test. Given the PR description mentions staged performance validation for this endpoint, please confirm the relevant index exists (or was already validated) for this access pattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/db/query/feature_gates.go` around lines 23 - 42, Verify that test_cumulative_summaries has an index supporting activeTestExists lookups across test_id, date, release, prow_job_id, and suite_id, including the self-join predicates. If the index is absent, add the appropriate migration or index definition; otherwise confirm the existing index covers this correlated EXISTS access pattern and preserve the byTag and byInstall queries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/db/query/feature_gates.go`:
- Around line 14-21: Update the date range setup around resolveDateRanges to
request a seven-day window instead of eight, and handle the clamped-range case
where lookupStart equals lookupEnd so valid feature gates are not filtered out.
Preserve the intended comparison of cumulative runs across the actual seven-day
period while ensuring releases with no in-range cumulative data still use an
effective lookup window.
---
Outside diff comments:
In `@pkg/db/query/feature_gates.go`:
- Around line 13-90: Add regression tests for GetFeatureGatesFromDB covering
both active-test detection via prefix_sum_runs and the combined byTag/byInstall
query paths. Use fixtures that distinguish tests with actual recent runs from
carried-forward rows, and verify feature-gate and install-test results are
returned correctly.
---
Nitpick comments:
In `@pkg/db/query/feature_gates.go`:
- Around line 33-42: Add concise inline comments around the byTag and byInstall
query blocks explaining the gate-name extraction from the lateral regexp match,
the purpose of the CROSS JOIN LATERAL, and why both derived subsets reuse
activeTestExists with the lookup bounds. Keep the query logic unchanged.
- Around line 23-42: Verify that test_cumulative_summaries has an index
supporting activeTestExists lookups across test_id, date, release, prow_job_id,
and suite_id, including the self-join predicates. If the index is absent, add
the appropriate migration or index definition; otherwise confirm the existing
index covers this correlated EXISTS access pattern and preserve the byTag and
byInstall queries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 691d4a7e-7f66-4a54-b44f-72dfdfd7e967
📒 Files selected for processing (13)
pkg/api/cache.gopkg/api/cache_test.gopkg/api/install.gopkg/api/tests.gopkg/db/cumulativesummary/cumulative_summary.gopkg/db/query/cumulative_query.gopkg/db/query/feature_gates.gopkg/db/query/misc_queries.gopkg/db/query/test_queries.gopkg/db/views.gopkg/flags/postgres_benchmarking_test.gopkg/html/installhtml/util.gopkg/sippyserver/server.go
💤 Files with no reviewable changes (4)
- pkg/db/query/misc_queries.go
- pkg/db/views.go
- pkg/api/cache.go
- pkg/api/cache_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- pkg/db/cumulativesummary/cumulative_summary.go
- pkg/html/installhtml/util.go
- pkg/db/query/cumulative_query.go
- pkg/sippyserver/server.go
- pkg/api/tests.go
- pkg/api/install.go
- pkg/flags/postgres_benchmarking_test.go
- pkg/db/query/test_queries.go
mstaeble
force-pushed
the
worktree-trt-2741-phase2
branch
from
cc82cf3 to
7d81fe6
Compare
openshift-ci
Bot
added
the
ready-for-human-review
mstaeble
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
Scheduling required tests: |
|
@mstaeble: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
mstaeble
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
mstaeble
force-pushed
the
worktree-trt-2741-phase2
branch
3 times, most recently
from
730ec83 to
e2513ef
Compare
mstaeble
force-pushed
the
worktree-trt-2741-phase2
branch
from
3880ecb to
65c172e
Compare
mstaeble
changed the title
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 46 minutes. |
… summary queries Replace all four prow_test_report materialized views with direct queries against the partitioned test_cumulative_summaries table from Phase 1. This eliminates the expensive matview refresh cycle and reduces query latency across all test report endpoints. Key changes: - New cumulative_query.go with core query builders (TestReportQuery, TestReportQueryCollapsed, UncollapsedTestReportWithStats) using 3-way prefix sum self-joins with partition-pruning date resolution - Materialized CTE pipeline (filtered -> post_filtered -> stats) for uncollapsed /api/tests with processedFilterConditions push-down - Aggregate-first approach for collapsed queries (per-date pre-aggregation then 3-way join of small results) - SET LOCAL work_mem tuning per query path (4MB uncollapsed, 16MB collapsed) to enable parallel HashAggregates instead of sort-based plans - Cross-variant statistics (AVG/STDDEV) computed in Go for the uncollapsed path, avoiding single-threaded WindowAgg on 1.8M rows - Overall test summary computed from already-scanned results in Go - Install/upgrade endpoints use precise name matching with UNION ALL for per-variant + "All" aggregate in a single query - Feature gates rewritten with EXISTS-based lookup against partitioned table - Remove matview cache, views.go, and PlatformInfraSuccess dead code Performance (staging, release 4.19): - /api/tests uncollapsed filtered: 55s -> 17s - /api/tests collapsed: 14s -> 3.5s - /api/install: 9.5s -> 0.56s - /api/feature_gates: 8.2s -> 0.26s Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
mstaeble
force-pushed
the
worktree-trt-2741-phase2
branch
from
65c172e to
8685ae5
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
pkg/dataloader/prowloader/prow.go (2)
1618-1622: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer structured logging for errors.
As per coding guidelines, prefer
log.WithError(err)over formatting error strings manually.♻️ Proposed refactor
suites, err := gcsJobRun.GetCombinedJUnitTestSuites(ctx) if err != nil { - log.Warningf("failed to get junit test suites: %s", err.Error()) + log.WithError(err).Warning("failed to get junit test suites") return nil, 0, "", err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/dataloader/prowloader/prow.go` around lines 1618 - 1622, Update the error logging in the GetCombinedJUnitTestSuites failure path to use structured logging via log.WithError(err) instead of interpolating err.Error() into the message, while preserving the existing warning message and return behavior.Source: Coding guidelines
749-769: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winEnsure rows are closed on early exit.
It is idiomatic Go to defer
rows.Close()immediately after checking the query error. Ifrows.Next()panics or a future code change returns early,defer rows.Close()prevents the connection from leaking.♻️ Proposed refactor
rows, err := conn.Query(ctx, ` SELECT t.id FROM tmp_candidate_ids t LEFT JOIN prow_job_runs r ON r.id = t.id WHERE r.id IS NULL `) if err != nil { return nil, fmt.Errorf("querying new job run IDs: %w", err) } + defer rows.Close() var newIDs []uint for rows.Next() { var id uint if err := rows.Scan(&id); err != nil { - rows.Close() return nil, fmt.Errorf("scanning new job run ID: %w", err) } newIDs = append(newIDs, id) } - rows.Close() if err := rows.Err(); err != nil { return nil, fmt.Errorf("iterating new job run IDs: %w", err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/dataloader/prowloader/prow.go` around lines 749 - 769, In the query-processing block, immediately defer rows.Close() after the successful conn.Query call. Remove the manual rows.Close() calls, including the scan-error path, while preserving the existing scan and rows.Err error handling.pkg/api/tests.go (1)
492-496: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap both transaction errors with operation context.
Returning the raw
ExecandScanerrors makes failures difficult to distinguish. Wrap them with%w, identifying whether settingwork_memor scanning reports failed.As per coding guidelines, “wrap errors with context using
fmt.Errorfand%w.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/api/tests.go` around lines 492 - 496, Update the transaction callback in the database test flow to wrap both failures with operation-specific context using fmt.Errorf and %w: identify errors from setting work_mem in the tx.Exec call and errors from scanning testReports in the tx.Table(...).Scan call. Preserve the underlying errors and transaction behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/api/tests.go`:
- Around line 447-452: Update the filter splitting logic around
spec.Filter.Split so OR expressions spanning name, variants, and processed
fields are not applied as separate ANDed query layers. Preserve the complete
Boolean expression together, or translate it in a single query layer for both
collapsed and uncollapsed paths, while retaining independent pushdown only for
expressions whose semantics remain unchanged.
- Around line 523-557: Add table-driven tests targeting computeOverallTest, with
cases for empty input, zero current and previous runs, and mixed test reports.
Assert aggregated totals, all percentage fields, and improvement
values—including their signs—while using safePercent’s zero-denominator behavior
for zero-run cases.
In `@pkg/db/query/cumulative_query.go`:
- Around line 146-155: Update resolveDateRanges to return the Scan error from
the max-date lookup, wrapping it with release context instead of only logging
and returning. Propagate the returned error through every caller and query
builder so failures stop report construction rather than producing an empty
result; preserve normal date-range behavior when the lookup succeeds.
- Around line 218-247: Restrict processedFilterConditions in
pkg/db/query/cumulative_query.go:218-247 to an allowlist of fields available in
the filtered CTE, returning stats-derived fields such as working_average,
standard deviations, and delta_from_* in remaining for later filtering. Update
the pushdown expectations and coverage in pkg/db/query/test_queries.go:277-350
to include only pre-stats fields, and ensure
pkg/db/query/test_queries.go:383-398 leaves stats-derived filters for
application around the final result.
- Around line 331-339: The query builders do not consistently preserve
Filter.LinkOperator, causing default-AND filters and explicit-OR variant filters
to produce incorrect results. In pkg/db/query/cumulative_query.go:331-339,
default the name joiner to AND and switch only for explicit OR; at 181-204,
retain the variant filter’s operator with its conditions; and at 363-367, join
variant conditions using that operator. Apply equivalent behavior in
pkg/db/query/test_queries.go:265-276 for names and 328-332 for variants, then
add regression coverage for default-AND names and explicit-OR variants.
In `@pkg/db/query/test_queries.go`:
- Around line 249-250: Expand “CTE” on its first occurrence in the
UncollapsedTestReportWithStats comment to “common table expression (CTE)”, while
leaving the subsequent reference unchanged.
---
Nitpick comments:
In `@pkg/api/tests.go`:
- Around line 492-496: Update the transaction callback in the database test flow
to wrap both failures with operation-specific context using fmt.Errorf and %w:
identify errors from setting work_mem in the tx.Exec call and errors from
scanning testReports in the tx.Table(...).Scan call. Preserve the underlying
errors and transaction behavior.
In `@pkg/dataloader/prowloader/prow.go`:
- Around line 1618-1622: Update the error logging in the
GetCombinedJUnitTestSuites failure path to use structured logging via
log.WithError(err) instead of interpolating err.Error() into the message, while
preserving the existing warning message and return behavior.
- Around line 749-769: In the query-processing block, immediately defer
rows.Close() after the successful conn.Query call. Remove the manual
rows.Close() calls, including the scan-error path, while preserving the existing
scan and rows.Err error handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a8bd26b8-90ae-46c2-9172-e1b8fb402697
📒 Files selected for processing (15)
pkg/api/cache.gopkg/api/cache_test.gopkg/api/install.gopkg/api/tests.gopkg/dataloader/prowloader/prow.gopkg/db/cumulativesummary/cumulative_summary.gopkg/db/query/cumulative_query.gopkg/db/query/cumulative_query_test.gopkg/db/query/feature_gates.gopkg/db/query/misc_queries.gopkg/db/query/test_queries.gopkg/db/views.gopkg/flags/postgres_benchmarking_test.gopkg/html/installhtml/util.gopkg/sippyserver/server.go
💤 Files with no reviewable changes (4)
- pkg/db/views.go
- pkg/api/cache.go
- pkg/db/query/misc_queries.go
- pkg/api/cache_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/sippyserver/server.go
- pkg/db/cumulativesummary/cumulative_summary.go
- pkg/html/installhtml/util.go
- pkg/db/query/feature_gates.go
- pkg/flags/postgres_benchmarking_test.go
| var nameFilter, variantFilter, processedFilter *filter.Filter | ||
| if spec.Filter != nil { | ||
| var rawFilter *filter.Filter | ||
| rawFilter, processedFilter = spec.Filter.Split([]string{"name", "variants"}) | ||
| nameFilter, variantFilter = rawFilter.Split([]string{"name"}) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve OR semantics when pushing filters into separate query layers.
A filter such as name = X OR current_runs > 10 is split here, then applied as an inner name condition and an outer processed condition. The same problem affects name/variant OR combinations, silently narrowing results in both collapsed and uncollapsed paths. Keep cross-group OR expressions together or translate the complete Boolean expression in one query layer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/api/tests.go` around lines 447 - 452, Update the filter splitting logic
around spec.Filter.Split so OR expressions spanning name, variants, and
processed fields are not applied as separate ANDed query layers. Preserve the
complete Boolean expression together, or translate it in a single query layer
for both collapsed and uncollapsed paths, while retaining independent pushdown
only for expressions whose semantics remain unchanged.
| func safePercent(numerator, denominator int) float64 { | ||
| if denominator == 0 { | ||
| return 0 | ||
| } | ||
| return float64(numerator) * 100.0 / float64(denominator) | ||
| } | ||
|
|
||
| func computeOverallTest(testReports []apitype.Test) *apitype.Test { | ||
| overall := &apitype.Test{ | ||
| ID: math.MaxInt32, | ||
| Name: "Overall", | ||
| } | ||
| for _, t := range testReports { | ||
| overall.CurrentRuns += t.CurrentRuns | ||
| overall.CurrentSuccesses += t.CurrentSuccesses | ||
| overall.CurrentFailures += t.CurrentFailures | ||
| overall.CurrentFlakes += t.CurrentFlakes | ||
| overall.PreviousRuns += t.PreviousRuns | ||
| overall.PreviousSuccesses += t.PreviousSuccesses | ||
| overall.PreviousFailures += t.PreviousFailures | ||
| overall.PreviousFlakes += t.PreviousFlakes | ||
| } | ||
| overall.CurrentPassPercentage = safePercent(overall.CurrentSuccesses, overall.CurrentRuns) | ||
| overall.CurrentFailurePercentage = safePercent(overall.CurrentFailures, overall.CurrentRuns) | ||
| overall.CurrentFlakePercentage = safePercent(overall.CurrentFlakes, overall.CurrentRuns) | ||
| overall.CurrentWorkingPercentage = safePercent(overall.CurrentSuccesses+overall.CurrentFlakes, overall.CurrentRuns) | ||
| overall.PreviousPassPercentage = safePercent(overall.PreviousSuccesses, overall.PreviousRuns) | ||
| overall.PreviousFailurePercentage = safePercent(overall.PreviousFailures, overall.PreviousRuns) | ||
| overall.PreviousFlakePercentage = safePercent(overall.PreviousFlakes, overall.PreviousRuns) | ||
| overall.PreviousWorkingPercentage = safePercent(overall.PreviousSuccesses+overall.PreviousFlakes, overall.PreviousRuns) | ||
| overall.NetFailureImprovement = overall.PreviousFailurePercentage - overall.CurrentFailurePercentage | ||
| overall.NetFlakeImprovement = overall.PreviousFlakePercentage - overall.CurrentFlakePercentage | ||
| overall.NetWorkingImprovement = overall.CurrentWorkingPercentage - overall.PreviousWorkingPercentage | ||
| overall.NetImprovement = overall.CurrentPassPercentage - overall.PreviousPassPercentage | ||
| return overall |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 '\b(computeOverallTest|safePercent)\s*\(' pkg/api --glob '*_test.go'Repository: openshift/sippy
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== pkg/api files ==\n'
git ls-files 'pkg/api/*' | sed -n '1,200p'
printf '\n== outline tests.go ==\n'
ast-grep outline pkg/api/tests.go --view expanded || true
printf '\n== relevant slice of tests.go ==\n'
sed -n '500,590p' pkg/api/tests.go
printf '\n== pkg/api test files mentioning Test / overall ==\n'
rg -n -C3 'Overall|computeOverallTest|safePercent|CurrentPassPercentage|NetFailureImprovement' pkg/api --glob '*_test.go' || trueRepository: openshift/sippy
Length of output: 25041
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== direct references to computeOverallTest ==\n'
rg -n 'computeOverallTest|safePercent' .
printf '\n== likely test coverage around tests.go ==\n'
rg -n -C3 'IncludeOverall|Overall"|\bOverall\b|NetImprovement|CurrentWorkingPercentage|PreviousWorkingPercentage' pkg/api --glob '*_test.go'Repository: openshift/sippy
Length of output: 1467
Add table-driven tests for computeOverallTest in pkg/api/tests.go. Cover empty input, zero current/previous runs, and mixed reports so the totals, percentages, and improvement signs stay pinned down.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/api/tests.go` around lines 523 - 557, Add table-driven tests targeting
computeOverallTest, with cases for empty input, zero current and previous runs,
and mixed test reports. Assert aggregated totals, all percentage fields, and
improvement values—including their signs—while using safePercent’s
zero-denominator behavior for zero-run cases.
Source: Coding guidelines
| func resolveDateRanges(dbc *db.DB, release string, ranges ...*DateRange) { | ||
| var maxDate *civil.Date | ||
| row := dbc.DB.Table("test_cumulative_summaries"). | ||
| Select("MAX(date)"). | ||
| Where("release = ?", release). | ||
| Row() | ||
| if err := row.Scan(&maxDate); err != nil { | ||
| log.WithError(err).WithField("release", release).Error("failed to resolve max date for cumulative summaries") | ||
| return | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Propagate failures from the max-date lookup.
After Scan fails, query construction continues with unresolved future dates, so a transient lookup failure can become a successful but empty report. Return and wrap this error through the query builders instead of only logging it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/db/query/cumulative_query.go` around lines 146 - 155, Update
resolveDateRanges to return the Scan error from the max-date lookup, wrapping it
with release context instead of only logging and returning. Propagate the
returned error through every caller and query builder so failures stop report
construction rather than producing an empty result; preserve normal date-range
behavior when the lookup succeeds.
| // processedFilterConditions converts arithmetic filter items to raw SQL WHERE | ||
| // conditions with parameterized args. Items with unsupported operators (ILIKE, | ||
| // array membership, etc.) are returned in the remaining filter for the caller to | ||
| // apply via GORM. Splitting is only safe for AND-linked filters; OR-linked | ||
| // filters are returned entirely as remaining. | ||
| func processedFilterConditions(f *filter.Filter) (conditions []string, args []any, remaining *filter.Filter) { | ||
| if f == nil || len(f.Items) == 0 { | ||
| return nil, nil, nil | ||
| } | ||
| if f.LinkOperator == filter.LinkOperatorOr { | ||
| return nil, nil, f | ||
| } | ||
| var unsupported []filter.FilterItem | ||
| for _, item := range f.Items { | ||
| if op, ok := arithmeticOps[item.Operator]; ok { | ||
| field := pq.QuoteIdentifier(item.Field) | ||
| cond := fmt.Sprintf("%s %s ?", field, op) | ||
| if item.Not { | ||
| cond = negateConditions([]string{cond})[0] | ||
| } | ||
| conditions = append(conditions, cond) | ||
| args = append(args, item.Value) | ||
| } else { | ||
| unsupported = append(unsupported, item) | ||
| } | ||
| } | ||
| if len(unsupported) > 0 { | ||
| remaining = &filter.Filter{Items: unsupported, LinkOperator: f.LinkOperator} | ||
| } | ||
| return conditions, args, remaining |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not push stats-only fields into post_filtered.
processedFilterConditions treats every arithmetic field as pushdown-safe. Fields such as working_average, standard deviations, and delta_from_* are created only by the final stats join, so default/AND filters on them produce missing-column SQL errors.
pkg/db/query/cumulative_query.go#L218-L247: allowlist fields available infiltered; return later-stage fields inremaining.pkg/db/query/test_queries.go#L277-L350: push only fields available before the stats CTE.pkg/db/query/test_queries.go#L383-L398: leave stats-derived filters for the caller to apply around this final result.
📍 Affects 2 files
pkg/db/query/cumulative_query.go#L218-L247(this comment)pkg/db/query/test_queries.go#L277-L350pkg/db/query/test_queries.go#L383-L398
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/db/query/cumulative_query.go` around lines 218 - 247, Restrict
processedFilterConditions in pkg/db/query/cumulative_query.go:218-247 to an
allowlist of fields available in the filtered CTE, returning stats-derived
fields such as working_average, standard deviations, and delta_from_* in
remaining for later filtering. Update the pushdown expectations and coverage in
pkg/db/query/test_queries.go:277-350 to include only pre-stats fields, and
ensure pkg/db/query/test_queries.go:383-398 leaves stats-derived filters for
application around the final result.
| var nameJoinClause string | ||
| var nameJoinArgs []any | ||
| if len(nameConds) > 0 { | ||
| joiner := " OR " | ||
| if nameFilter.LinkOperator == filter.LinkOperatorAnd { | ||
| joiner = " AND " | ||
| } | ||
| nameJoinClause = "JOIN tests ON tests.id = tcs.test_id AND (" + strings.Join(nameConds, joiner) + ")" | ||
| nameJoinArgs = nameArgs |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve Filter.LinkOperator when pushing filters down.
The filter contract defaults to AND unless LinkOperatorOr is explicit, but name conditions default to OR here while variant conditions are always combined with AND. Multi-item filters therefore return incorrect rows.
pkg/db/query/cumulative_query.go#L331-L339: default the name joiner to AND and switch only for explicit OR.pkg/db/query/cumulative_query.go#L181-L204: preserve the variant filter’s link operator alongside its conditions.pkg/db/query/cumulative_query.go#L363-L367: group variant conditions using that operator instead of appending each with AND.pkg/db/query/test_queries.go#L265-L276: apply the same default-AND name behavior.pkg/db/query/test_queries.go#L328-L332: combine variant conditions using the requested link operator.
Add regression cases for default-AND names and explicit-OR variants.
📍 Affects 2 files
pkg/db/query/cumulative_query.go#L331-L339(this comment)pkg/db/query/cumulative_query.go#L181-L204pkg/db/query/cumulative_query.go#L363-L367pkg/db/query/test_queries.go#L265-L276pkg/db/query/test_queries.go#L328-L332
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/db/query/cumulative_query.go` around lines 331 - 339, The query builders
do not consistently preserve Filter.LinkOperator, causing default-AND filters
and explicit-OR variant filters to produce incorrect results. In
pkg/db/query/cumulative_query.go:331-339, default the name joiner to AND and
switch only for explicit OR; at 181-204, retain the variant filter’s operator
with its conditions; and at 363-367, join variant conditions using that
operator. Apply equivalent behavior in pkg/db/query/test_queries.go:265-276 for
names and 328-332 for variants, then add regression coverage for default-AND
names and explicit-OR variants.
| // UncollapsedTestReportWithStats builds a per-variant test report with cross-variant | ||
| // statistics using a materialized CTE pipeline: filtered -> post_filtered -> stats. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Expand CTE on first use.
Use “common table expression (CTE)” here.
As per path instructions, “Abbreviations like CTE, matview, etc. should be expanded on first use.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/db/query/test_queries.go` around lines 249 - 250, Expand “CTE” on its
first occurrence in the UncollapsedTestReportWithStats comment to “common table
expression (CTE)”, while leaving the subsequent reference unchanged.
Source: Path instructions
2 participants
Summary
Replace
prow_test_report_7d_matview,prow_test_report_2d_matview, and their collapsed variants with direct queries against thetest_cumulative_summariespartitioned prefix sum tables introduced in Phase 1 (PR #3762). This eliminates the expensive materialized view refresh step from the server startup/refresh path.Key changes
cumulative_query.go: Core query builders using a 3-way self-join ontest_cumulative_summaries(end, boundary, start dates) to compute period counts from prefix sums. IncludestestReportCoreJoin(per-variant),testReportPreAgg(with metadata), andTestReportQueryCollapsed(aggregated per test).DateRangetype: Half-open[Start, End)intervals withPeriodsForReportTypeto compute sample/base periods andresolvePrefixSumDatesto clamp to available data.nameFilterConditionsconverts name filters (equals, starts-with, contains, with NOT support) directly into SQL conditions pushed into inner queries, avoiding full-table scans.variantFilterConditionsgenerates EXISTS/NOT EXISTS subqueries againstvariant_combinationsfor raw SQL contexts.processedFilterConditionspushes arithmetic comparisons (>=, <=, =, etc.) into the materialized CTE in the uncollapsed path.strings.Builder+ positional args for query construction, sharing theopenBugsSQLconstant and using the samenameFilterConditions/variantFilterConditionshelpers. This eliminates duplication between GORM and raw SQL approaches.GetDataFromCacheOrMatview: Replaced by existingGetDataFromCacheOrGenerate.pkg/db/views.godeleted;UpdateSchemano longer creates/refreshes materialized views.~*regex matching for case-insensitive substring filters.Staging performance (2026-07-16, commit 65c172e)
Deployed to staging (
sippy-staging-88). All times are cold cache (Redis flushed before each test)./api/testsendpointOther endpoints
/api/install?release=4.19/api/feature_gates?release=4.19Parity verification (staging vs production)
Ran 5 parallel parity test suites comparing staging (new cumulative query code) against production (old matview code) across releases 4.18, 4.19, and 5.0.
Methodology
Each test fetched the same API endpoint from both environments and compared row counts, test names, and field values. Staging and production have independently-refreshed databases, so data freshness differences are expected.
Results
Key finding: Across all test suites, whenever staging and production have the same underlying data (identical run counts), all computed percentages, net improvements, and derived fields match exactly. Every discrepancy traces to different data coverage between the two independently-refreshed environments.
Post-refactor parity (raw SQL conversion)
After converting the collapsed path from GORM to raw SQL, ran full parity check against the staging deployment (old GORM code, same database):
Zero mismatches across all rows and all fields (runs, successes, failures, flakes, percentages, open_bugs, jira_component).
Test plan
go vet ./pkg/...passesmake lintpasses (0 issues)make testpassesmake e2epasses (5/5 suites)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Refactor