Implement new group report by revision Bug: 438183175 Change-Id: Ib65cf47bb228373d6dc45f8c7655b9d7ab09b891 Reviewed-on: https://skia-review.googlesource.com/c/buildbot/+/1075396 Reviewed-by: Anri Sidorov <ansid@google.com> Commit-Queue: Marcin Mordecki <mordeckimarcin@google.com>
diff --git a/perf/go/frontend/api/BUILD.bazel b/perf/go/frontend/api/BUILD.bazel index 92bf89d..215f1cc 100644 --- a/perf/go/frontend/api/BUILD.bazel +++ b/perf/go/frontend/api/BUILD.bazel
@@ -91,6 +91,7 @@ "//go/alogin", "//go/alogin/mocks", "//go/roles", + "//go/skerr", "//go/testutils", "//perf/go/anomalygroup/mocks", "//perf/go/config",
diff --git a/perf/go/frontend/api/anomaliesApi.go b/perf/go/frontend/api/anomaliesApi.go index be22384..b6e0226 100644 --- a/perf/go/frontend/api/anomaliesApi.go +++ b/perf/go/frontend/api/anomaliesApi.go
@@ -469,9 +469,7 @@ sklog.Debugf("Unsupported parameters for group report: %v", groupReportRequest) return } else if groupReportRequest.Revison != "" { - httputils.ReportError(w, errors.New("not implemented"), "This API is not implemented for this parameter.", http.StatusInternalServerError) - sklog.Debugf("Unsupported parameters for group report: %v", groupReportRequest) - return + groupReportResponse, err = api.getGroupReportByRevision(ctx, groupReportRequest) } else if groupReportRequest.AnomalyGroupID != "" { groupReportResponse, err = api.getGroupReportByAnomalyGroupId(ctx, groupReportRequest) } else {
diff --git a/perf/go/frontend/api/anomaliesApi_test.go b/perf/go/frontend/api/anomaliesApi_test.go index a60549a..e1b8b02 100644 --- a/perf/go/frontend/api/anomaliesApi_test.go +++ b/perf/go/frontend/api/anomaliesApi_test.go
@@ -7,6 +7,7 @@ "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.skia.org/infra/go/skerr" "go.skia.org/infra/go/testutils" "go.skia.org/infra/perf/go/anomalygroup/mocks" "go.skia.org/infra/perf/go/config" @@ -201,6 +202,93 @@ regStore.AssertExpectations(t) } +func TestGetGroupReportByRevision(t *testing.T) { + api, anomalygroupStore, regStore := setupAnomaliesApiWithMocks(t) + + ctx := context.Background() + anomalyIds := []string{"anom-id-1", "anom-id-2"} + traceset := ",arch=x86,bot=linux,benchmark=jetstream2,test=score,config=default,master=main," + revision := "1" + + // Mock the response from the regStore. + regressions := []*regression.Regression{ + { + Id: "anom-id-1", + Frame: &frame.FrameResponse{ + DataFrame: &dataframe.DataFrame{ + TraceSet: types.TraceSet{ + traceset: []float32{1.0}, + }, + }, + }, + }, + { + Id: "anom-id-2", + Frame: &frame.FrameResponse{ + DataFrame: &dataframe.DataFrame{ + TraceSet: types.TraceSet{ + traceset: []float32{2.0}, + }, + }, + }, + }, + } + regStore.On("GetByRevision", ctx, revision).Return(regressions, nil).Once() + + // Create the request. + req := GetGroupReportRequest{ + Revison: revision, + } + + // Call the function under test. + resp, err := api.getGroupReportByRevision(ctx, req) + + // Assert the results. + require.NoError(t, err) + require.NotNil(t, resp) + assert.Len(t, resp.Anomalies, 2) + // Note: compat.ConvertRegressionToAnomalies is not mocked, so we can't check all fields. + // We are just checking the Id. + for _, id := range anomalyIds { + idPresent := false + for _, anomaly := range resp.Anomalies { + if anomaly.Id == id { + idPresent = true + break + } + } + require.True(t, idPresent) + } + // Ensure the mocks were called as expected. + anomalygroupStore.AssertExpectations(t) + regStore.AssertExpectations(t) +} + +func TestGetGroupReportByRevision_InvalidRevisionsAreRejected(t *testing.T) { + api, anomalygroupStore, regStore := setupAnomaliesApiWithMocks(t) + + ctx := context.Background() + + badRevision := "not-a-number" + + // Create the request. + req := GetGroupReportRequest{ + Revison: badRevision, + } + + regStore.On("GetByRevision", ctx, badRevision).Return(nil, skerr.Fmt("error")) + + // Call the function under test. + resp, err := api.getGroupReportByRevision(ctx, req) + + // Assert the results. + require.Error(t, err) + require.Nil(t, resp) + // Ensure the mocks were called as expected. + anomalygroupStore.AssertExpectations(t) + regStore.AssertExpectations(t) +} + func TestAnomaliesApi_CleanTestName_Default(t *testing.T) { configFileBytes := testutils.ReadFileBytes(t, "config.json") err := json.Unmarshal(configFileBytes, &config.Config)
diff --git a/perf/go/frontend/api/anomaliesGetGroupReportBy.go b/perf/go/frontend/api/anomaliesGetGroupReportBy.go index 861844b..059735f 100644 --- a/perf/go/frontend/api/anomaliesGetGroupReportBy.go +++ b/perf/go/frontend/api/anomaliesGetGroupReportBy.go
@@ -2,24 +2,67 @@ import ( "context" - "fmt" "strings" + "go.skia.org/infra/go/skerr" "go.skia.org/infra/go/sklog" "go.skia.org/infra/perf/go/chromeperf" "go.skia.org/infra/perf/go/chromeperf/compat" + "go.skia.org/infra/perf/go/regression" ) +// GetGroupReport for regressions that match GetGroupReportRequest.anomalyIDs list +func (api anomaliesApi) getGroupReportByAnomalyId(ctx context.Context, groupReportRequest GetGroupReportRequest) (*GetGroupReportResponse, error) { + anomalyIds := strings.Split(groupReportRequest.AnomalyIDs, ",") + return api.getGroupReportByAnomalyIdList(ctx, &anomalyIds) +} + +// GetGroupReport for regressions that match GetGroupReportRequest.BugID +func (api anomaliesApi) getGroupReportByBugId(ctx context.Context, groupReportRequest GetGroupReportRequest) (*GetGroupReportResponse, error) { + id := groupReportRequest.BugID + anomalyIds, err := api.anomalygroupStore.GetAnomalyIdsByIssueId(ctx, id) + if err != nil { + return nil, skerr.Fmt("failed to get anomalyIds from anomalygroup Store by issue ID: %s", err) + } + // TODO(b/438183175) query from Culprits, too. Looks like reported_issue_id can be all null, even though we have ongoing bugs. + return api.getGroupReportByAnomalyIdList(ctx, &anomalyIds) +} + +// GetGroupReport for regressions that match GetGroupReportRequest.AnomalyGroupId +func (api anomaliesApi) getGroupReportByAnomalyGroupId(ctx context.Context, groupReportRequest GetGroupReportRequest) (*GetGroupReportResponse, error) { + id := groupReportRequest.AnomalyGroupID + anomalyIds, err := api.anomalygroupStore.GetAnomalyIdsByAnomalyGroupId(ctx, id) + if err != nil { + return nil, skerr.Fmt("failed to get anomalyIds from anomalygroup Store by anomaly group ID: %s", err) + } + + return api.getGroupReportByAnomalyIdList(ctx, &anomalyIds) +} + +// GetGroupReport for regressions that match GetGroupReportRequest.Revision +func (api anomaliesApi) getGroupReportByRevision(ctx context.Context, groupReportRequest GetGroupReportRequest) (*GetGroupReportResponse, error) { + rev := groupReportRequest.Revison + regressions, err := api.regStore.GetByRevision(ctx, rev) + if err != nil { + return nil, skerr.Fmt("failed to get anomalyIds from anomalygroup Store by Revision: %s", err) + } + + return prepareResponseFromRegressions(regressions) +} + +// Given a list of anomaly IDs, fill GetGroupReportResponse Anomalies list. +func (api anomaliesApi) getGroupReportByAnomalyIdList(ctx context.Context, anomalyIds *[]string) (*GetGroupReportResponse, error) { + regressions, err := api.regStore.GetByIDs(ctx, *anomalyIds) + if err != nil { + return nil, skerr.Fmt("failed to get regressions by ID: %s", err) + } + return prepareResponseFromRegressions(regressions) +} + // TODO(b/438183175) Populate remaining fields of GetGroupReportResponse: // StateId, SelectedKeys, Error, TimerangeMap -// Given a list of anomaly IDs (regressions and possibly improvements), -// fill GetGroupReportResponse Anomalies list using just regressions. -func (api anomaliesApi) getGroupReportByAnomalyIdList(ctx context.Context, anomalyIds *[]string) (*GetGroupReportResponse, error) { +func prepareResponseFromRegressions(regressions []*regression.Regression) (*GetGroupReportResponse, error) { groupReportResponse := &GetGroupReportResponse{} - regressions, err := api.regStore.GetByIDs(ctx, *anomalyIds) - if err != nil { - return nil, fmt.Errorf("Failed to get regressions by ID: %s", err) - } groupReportResponse.Anomalies = make([]chromeperf.Anomaly, 0) for _, reg := range regressions { anomalies, err := compat.ConvertRegressionToAnomalies(reg) @@ -35,31 +78,3 @@ } return groupReportResponse, nil } - -// GetGroupReport for regressions that match GetGroupReportRequest.anomalyIDs list -func (api anomaliesApi) getGroupReportByAnomalyId(ctx context.Context, groupReportRequest GetGroupReportRequest) (*GetGroupReportResponse, error) { - anomalyIds := strings.Split(groupReportRequest.AnomalyIDs, ",") - return api.getGroupReportByAnomalyIdList(ctx, &anomalyIds) -} - -// GetGroupReport for regressions that match GetGroupReportRequest.BugID -func (api anomaliesApi) getGroupReportByBugId(ctx context.Context, groupReportRequest GetGroupReportRequest) (*GetGroupReportResponse, error) { - id := groupReportRequest.BugID - anomalyIds, err := api.anomalygroupStore.GetAnomalyIdsByIssueId(ctx, id) - if err != nil { - return nil, fmt.Errorf("Failed to get anomalyIds from anomalygroup Store by issue ID: %s", err) - } - // TODO(b/438183175) query from Culprits, too. Looks like reported_issue_id can be all null, even though we have ongoing bugs. - return api.getGroupReportByAnomalyIdList(ctx, &anomalyIds) -} - -// GetGroupReport for regressions that match GetGroupReportRequest.AnomalyGroupId -func (api anomaliesApi) getGroupReportByAnomalyGroupId(ctx context.Context, groupReportRequest GetGroupReportRequest) (*GetGroupReportResponse, error) { - id := groupReportRequest.AnomalyGroupID - anomalyIds, err := api.anomalygroupStore.GetAnomalyIdsByAnomalyGroupId(ctx, id) - if err != nil { - return nil, fmt.Errorf("Failed to get anomalyIds from anomalygroup Store by anomaly group ID: %s", err) - } - - return api.getGroupReportByAnomalyIdList(ctx, &anomalyIds) -}
diff --git a/perf/go/regression/mocks/Store.go b/perf/go/regression/mocks/Store.go index 9f41456..13d9662 100644 --- a/perf/go/regression/mocks/Store.go +++ b/perf/go/regression/mocks/Store.go
@@ -71,6 +71,36 @@ return r0, r1 } +// GetByRevision provides a mock function with given fields: ctx, rev +func (_m *Store) GetByRevision(ctx context.Context, rev string) ([]*regression.Regression, error) { + ret := _m.Called(ctx, rev) + + if len(ret) == 0 { + panic("no return value specified for GetByRevision") + } + + var r0 []*regression.Regression + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, string) ([]*regression.Regression, error)); ok { + return rf(ctx, rev) + } + if rf, ok := ret.Get(0).(func(context.Context, string) []*regression.Regression); ok { + r0 = rf(ctx, rev) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]*regression.Regression) + } + } + + if rf, ok := ret.Get(1).(func(context.Context, string) error); ok { + r1 = rf(ctx, rev) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // GetOldestCommit provides a mock function with given fields: ctx func (_m *Store) GetOldestCommit(ctx context.Context) (*types.CommitNumber, error) { ret := _m.Called(ctx)
diff --git a/perf/go/regression/sqlregression2store/BUILD.bazel b/perf/go/regression/sqlregression2store/BUILD.bazel index 2110faf..6745d25 100644 --- a/perf/go/regression/sqlregression2store/BUILD.bazel +++ b/perf/go/regression/sqlregression2store/BUILD.bazel
@@ -43,5 +43,6 @@ "@com_github_google_uuid//:uuid", "@com_github_jackc_pgconn//:pgconn", "@com_github_stretchr_testify//assert", + "@com_github_stretchr_testify//require", ], )
diff --git a/perf/go/regression/sqlregression2store/sqlregression2store.go b/perf/go/regression/sqlregression2store/sqlregression2store.go index 51b4771..1bb0a2a 100644 --- a/perf/go/regression/sqlregression2store/sqlregression2store.go +++ b/perf/go/regression/sqlregression2store/sqlregression2store.go
@@ -5,6 +5,7 @@ "context" "errors" "fmt" + "strconv" "strings" "text/template" "time" @@ -46,6 +47,7 @@ readCompat readOldest readRange + readByRev readByIDs readBySubName deleteByCommit @@ -89,6 +91,15 @@ commit_number >= $1 AND commit_number <= $2 `, + readByRev: ` + SELECT + {{ .Columns }} + FROM + Regressions2 + WHERE + prev_commit_number < $1 + AND commit_number >= $1 + `, readRangeFiltered: ` SELECT {{ .Columns }} @@ -361,6 +372,30 @@ return regressions, nil } +// Get a list of regressions given a revision. +func (s *SQLRegression2Store) GetByRevision(ctx context.Context, rev string) ([]*regression.Regression, error) { + var regressions []*regression.Regression + revInt, err := strconv.ParseInt(rev, 10, 64) + if err != nil { + return regressions, skerr.Fmt("failed to convert rev %s to int: %s", rev, err) + } + statement := s.statements[readByRev] + rows, err := s.db.Query(ctx, statement, revInt) + if err != nil { + return nil, skerr.Wrapf(err, "failed to get regressions by revision") + } + + for rows.Next() { + r, err := convertRowToRegression(rows) + if err != nil { + return nil, skerr.Wrapf(err, "failed to convert row to regression.") + } + regressions = append(regressions, r) + } + + return regressions, nil +} + // convertRowToRegression converts the content of the row retrieved from the database // into a regression object. This will return an error if either there is no data // in the row, or if the data is invalid (eg: failed data conversion).
diff --git a/perf/go/regression/sqlregression2store/sqlregression2store_test.go b/perf/go/regression/sqlregression2store/sqlregression2store_test.go index ae41ad9..8aa71a5 100644 --- a/perf/go/regression/sqlregression2store/sqlregression2store_test.go +++ b/perf/go/regression/sqlregression2store/sqlregression2store_test.go
@@ -10,6 +10,7 @@ "github.com/google/uuid" "github.com/jackc/pgconn" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "go.skia.org/infra/perf/go/alerts" alerts_mock "go.skia.org/infra/perf/go/alerts/mock" "go.skia.org/infra/perf/go/clustering2" @@ -76,14 +77,6 @@ return r } -func generateAndStoreNewRegressionImprovement(ctx context.Context, t *testing.T, store *SQLRegression2Store) *regression.Regression { - r := generateNewRegression() - r.IsImprovement = true - _, err := store.WriteRegression(ctx, r, nil) - assert.Nil(t, err) - return r -} - func assertRegression(t *testing.T, expected *regression.Regression, actual *regression.Regression) { assert.Equal(t, expected.AlertId, actual.AlertId) assert.Equal(t, expected.CommitNumber, actual.CommitNumber) @@ -142,8 +135,13 @@ ctx := context.Background() r := generateAndStoreNewRegression(ctx, t, store) r2 := generateAndStoreNewRegression(ctx, t, store) + // Improvements are anomalies, and they are stored, too. - rImprovement := generateAndStoreNewRegressionImprovement(ctx, t, store) + rImprovement := generateNewRegression() + populateRegression2Fields(rImprovement) + rImprovement.IsImprovement = true + err := store.writeSingleRegression(ctx, rImprovement, nil) + assert.Nil(t, err) tests := []struct { name string @@ -195,6 +193,87 @@ } } +// TestGetByIDs_Success reads the database using the +// ids of the created regressions. +func TestGetByRevision_Success(t *testing.T) { + alertsProvider := alerts_mock.NewConfigProvider(t) + + store := setupStore(t, alertsProvider) + ctx := context.Background() + + generateRegression := func(previousCommit int64, commit int64) (r *regression.Regression) { + r = generateNewRegression() + populateRegression2Fields(r) + r.PrevCommitNumber = types.CommitNumber(previousCommit) + r.CommitNumber = types.CommitNumber(commit) + err := store.writeSingleRegression(ctx, r, nil) + require.NoError(t, err) + return + } + + r100_200 := generateRegression(100, 200) + r101_200 := generateRegression(101, 200) + r300_301 := generateRegression(300, 301) + + tests := []struct { + name string + revision string + shouldContainIDs []string + }{ + { + name: "revision inside two regressions", + revision: "102", + shouldContainIDs: []string{r100_200.Id, r101_200.Id}, + }, + { + name: "inside a regression and at the beginning of another", + revision: "101", + shouldContainIDs: []string{r100_200.Id}, + }, + { + name: "beginning of a regression and before others", + revision: "100", + shouldContainIDs: []string{}, + }, + { + name: "before all regressions", + revision: "99", + shouldContainIDs: []string{}, + }, + { + name: "just before the end of regressions", + revision: "199", + shouldContainIDs: []string{r100_200.Id, r101_200.Id}, + }, + { + name: "coinciding with the commit number of two regressions", + revision: "200", + shouldContainIDs: []string{r100_200.Id, r101_200.Id}, + }, + { + name: "right after some regressions", + revision: "201", + shouldContainIDs: []string{}, + }, + { + name: "inside a 1-wide regression", + revision: "301", + shouldContainIDs: []string{r300_301.Id}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + regressions, err := store.GetByRevision(ctx, tc.revision) + assert.NoError(t, err) + assert.Equal(t, len(tc.shouldContainIDs), len(regressions)) + for _, r := range regressions { + assert.Contains(t, tc.shouldContainIDs, r.Id) + } + }) + } +} + // TestHighRegression_KMeans_Triage sets a High regression into the database, triages it // and verifies that the data was updated correctly. The alert Algo is set to be KMeans. func TestHighRegression_KMeans_Triage(t *testing.T) {
diff --git a/perf/go/regression/sqlregressionstore/sqlregressionstore.go b/perf/go/regression/sqlregressionstore/sqlregressionstore.go index ad79019..7569d80 100644 --- a/perf/go/regression/sqlregressionstore/sqlregressionstore.go +++ b/perf/go/regression/sqlregressionstore/sqlregressionstore.go
@@ -377,6 +377,11 @@ return nil, skerr.Fmt("GetByIDs are not implemented in old version of regression store.") } +// Not implemented, old regression will not be developed +func (s *SQLRegressionStore) GetByRevision(ctx context.Context, revision string) ([]*regression.Regression, error) { + return nil, skerr.Fmt("GetByRev is not implemented in old version of regression store.") +} + // GetOldestCommit implements the regression.Store interface. Gets the oldest commit in the table. func (s *SQLRegressionStore) GetOldestCommit(ctx context.Context) (*types.CommitNumber, error) { var num int
diff --git a/perf/go/regression/types.go b/perf/go/regression/types.go index f68df83..5b5b375 100644 --- a/perf/go/regression/types.go +++ b/perf/go/regression/types.go
@@ -44,6 +44,9 @@ // return a list of regressions. GetByIDs(ctx context.Context, ids []string) ([]*Regression, error) + // Return a list of regressions satisfying: previous_commit < rev <= commit. + GetByRevision(ctx context.Context, rev string) ([]*Regression, error) + // GetOldestCommit returns the commit with the lowest commit number GetOldestCommit(ctx context.Context) (*types.CommitNumber, error)