[WIP] TRT-2741: Add per-request BQ/PG toggle for Component Readiness#3797
Draft
mstaeble wants to merge 2 commits into
Draft
[WIP] TRT-2741: Add per-request BQ/PG toggle for Component Readiness#3797mstaeble wants to merge 2 commits into
mstaeble wants to merge 2 commits into
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@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
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
[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 |
Contributor
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (36)
WalkthroughComponent Readiness now supports deterministic identifier encoding, request-scoped BigQuery/PostgreSQL routing, PostgreSQL status aggregation, indexed regression matching, data-source-aware URLs, and 30-day frontend date presets. ChangesComponent Readiness data flow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ComponentReadiness as ComponentReadiness
participant SippyServer as sippyserver
participant MixedProvider as MixedProvider
participant PostgresProvider as PostgresProvider
ComponentReadiness->>SippyServer: send dataSource query parameter
SippyServer->>MixedProvider: pass request options
MixedProvider->>PostgresProvider: route PostgreSQL request
PostgresProvider-->>SippyServer: return aggregated report data
SippyServer-->>ComponentReadiness: return Component Readiness response
Suggested reviewers: 🚥 Pre-merge checks | ✅ 17 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (17 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
pkg/api/componentreadiness/dataprovider/postgres/provider.go (1)
224-245: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSilently swallowed DB error in
baseMatchesGAWindowhides real failures.Any error from the
Pluckquery (not just "no GA date yet") causes a silentreturn false, permanently falling back to the slower prefix-sum path with no log signal. Since this optimization is a core goal of this PR, a genuine query/connectivity failure here would be indistinguishable from "release isn't GA yet."♻️ Suggested fix
var gaDate *time.Time err := p.dbc.DB.WithContext(ctx). Model(&models.ReleaseDefinition{}). Where("release = ? AND ga_date < CURRENT_DATE", reqOptions.BaseRelease.Name). Pluck("ga_date", &gaDate).Error - if err != nil || gaDate == nil { + if err != nil { + log.WithError(err).Warn("failed to query GA date for base release, falling back to prefix-sum query") + return false + } + if gaDate == nil { return false }🤖 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/componentreadiness/dataprovider/postgres/provider.go` around lines 224 - 245, Update PostgresProvider.baseMatchesGAWindow to distinguish a genuine database query failure from a missing GA date: log the Pluck error with relevant release context before returning false, while retaining the existing false result for nil gaDate and non-matching windows.pkg/api/componentreadiness/middleware/regressiontracker/regressiontracker_test.go (1)
375-390: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePull common tracker initialization out of the conditional branches.
Since both branches build the index and set the
hasLoadedRegressionsflag in the exact same way based onmw.openRegressions, you can pull those assignments out to simplify the setup.♻️ Proposed refactor
if tt.hasOpenRegression { openRegression := &models.TestRegression{ ID: 1, Release: sampleRelease, TestID: testKey.TestID, TestName: testKey.TestName, Variants: variantsStrSlice, Opened: time.Now().UTC().Add(-5 * 24 * time.Hour), Closed: sql.NullTime{Valid: false}, } mw.openRegressions = []*models.TestRegression{openRegression} - mw.regressionsByTestID = BuildRegressionIndex(mw.openRegressions) - mw.hasLoadedRegressions = true } else { mw.openRegressions = []*models.TestRegression{} - mw.regressionsByTestID = BuildRegressionIndex(mw.openRegressions) - mw.hasLoadedRegressions = true } + mw.regressionsByTestID = BuildRegressionIndex(mw.openRegressions) + mw.hasLoadedRegressions = true🤖 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/componentreadiness/middleware/regressiontracker/regressiontracker_test.go` around lines 375 - 390, Refactor the conditional setup around mw.openRegressions so each branch only assigns the appropriate regression slice; after the conditional, call BuildRegressionIndex once using mw.openRegressions and set hasLoadedRegressions once.pkg/api/componentreadiness/regressiontracker.go (1)
295-297: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueHoist loop-invariant
crossCompareevaluation.The
crossCompareboolean is derived solely fromview.VariantOptions, which does not change during the loop. Consider hoisting this check outside the loop to avoid redundant evaluations.♻️ Proposed refactor
+ crossCompare := len(view.VariantOptions.VariantCrossCompare) > 0 for _, regTest := range allRegressedTests { - crossCompare := len(view.VariantOptions.VariantCrossCompare) > 0 if openReg := regressiontracker.FindOpenRegression(view.SampleRelease.Name, regTest.TestID, crossCompare, regTest.Variants, regressionIndex); openReg != nil {🤖 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/componentreadiness/regressiontracker.go` around lines 295 - 297, Move the crossCompare assignment out of the allRegressedTests loop so len(view.VariantOptions.VariantCrossCompare) is evaluated once before iteration. Keep the existing value and pass the hoisted crossCompare variable unchanged to regressiontracker.FindOpenRegression.sippy-ng/src/component_readiness/CompReadyVars.js (1)
461-462: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
dataSourcequery param built via raw string concatenation in three places instead of the required URL-encoding helper. Path instructions forsippy-ng/**/*.jsstate that new Component Readiness query parameters likedataSourceshould be encoded viasafeEncodeURIComponent/single/multiple/multiple_or, but all three sites manually splice the value into the URL string.
sippy-ng/src/component_readiness/CompReadyVars.js#L461-L462: replace`?dataSource=${dataSource}`withsafeEncodeURIComponent(dataSource)(or route through the shared helper used elsewhere) before appending to the job-variants URL.sippy-ng/src/component_readiness/ComponentReadiness.js#L313-L315: replace'&dataSource=' + varsContext.dataSourcewith an encoded equivalent, e.g.'&dataSource=' + safeEncodeURIComponent(varsContext.dataSource).sippy-ng/src/component_readiness/ComponentReadinessIndicator.js#L61-L72: replace`&dataSource=${dataSource}`with an encoded equivalent, e.g.`&dataSource=${safeEncodeURIComponent(dataSource)}`.Today
dataSourceonly ever takes the value''/'postgres'(from the cookie), so there's no live injection risk, but this deviates from the documented pattern for consistency.As per path instructions, "When constructing query-string URLs/filters for Component Readiness (including any new parameters like
dataSource), use the provided URL-encoding helpers (safeEncodeURIComponent,single,multiple,multiple_or) so values are encoded consistently."🤖 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 `@sippy-ng/src/component_readiness/CompReadyVars.js` around lines 461 - 462, Encode the dataSource query parameter consistently at all three sites: in sippy-ng/src/component_readiness/CompReadyVars.js lines 461-462, update the job-variants URL construction to use safeEncodeURIComponent; in sippy-ng/src/component_readiness/ComponentReadiness.js lines 313-315, encode varsContext.dataSource before appending it; and in sippy-ng/src/component_readiness/ComponentReadinessIndicator.js lines 61-72, encode dataSource before interpolation. Use the existing URL-encoding helper and preserve the surrounding URL construction.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/sippyserver/server.go`:
- Around line 1108-1112: The GetJobVariants error paths discard the returned
errs context. Update both error sites in pkg/sippyserver/server.go (lines
1108-1112 and 1161-1166): include errs in the fmt.Errorf message at the first
site and in the fmt.Errorf call inside failureResponseWithError at the second
site, preserving the existing error handling behavior.
In `@sippy-ng/src/component_readiness/ReleaseSelector.js`:
- Around line 335-339: The “Last 30 days” toggle still uses the numeric Filter4
icon, which conflicts with its label. Update the icon rendered in the
ToggleButton using set4Weeks to a non-numeric or generic calendar icon, while
leaving the internal fourWeeksStart/set4Weeks names unchanged.
---
Nitpick comments:
In `@pkg/api/componentreadiness/dataprovider/postgres/provider.go`:
- Around line 224-245: Update PostgresProvider.baseMatchesGAWindow to
distinguish a genuine database query failure from a missing GA date: log the
Pluck error with relevant release context before returning false, while
retaining the existing false result for nil gaDate and non-matching windows.
In
`@pkg/api/componentreadiness/middleware/regressiontracker/regressiontracker_test.go`:
- Around line 375-390: Refactor the conditional setup around mw.openRegressions
so each branch only assigns the appropriate regression slice; after the
conditional, call BuildRegressionIndex once using mw.openRegressions and set
hasLoadedRegressions once.
In `@pkg/api/componentreadiness/regressiontracker.go`:
- Around line 295-297: Move the crossCompare assignment out of the
allRegressedTests loop so len(view.VariantOptions.VariantCrossCompare) is
evaluated once before iteration. Keep the existing value and pass the hoisted
crossCompare variable unchanged to regressiontracker.FindOpenRegression.
In `@sippy-ng/src/component_readiness/CompReadyVars.js`:
- Around line 461-462: Encode the dataSource query parameter consistently at all
three sites: in sippy-ng/src/component_readiness/CompReadyVars.js lines 461-462,
update the job-variants URL construction to use safeEncodeURIComponent; in
sippy-ng/src/component_readiness/ComponentReadiness.js lines 313-315, encode
varsContext.dataSource before appending it; and in
sippy-ng/src/component_readiness/ComponentReadinessIndicator.js lines 61-72,
encode dataSource before interpolation. Use the existing URL-encoding helper and
preserve the surrounding URL construction.
🪄 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: fbe2a5de-4b74-4eb1-b1d8-3e9fe1b03826
📒 Files selected for processing (36)
cmd/sippy/annotatejobruns.gocmd/sippy/automatejira.gopkg/api/componentreadiness/component_report.gopkg/api/componentreadiness/component_report_test.gopkg/api/componentreadiness/dataprovider/bigquery/provider.gopkg/api/componentreadiness/dataprovider/bigquery/querygenerators.gopkg/api/componentreadiness/dataprovider/interface.gopkg/api/componentreadiness/dataprovider/mixed/provider.gopkg/api/componentreadiness/dataprovider/postgres/cr_queries.gopkg/api/componentreadiness/dataprovider/postgres/provider.gopkg/api/componentreadiness/dataprovider/postgres/variants.gopkg/api/componentreadiness/middleware/linkinjector/linkinjector.gopkg/api/componentreadiness/middleware/regressiontracker/regressiontracker.gopkg/api/componentreadiness/middleware/regressiontracker/regressiontracker_test.gopkg/api/componentreadiness/middleware/releasefallback/releasefallback.gopkg/api/componentreadiness/middleware/releasefallback/releasefallback_test.gopkg/api/componentreadiness/queryparamparser_test.gopkg/api/componentreadiness/regressiontracker.gopkg/api/componentreadiness/test_details.gopkg/api/componentreadiness/triage.gopkg/api/componentreadiness/utils/queryparamparser.gopkg/api/componentreadiness/utils/utils.gopkg/api/componentreadiness/utils/utils_test.gopkg/apis/api/componentreport/crstatus/types.gopkg/apis/api/componentreport/crtest/types.gopkg/apis/api/componentreport/crtest/types_test.gopkg/apis/api/componentreport/reqopts/types.gopkg/dataloader/gateststatus/loader.gopkg/db/models/prow.gopkg/sippyserver/server.gopkg/util/param/param.gosippy-ng/src/component_readiness/CompReadyUtils.jssippy-ng/src/component_readiness/CompReadyVars.jssippy-ng/src/component_readiness/ComponentReadiness.jssippy-ng/src/component_readiness/ComponentReadinessIndicator.jssippy-ng/src/component_readiness/ReleaseSelector.js
mstaeble
changed the title
mstaeble
force-pushed
the
trt-2741-phase3
branch
2 times, most recently
from
July 21, 2026 16:43
520e911 to
a5e464f
Compare
openshift-ci
Bot
added
the
ready-for-human-review
Add a dataSource URL parameter that lets users switch Component Readiness between BigQuery and PostgreSQL on a per-request basis. The PostgreSQL path uses prefix-sum queries on test_cumulative_summaries for the sample period and query-time aggregation of prow_ga_raw_test_data for the GA base period. Key changes: - Wire dataSource parameter through API to select BigQuery or Postgres provider - Implement prefix-sum sample queries and GA base queries in Postgres - Optimize Go-side processing: replace JSON key serialization with fast null-byte encoding, add regression index for O(1) lookups - Remove unused prow_ga_test_statuses_matview (sample query dominates wall-clock time, so matview provides no visible speedup) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mstaeble
force-pushed
the
trt-2741-phase3
branch
from
July 22, 2026 13:17
a5e464f to
51fc218
Compare
…ested runs The modified_time partition-pruning filter on the junit table was using the exact @From/@to range, excluding test runs where BQ ingestion lagged behind prowjob_start by up to a day. Add a 1-day buffer on each side so the prowjob_start JOIN remains the authoritative date gate while partition pruning still limits scanned data. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
dataSourceURL parameter (bigqueryorpostgres) to Component Readiness endpoints, allowing per-request selection of the data backendDBGroupBydimensions in SQL rather than in Goprow_ga_raw_test_datafor performance, falling back to prefix-sum for non-GA windows (cross-compare views, custom date ranges)dataSourcethrough HATEOAS test details links so drill-down stays on the selected backendga-30dview config) so the PG GA optimization path is hitTest plan
make lintpassesmake testpasses (Go + Jest)make e2epasses4.18) produce matching results between BQ and PG against staging DB4.18-ha-vs-single) work withdataSource=postgresdataSourcewhen set🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
UI Updates