From d88aa810630ed791188efe104c774ff83d386e0d Mon Sep 17 00:00:00 2001 From: Luke Meyer Date: Thu, 11 Jul 2024 17:36:29 -0400 Subject: [PATCH 1/7] Risk Analysis: ensure Test ID is populated At the time that openshift-e2e-tests requests RA, test results haven't yet been associated with test IDs (we just have junits). So it is posting a `ProwJobRun` with partial `Test` records to the API, sans IDs. This PR changes the past-results queries by test name to also return the test ID, and uses that to populate RA results. --- pkg/api/job_runs.go | 10 +++++++++- pkg/api/tests.go | 5 ++--- pkg/db/query/test_queries.go | 2 +- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/pkg/api/job_runs.go b/pkg/api/job_runs.go index 77cd2eb72..686ad4661 100644 --- a/pkg/api/job_runs.go +++ b/pkg/api/job_runs.go @@ -528,9 +528,17 @@ func runTestRunAnalysis(failedTest models.ProwJobRunTest, jobRun *models.ProwJob return apitype.ProwJobRunTestRiskAnalysis{}, errJobNames } + // one of our data sources should have the test ID + testID := failedTest.Test.ID + if testID == 0 && testResultsJobNames != nil { + testID = uint(testResultsJobNames.ID) + } + if testID == 0 && testResultsVariants != nil { + testID = uint(testResultsVariants.ID) + } analysis := apitype.ProwJobRunTestRiskAnalysis{ Name: failedTest.Test.Name, - TestID: failedTest.Test.ID, + TestID: testID, OpenBugs: failedTest.Test.Bugs, } // Watch out for tests that ran in previous period, but not current, no sense comparing to 0 runs: diff --git a/pkg/api/tests.go b/pkg/api/tests.go index 34e9cb3f5..8f0d2c705 100644 --- a/pkg/api/tests.go +++ b/pkg/api/tests.go @@ -195,7 +195,7 @@ func BuildTestsResults(dbc *db.DB, release, period string, collapse, includeOver // Collapse groups the test results together -- otherwise we return the test results per-variant combo (NURP+) variantSelect := "" if collapse { - rawQuery = rawQuery.Select(`name,watchlist,jira_component,jira_component_id,` + query.QueryTestSummer).Group("name,watchlist,jira_component,jira_component_id") + rawQuery = rawQuery.Select(`id,name,watchlist,jira_component,jira_component_id,` + query.QueryTestSummer).Group("id,name,watchlist,jira_component,jira_component_id") } else { rawQuery = query.TestsByNURPAndStandardDeviation(dbc, release, table) variantSelect = "suite_name, variants," + @@ -210,9 +210,8 @@ func BuildTestsResults(dbc *db.DB, release, period string, collapse, includeOver } testReports := make([]apitype.Test, 0) - // FIXME: Add test id to matview, for now generate with ROW_NUMBER OVER processedResults := dbc.DB.Table("(?) as results", rawQuery). - Select(`ROW_NUMBER() OVER() as id, watchlist, name, jira_component, jira_component_id,` + variantSelect + query.QueryTestSummarizer). + Select(`id, watchlist, name, jira_component, jira_component_id,` + variantSelect + query.QueryTestSummarizer). Where("current_runs > 0 or previous_runs > 0") finalResults := dbc.DB.Table("(?) as final_results", processedResults) diff --git a/pkg/db/query/test_queries.go b/pkg/db/query/test_queries.go index 9ccdc14ae..e6c9f25a0 100644 --- a/pkg/db/query/test_queries.go +++ b/pkg/db/query/test_queries.go @@ -55,7 +55,7 @@ const ( QueryTestSummarizer = QueryTestFields + "," + QueryTestPercentages - QueryTestAnalysis = "select current_successes, current_runs, current_successes * 100.0 / NULLIF(current_runs, 0) AS current_pass_percentage from ( select sum(runs) as current_runs, sum(passes) as current_successes from prow_test_analysis_by_job_14d_matview where test_name = '%s' AND job_name in (%s))t" + QueryTestAnalysis = "select test_id as id, current_successes, current_runs, current_successes * 100.0 / NULLIF(current_runs, 0) AS current_pass_percentage from ( select test_id, sum(runs) as current_runs, sum(passes) as current_successes from prow_test_analysis_by_job_14d_matview where test_name = '%s' AND job_name in (%s) GROUP BY test_id)t" ) // TestReportsByVariant returns a test report for every test in the db matching the given substrings, separated by variant. From 70bbde5e1e3c49b6b71bcdc626adf515614f42a8 Mon Sep 17 00:00:00 2001 From: Luke Meyer Date: Thu, 11 Jul 2024 16:21:35 -0400 Subject: [PATCH 2/7] serve.go: add debuggable entry point --- cmd/sippy/serve.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cmd/sippy/serve.go b/cmd/sippy/serve.go index 63e61198f..f4ecca6d0 100644 --- a/cmd/sippy/serve.go +++ b/cmd/sippy/serve.go @@ -79,7 +79,11 @@ func (f *ServerFlags) Validate() error { func NewServeCommand() *cobra.Command { f := NewServerFlags() - + cmd := newServeFlagsCommand(f) + f.BindFlags(cmd.Flags()) + return cmd +} +func newServeFlagsCommand(f *ServerFlags) *cobra.Command { cmd := &cobra.Command{ Use: "serve", Short: "Run the sippy server", From c969e6c2f24bb3af73d2a226160768603e0a2263 Mon Sep 17 00:00:00 2001 From: Luke Meyer Date: Sat, 27 Jul 2024 16:02:10 -0400 Subject: [PATCH 3/7] debug: raise db logging --- pkg/db/db.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/db/db.go b/pkg/db/db.go index 05c28c85b..b0c7f0c00 100644 --- a/pkg/db/db.go +++ b/pkg/db/db.go @@ -37,7 +37,7 @@ type log2LogrusWriter struct { } func (w log2LogrusWriter) Printf(msg string, args ...interface{}) { - w.entry.Debugf(msg, args...) + w.entry.Infof(msg, args...) } func New(dsn string, logLevel gormlogger.LogLevel) (*DB, error) { From c9b465977b3ed5db310c2123d7b046589c1b3fc0 Mon Sep 17 00:00:00 2001 From: Luke Meyer Date: Sun, 28 Jul 2024 15:12:28 -0400 Subject: [PATCH 4/7] include string test_id in analysis matviews --- pkg/db/matviews.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pkg/db/matviews.go b/pkg/db/matviews.go index 2f36cb452..fd7471bcf 100644 --- a/pkg/db/matviews.go +++ b/pkg/db/matviews.go @@ -242,6 +242,7 @@ GROUP BY const testAnalysisByVariantMatView = ` SELECT tests.id AS test_id, + town.unique_id as test_str_id, tests.name AS test_name, tests.watchlist, date(prow_job_runs."timestamp") AS date, @@ -254,17 +255,19 @@ SELECT FROM prow_job_run_tests JOIN tests ON tests.id = prow_job_run_tests.test_id + JOIN test_ownerships town ON town.test_id = tests.id JOIN prow_job_runs ON prow_job_runs.id = prow_job_run_tests.prow_job_run_id JOIN prow_jobs ON prow_jobs.id = prow_job_runs.prow_job_id WHERE prow_job_run_tests.created_at > (|||TIMENOW||| - '14 days'::interval) AND prow_job_runs."timestamp" > (|||TIMENOW||| - '14 days'::interval) GROUP BY - tests.name, tests.id, date(prow_job_runs."timestamp"), unnest(prow_jobs.variants), prow_jobs.release + tests.name, town.unique_id, tests.id, date(prow_job_runs."timestamp"), unnest(prow_jobs.variants), prow_jobs.release ` const testAnalysisByJobMatView = ` SELECT tests.id AS test_id, + town.unique_id as test_str_id, tests.name AS test_name, tests.watchlist, date(prow_job_runs."timestamp") AS date, @@ -277,12 +280,13 @@ SELECT FROM prow_job_run_tests JOIN tests ON tests.id = prow_job_run_tests.test_id + JOIN test_ownerships town ON town.test_id = tests.id JOIN prow_job_runs ON prow_job_runs.id = prow_job_run_tests.prow_job_run_id JOIN prow_jobs ON prow_jobs.id = prow_job_runs.prow_job_id WHERE prow_job_run_tests.created_at > (|||TIMENOW||| - '14 days'::interval) AND prow_job_runs."timestamp" > (|||TIMENOW||| - '14 days'::interval) GROUP BY - tests.name, tests.id, date(prow_job_runs."timestamp"), prow_jobs.release, prow_jobs.name + tests.name, town.unique_id, tests.id, date(prow_job_runs."timestamp"), prow_jobs.release, prow_jobs.name ` const prowJobFailedTestsMatView = ` From 7f29fb95b28af3e9b284888f07ad10ae973de0fd Mon Sep 17 00:00:00 2001 From: Luke Meyer Date: Sun, 28 Jul 2024 15:14:24 -0400 Subject: [PATCH 5/7] WIP consume string test_id --- pkg/api/job_runs.go | 6 +++--- pkg/api/tests.go | 4 ++-- pkg/apis/api/types.go | 3 ++- pkg/db/query/test_queries.go | 2 +- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/pkg/api/job_runs.go b/pkg/api/job_runs.go index 686ad4661..0aa61046a 100644 --- a/pkg/api/job_runs.go +++ b/pkg/api/job_runs.go @@ -529,12 +529,12 @@ func runTestRunAnalysis(failedTest models.ProwJobRunTest, jobRun *models.ProwJob } // one of our data sources should have the test ID - testID := failedTest.Test.ID + testID := failedTest.Test.TestID if testID == 0 && testResultsJobNames != nil { - testID = uint(testResultsJobNames.ID) + testID = testResultsJobNames.TestID } if testID == 0 && testResultsVariants != nil { - testID = uint(testResultsVariants.ID) + testID = testResultsVariants.TestID } analysis := apitype.ProwJobRunTestRiskAnalysis{ Name: failedTest.Test.Name, diff --git a/pkg/api/tests.go b/pkg/api/tests.go index 8f0d2c705..b17e40974 100644 --- a/pkg/api/tests.go +++ b/pkg/api/tests.go @@ -195,7 +195,7 @@ func BuildTestsResults(dbc *db.DB, release, period string, collapse, includeOver // Collapse groups the test results together -- otherwise we return the test results per-variant combo (NURP+) variantSelect := "" if collapse { - rawQuery = rawQuery.Select(`id,name,watchlist,jira_component,jira_component_id,` + query.QueryTestSummer).Group("id,name,watchlist,jira_component,jira_component_id") + rawQuery = rawQuery.Select(`test_id,name,watchlist,jira_component,jira_component_id,` + query.QueryTestSummer).Group("test_id,name,watchlist,jira_component,jira_component_id") } else { rawQuery = query.TestsByNURPAndStandardDeviation(dbc, release, table) variantSelect = "suite_name, variants," + @@ -211,7 +211,7 @@ func BuildTestsResults(dbc *db.DB, release, period string, collapse, includeOver testReports := make([]apitype.Test, 0) processedResults := dbc.DB.Table("(?) as results", rawQuery). - Select(`id, watchlist, name, jira_component, jira_component_id,` + variantSelect + query.QueryTestSummarizer). + Select(`ROW_NUMBER() OVER() as id, test_id, watchlist, name, jira_component, jira_component_id,` + variantSelect + query.QueryTestSummarizer). Where("current_runs > 0 or previous_runs > 0") finalResults := dbc.DB.Table("(?) as final_results", processedResults) diff --git a/pkg/apis/api/types.go b/pkg/apis/api/types.go index 429be3b87..6616a9a0c 100644 --- a/pkg/apis/api/types.go +++ b/pkg/apis/api/types.go @@ -452,6 +452,7 @@ func (run JobRun) GetArrayValue(param string) ([]string, error) { // of this struct is suitable for use in a data table. type Test struct { ID int `json:"id,omitempty"` + TestID string `json:"test_id,omitempty"` Name string `json:"name"` SuiteName string `json:"suite_name"` Variant string `json:"variant,omitempty"` @@ -757,7 +758,7 @@ type ProwJobRunRiskAnalysis struct { type ProwJobRunTestRiskAnalysis struct { Name string - TestID uint + TestID string Risk TestFailureRisk OpenBugs []models.Bug } diff --git a/pkg/db/query/test_queries.go b/pkg/db/query/test_queries.go index e6c9f25a0..1b96f67e5 100644 --- a/pkg/db/query/test_queries.go +++ b/pkg/db/query/test_queries.go @@ -55,7 +55,7 @@ const ( QueryTestSummarizer = QueryTestFields + "," + QueryTestPercentages - QueryTestAnalysis = "select test_id as id, current_successes, current_runs, current_successes * 100.0 / NULLIF(current_runs, 0) AS current_pass_percentage from ( select test_id, sum(runs) as current_runs, sum(passes) as current_successes from prow_test_analysis_by_job_14d_matview where test_name = '%s' AND job_name in (%s) GROUP BY test_id)t" + QueryTestAnalysis = "select test_id as id, current_successes, current_runs, current_successes * 100.0 / NULLIF(current_runs, 0) AS current_pass_percentage from ( select test_id, sum(runs) as current_runs, sum(passes) as current_successes from prow_test_analysis_by_job_14d_matview join where test_name = '%s' AND job_name in (%s) GROUP BY test_id)t" ) // TestReportsByVariant returns a test report for every test in the db matching the given substrings, separated by variant. From a75cdcfa38fb74d9573078f9ba949363288bbf69 Mon Sep 17 00:00:00 2001 From: Luke Meyer Date: Mon, 29 Jul 2024 16:54:35 -0400 Subject: [PATCH 6/7] back to int test ids --- pkg/api/job_runs.go | 6 +++--- pkg/apis/api/types.go | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/api/job_runs.go b/pkg/api/job_runs.go index 0aa61046a..145321ba8 100644 --- a/pkg/api/job_runs.go +++ b/pkg/api/job_runs.go @@ -529,12 +529,12 @@ func runTestRunAnalysis(failedTest models.ProwJobRunTest, jobRun *models.ProwJob } // one of our data sources should have the test ID - testID := failedTest.Test.TestID + testID := failedTest.TestID if testID == 0 && testResultsJobNames != nil { - testID = testResultsJobNames.TestID + testID = uint(testResultsJobNames.TestID) } if testID == 0 && testResultsVariants != nil { - testID = testResultsVariants.TestID + testID = uint(testResultsVariants.TestID) } analysis := apitype.ProwJobRunTestRiskAnalysis{ Name: failedTest.Test.Name, diff --git a/pkg/apis/api/types.go b/pkg/apis/api/types.go index 6616a9a0c..8953ff81c 100644 --- a/pkg/apis/api/types.go +++ b/pkg/apis/api/types.go @@ -452,7 +452,7 @@ func (run JobRun) GetArrayValue(param string) ([]string, error) { // of this struct is suitable for use in a data table. type Test struct { ID int `json:"id,omitempty"` - TestID string `json:"test_id,omitempty"` + TestID int `json:"test_id,omitempty"` Name string `json:"name"` SuiteName string `json:"suite_name"` Variant string `json:"variant,omitempty"` @@ -758,7 +758,7 @@ type ProwJobRunRiskAnalysis struct { type ProwJobRunTestRiskAnalysis struct { Name string - TestID string + TestID uint Risk TestFailureRisk OpenBugs []models.Bug } From 5dcaa3b4850b89ab59cea5141a55c5042fc5ab5e Mon Sep 17 00:00:00 2001 From: Luke Meyer Date: Mon, 29 Jul 2024 16:56:24 -0400 Subject: [PATCH 7/7] not all test ids have string equivalents --- pkg/db/matviews.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/db/matviews.go b/pkg/db/matviews.go index fd7471bcf..0010217a4 100644 --- a/pkg/db/matviews.go +++ b/pkg/db/matviews.go @@ -255,13 +255,13 @@ SELECT FROM prow_job_run_tests JOIN tests ON tests.id = prow_job_run_tests.test_id - JOIN test_ownerships town ON town.test_id = tests.id JOIN prow_job_runs ON prow_job_runs.id = prow_job_run_tests.prow_job_run_id JOIN prow_jobs ON prow_jobs.id = prow_job_runs.prow_job_id + LEFT JOIN test_ownerships town ON town.test_id = tests.id WHERE prow_job_run_tests.created_at > (|||TIMENOW||| - '14 days'::interval) AND prow_job_runs."timestamp" > (|||TIMENOW||| - '14 days'::interval) GROUP BY - tests.name, town.unique_id, tests.id, date(prow_job_runs."timestamp"), unnest(prow_jobs.variants), prow_jobs.release + tests.name, tests.id, town.unique_id, date(prow_job_runs."timestamp"), unnest(prow_jobs.variants), prow_jobs.release ` const testAnalysisByJobMatView = ` @@ -280,13 +280,13 @@ SELECT FROM prow_job_run_tests JOIN tests ON tests.id = prow_job_run_tests.test_id - JOIN test_ownerships town ON town.test_id = tests.id JOIN prow_job_runs ON prow_job_runs.id = prow_job_run_tests.prow_job_run_id JOIN prow_jobs ON prow_jobs.id = prow_job_runs.prow_job_id + LEFT JOIN test_ownerships town ON town.test_id = tests.id WHERE prow_job_run_tests.created_at > (|||TIMENOW||| - '14 days'::interval) AND prow_job_runs."timestamp" > (|||TIMENOW||| - '14 days'::interval) GROUP BY - tests.name, town.unique_id, tests.id, date(prow_job_runs."timestamp"), prow_jobs.release, prow_jobs.name + tests.name, tests.id, town.unique_id, date(prow_job_runs."timestamp"), prow_jobs.release, prow_jobs.name ` const prowJobFailedTestsMatView = `