From 658148fecf90955f92405fedb2c698fa046b6ce7 Mon Sep 17 00:00:00 2001 From: Andrew Thorp Date: Wed, 5 Nov 2025 14:06:48 -0500 Subject: [PATCH 1/2] feat(perf): cache `vcs.GetFiles()` to reduce redundant VCS API volume Calls to `vcs.GetFiles()` only result in one or two API calls (paging aside). However the method is called each time a CEL expression references `"path".pathChanged()` or the `on-path-change` annotation is checked for matches. Because of this, because matching is evaluated several times in a row, and because some users have tens of PipelineRuns in their `.tekton` directory, in some cases a single push event may cause PaC to make hundreds of API requests listing the same files repeatedly. --- .../bitbucketdatacenter.go | 126 +++++++++--------- .../bitbucketdatacenter_test.go | 25 +++- pkg/provider/github/github.go | 102 +++++++------- pkg/provider/github/github_test.go | 69 ++++------ pkg/provider/gitlab/gitlab.go | 109 +++++++-------- pkg/provider/gitlab/gitlab_test.go | 58 +++----- pkg/provider/metrics/metrics_test.go | 12 +- pkg/reconciler/emit_metrics_test.go | 19 +-- pkg/test/metrics/metrics.go | 18 +++ 9 files changed, 265 insertions(+), 273 deletions(-) create mode 100644 pkg/test/metrics/metrics.go diff --git a/pkg/provider/bitbucketdatacenter/bitbucketdatacenter.go b/pkg/provider/bitbucketdatacenter/bitbucketdatacenter.go index fe48a91ec5..7a34c3df5b 100644 --- a/pkg/provider/bitbucketdatacenter/bitbucketdatacenter.go +++ b/pkg/provider/bitbucketdatacenter/bitbucketdatacenter.go @@ -42,6 +42,7 @@ type Provider struct { projectKey string repo *v1alpha1.Repository triggerEvent string + changedFiles *changedfiles.ChangedFiles } func (v Provider) Client() *scm.Client { @@ -371,79 +372,82 @@ func (v *Provider) GetConfig() *info.ProviderConfig { } func (v *Provider) GetFiles(ctx context.Context, runevent *info.Event) (changedfiles.ChangedFiles, error) { - OrgAndRepo := fmt.Sprintf("%s/%s", runevent.Organization, runevent.Repository) - if runevent.TriggerTarget == triggertype.PullRequest { - opts := &scm.ListOptions{Page: 1, Size: apiResponseLimit} - changedFiles := changedfiles.ChangedFiles{} - for { - changes, _, err := v.Client().PullRequests.ListChanges(ctx, OrgAndRepo, runevent.PullRequestNumber, opts) - if err != nil { - return changedfiles.ChangedFiles{}, fmt.Errorf("failed to list changes for pull request: %w", err) - } - - for _, c := range changes { - changedFiles.All = append(changedFiles.All, c.Path) - if c.Added { - changedFiles.Added = append(changedFiles.Added, c.Path) - } - if c.Modified { - changedFiles.Modified = append(changedFiles.Modified, c.Path) - } - if c.Renamed { - changedFiles.Renamed = append(changedFiles.Renamed, c.Path) + if v.changedFiles == nil { + OrgAndRepo := fmt.Sprintf("%s/%s", runevent.Organization, runevent.Repository) + switch runevent.TriggerTarget { + case triggertype.PullRequest: + opts := &scm.ListOptions{Page: 1, Size: apiResponseLimit} + changedFiles := changedfiles.ChangedFiles{} + for { + changes, _, err := v.Client().PullRequests.ListChanges(ctx, OrgAndRepo, runevent.PullRequestNumber, opts) + if err != nil { + return changedfiles.ChangedFiles{}, fmt.Errorf("failed to list changes for pull request: %w", err) } - if c.Deleted { - changedFiles.Deleted = append(changedFiles.Deleted, c.Path) - } - } - // In the Jenkins-x/go-scm package, the `isLastPage` field is not available, and the value of - // `response.Page.Last` is set to `0`. Therefore, to determine if there are more items to fetch, - // we can check if the length of the currently fetched items is less than the specified limit. - // If the length is less than the limit, it indicates that there are no more items to retrieve. - if len(changes) < apiResponseLimit { - break - } + for _, c := range changes { + changedFiles.All = append(changedFiles.All, c.Path) + if c.Added { + changedFiles.Added = append(changedFiles.Added, c.Path) + } + if c.Modified { + changedFiles.Modified = append(changedFiles.Modified, c.Path) + } + if c.Renamed { + changedFiles.Renamed = append(changedFiles.Renamed, c.Path) + } + if c.Deleted { + changedFiles.Deleted = append(changedFiles.Deleted, c.Path) + } + } - opts.Page++ - } - return changedFiles, nil - } + // In the Jenkins-x/go-scm package, the `isLastPage` field is not available, and the value of + // `response.Page.Last` is set to `0`. Therefore, to determine if there are more items to fetch, + // we can check if the length of the currently fetched items is less than the specified limit. + // If the length is less than the limit, it indicates that there are no more items to retrieve. + if len(changes) < apiResponseLimit { + break + } - if runevent.TriggerTarget == triggertype.Push { - opts := &scm.ListOptions{Page: 1, Size: apiResponseLimit} - changedFiles := changedfiles.ChangedFiles{} - for { - changes, _, err := v.Client().Git.ListChanges(ctx, OrgAndRepo, runevent.SHA, opts) - if err != nil { - return changedfiles.ChangedFiles{}, fmt.Errorf("failed to list changes for commit %s: %w", runevent.SHA, err) + opts.Page++ } - - for _, c := range changes { - changedFiles.All = append(changedFiles.All, c.Path) - if c.Added { - changedFiles.Added = append(changedFiles.Added, c.Path) - } - if c.Modified { - changedFiles.Modified = append(changedFiles.Modified, c.Path) + v.changedFiles = &changedFiles + case triggertype.Push: + opts := &scm.ListOptions{Page: 1, Size: apiResponseLimit} + changedFiles := changedfiles.ChangedFiles{} + for { + changes, _, err := v.Client().Git.ListChanges(ctx, OrgAndRepo, runevent.SHA, opts) + if err != nil { + return changedfiles.ChangedFiles{}, fmt.Errorf("failed to list changes for commit %s: %w", runevent.SHA, err) } - if c.Renamed { - changedFiles.Renamed = append(changedFiles.Renamed, c.Path) + + for _, c := range changes { + changedFiles.All = append(changedFiles.All, c.Path) + if c.Added { + changedFiles.Added = append(changedFiles.Added, c.Path) + } + if c.Modified { + changedFiles.Modified = append(changedFiles.Modified, c.Path) + } + if c.Renamed { + changedFiles.Renamed = append(changedFiles.Renamed, c.Path) + } + if c.Deleted { + changedFiles.Deleted = append(changedFiles.Deleted, c.Path) + } } - if c.Deleted { - changedFiles.Deleted = append(changedFiles.Deleted, c.Path) + + if len(changes) < apiResponseLimit { + break } - } - if len(changes) < apiResponseLimit { - break + opts.Page++ } - - opts.Page++ + v.changedFiles = &changedFiles + default: + v.changedFiles = &changedfiles.ChangedFiles{} } - return changedFiles, nil } - return changedfiles.ChangedFiles{}, nil + return *v.changedFiles, nil } func (v *Provider) CreateToken(_ context.Context, _ []string, _ *info.Event) (string, error) { diff --git a/pkg/provider/bitbucketdatacenter/bitbucketdatacenter_test.go b/pkg/provider/bitbucketdatacenter/bitbucketdatacenter_test.go index b786939c8b..5829693815 100644 --- a/pkg/provider/bitbucketdatacenter/bitbucketdatacenter_test.go +++ b/pkg/provider/bitbucketdatacenter/bitbucketdatacenter_test.go @@ -22,11 +22,14 @@ import ( "github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype" "github.com/openshift-pipelines/pipelines-as-code/pkg/provider" bbtest "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/bitbucketdatacenter/test" + "github.com/openshift-pipelines/pipelines-as-code/pkg/test/metrics" "github.com/jenkins-x/go-scm/scm" "go.uber.org/zap" zapobserver "go.uber.org/zap/zaptest/observer" "gotest.tools/v3/assert" + "knative.dev/pkg/metrics/metricstest" + _ "knative.dev/pkg/metrics/testing" rtesting "knative.dev/pkg/reconciler/testing" ) @@ -796,6 +799,7 @@ func TestGetFiles(t *testing.T) { ctx, _ := rtesting.SetupFakeContext(t) client, mux, tearDown, tURL := bbtest.SetupBBDataCenterClient() defer tearDown() + metrics.ResetMetrics() stats := &bbtest.DiffStats{ Values: tt.changeFiles, @@ -821,13 +825,17 @@ func TestGetFiles(t *testing.T) { } }) } - v := &Provider{client: client, baseURL: tURL} + + metricsTags := map[string]string{"provider": "bitbucket-datacenter", "event-type": string(tt.event.TriggerTarget)} + metricstest.CheckStatsNotReported(t, "pipelines_as_code_git_provider_api_request_count") + + v := &Provider{client: client, baseURL: tURL, triggerEvent: string(tt.event.TriggerTarget)} changedFiles, err := v.GetFiles(ctx, tt.event) if tt.wantError { assert.Equal(t, err.Error(), tt.errMsg) - return + } else { + assert.NilError(t, err, nil) } - assert.NilError(t, err, nil) assert.Equal(t, tt.wantAddedFilesCount, len(changedFiles.Added)) assert.Equal(t, tt.wantDeletedFilesCount, len(changedFiles.Deleted)) assert.Equal(t, tt.wantModifiedFilesCount, len(changedFiles.Modified)) @@ -844,6 +852,17 @@ func TestGetFiles(t *testing.T) { assert.Equal(t, tt.changeFiles[i].Path.ToString, changedFiles.All[i]) } } + + // Check caching + metricstest.CheckCountData(t, "pipelines_as_code_git_provider_api_request_count", metricsTags, 1) + _, _ = v.GetFiles(ctx, tt.event) + if tt.wantError { + // No caching on error + metricstest.CheckCountData(t, "pipelines_as_code_git_provider_api_request_count", metricsTags, 2) + } else { + // Cache API results on success + metricstest.CheckCountData(t, "pipelines_as_code_git_provider_api_request_count", metricsTags, 1) + } }) } } diff --git a/pkg/provider/github/github.go b/pkg/provider/github/github.go index dc74ebec04..4cb311b165 100644 --- a/pkg/provider/github/github.go +++ b/pkg/provider/github/github.go @@ -56,6 +56,7 @@ type Provider struct { userType string // The type of user i.e bot or not skippedRun triggerEvent string + changedFiles *changedfiles.ChangedFiles } type skippedRun struct { @@ -517,67 +518,70 @@ func (v *Provider) getPullRequest(ctx context.Context, runevent *info.Event) (*i return runevent, nil } -// GetFiles get a files from pull request. +// GetFiles gets the files changed by a given event. func (v *Provider) GetFiles(ctx context.Context, runevent *info.Event) (changedfiles.ChangedFiles, error) { - if runevent.TriggerTarget == triggertype.PullRequest { - opt := &github.ListOptions{PerPage: v.PaginedNumber} - changedFiles := changedfiles.ChangedFiles{} - for { - repoCommit, resp, err := wrapAPI(v, "list_pull_request_files", func() ([]*github.CommitFile, *github.Response, error) { - return v.Client().PullRequests.ListFiles(ctx, runevent.Organization, runevent.Repository, runevent.PullRequestNumber, opt) + if v.changedFiles == nil { + switch runevent.TriggerTarget { + case triggertype.PullRequest: + opt := &github.ListOptions{PerPage: v.PaginedNumber} + changedFiles := changedfiles.ChangedFiles{} + for { + repoCommit, resp, err := wrapAPI(v, "list_pull_request_files", func() ([]*github.CommitFile, *github.Response, error) { + return v.Client().PullRequests.ListFiles(ctx, runevent.Organization, runevent.Repository, runevent.PullRequestNumber, opt) + }) + if err != nil { + return changedfiles.ChangedFiles{}, err + } + for j := range repoCommit { + changedFiles.All = append(changedFiles.All, *repoCommit[j].Filename) + if *repoCommit[j].Status == "added" { + changedFiles.Added = append(changedFiles.Added, *repoCommit[j].Filename) + } + if *repoCommit[j].Status == "removed" { + changedFiles.Deleted = append(changedFiles.Deleted, *repoCommit[j].Filename) + } + if *repoCommit[j].Status == "modified" { + changedFiles.Modified = append(changedFiles.Modified, *repoCommit[j].Filename) + } + if *repoCommit[j].Status == "renamed" { + changedFiles.Renamed = append(changedFiles.Renamed, *repoCommit[j].Filename) + } + } + if resp.NextPage == 0 { + break + } + opt.Page = resp.NextPage + } + v.changedFiles = &changedFiles + case triggertype.Push: + changedFiles := changedfiles.ChangedFiles{} + rC, _, err := wrapAPI(v, "get_commit_files", func() (*github.RepositoryCommit, *github.Response, error) { + return v.Client().Repositories.GetCommit(ctx, runevent.Organization, runevent.Repository, runevent.SHA, &github.ListOptions{}) }) if err != nil { return changedfiles.ChangedFiles{}, err } - for j := range repoCommit { - changedFiles.All = append(changedFiles.All, *repoCommit[j].Filename) - if *repoCommit[j].Status == "added" { - changedFiles.Added = append(changedFiles.Added, *repoCommit[j].Filename) + for i := range rC.Files { + changedFiles.All = append(changedFiles.All, *rC.Files[i].Filename) + if *rC.Files[i].Status == "added" { + changedFiles.Added = append(changedFiles.Added, *rC.Files[i].Filename) } - if *repoCommit[j].Status == "removed" { - changedFiles.Deleted = append(changedFiles.Deleted, *repoCommit[j].Filename) + if *rC.Files[i].Status == "removed" { + changedFiles.Deleted = append(changedFiles.Deleted, *rC.Files[i].Filename) } - if *repoCommit[j].Status == "modified" { - changedFiles.Modified = append(changedFiles.Modified, *repoCommit[j].Filename) + if *rC.Files[i].Status == "modified" { + changedFiles.Modified = append(changedFiles.Modified, *rC.Files[i].Filename) } - if *repoCommit[j].Status == "renamed" { - changedFiles.Renamed = append(changedFiles.Renamed, *repoCommit[j].Filename) + if *rC.Files[i].Status == "renamed" { + changedFiles.Renamed = append(changedFiles.Renamed, *rC.Files[i].Filename) } } - if resp.NextPage == 0 { - break - } - opt.Page = resp.NextPage - } - return changedFiles, nil - } - - if runevent.TriggerTarget == "push" { - changedFiles := changedfiles.ChangedFiles{} - rC, _, err := wrapAPI(v, "get_commit_files", func() (*github.RepositoryCommit, *github.Response, error) { - return v.Client().Repositories.GetCommit(ctx, runevent.Organization, runevent.Repository, runevent.SHA, &github.ListOptions{}) - }) - if err != nil { - return changedfiles.ChangedFiles{}, err - } - for i := range rC.Files { - changedFiles.All = append(changedFiles.All, *rC.Files[i].Filename) - if *rC.Files[i].Status == "added" { - changedFiles.Added = append(changedFiles.Added, *rC.Files[i].Filename) - } - if *rC.Files[i].Status == "removed" { - changedFiles.Deleted = append(changedFiles.Deleted, *rC.Files[i].Filename) - } - if *rC.Files[i].Status == "modified" { - changedFiles.Modified = append(changedFiles.Modified, *rC.Files[i].Filename) - } - if *rC.Files[i].Status == "renamed" { - changedFiles.Renamed = append(changedFiles.Renamed, *rC.Files[i].Filename) - } + v.changedFiles = &changedFiles + default: + v.changedFiles = &changedfiles.ChangedFiles{} } - return changedFiles, nil } - return changedfiles.ChangedFiles{}, nil + return *v.changedFiles, nil } // getObject Get an object from a repository. diff --git a/pkg/provider/github/github_test.go b/pkg/provider/github/github_test.go index 38d544b67d..2a1fa36a1f 100644 --- a/pkg/provider/github/github_test.go +++ b/pkg/provider/github/github_test.go @@ -20,7 +20,6 @@ import ( "github.com/jonboulle/clockwork" "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys" "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" - "github.com/openshift-pipelines/pipelines-as-code/pkg/metrics" "github.com/openshift-pipelines/pipelines-as-code/pkg/params" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/clients" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" @@ -29,6 +28,8 @@ import ( testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients" ghtesthelper "github.com/openshift-pipelines/pipelines-as-code/pkg/test/github" "github.com/openshift-pipelines/pipelines-as-code/pkg/test/logger" + "github.com/openshift-pipelines/pipelines-as-code/pkg/test/metrics" + "go.uber.org/zap" zapobserver "go.uber.org/zap/zaptest/observer" "gotest.tools/v3/assert" @@ -347,7 +348,7 @@ func TestGetTektonDir(t *testing.T) { } for _, tt := range testGetTektonDir { t.Run(tt.name, func(t *testing.T) { - resetMetrics() + metrics.ResetMetrics() observer, exporter := zapobserver.New(zap.InfoLevel) fakelogger := zap.New(observer).Sugar() ctx, _ := rtesting.SetupFakeContext(t) @@ -892,6 +893,7 @@ func TestGetFiles(t *testing.T) { wantDeletedFilesCount int wantModifiedFilesCount int wantRenamedFilesCount int + wantAPIRequestCount int64 }{ { name: "pull-request", @@ -920,6 +922,7 @@ func TestGetFiles(t *testing.T) { wantDeletedFilesCount: 1, wantModifiedFilesCount: 1, wantRenamedFilesCount: 1, + wantAPIRequestCount: 2, }, { name: "push", @@ -950,43 +953,15 @@ func TestGetFiles(t *testing.T) { wantDeletedFilesCount: 1, wantModifiedFilesCount: 1, wantRenamedFilesCount: 1, + wantAPIRequestCount: 1, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { fakeclient, mux, _, teardown := ghtesthelper.SetupGH() defer teardown() - prCommitFiles := []*github.CommitFile{ - { - Filename: ptr.String("modified.yaml"), - Status: ptr.String("modified"), - }, { - Filename: ptr.String("added.doc"), - Status: ptr.String("added"), - }, { - Filename: ptr.String("removed.yaml"), - Status: ptr.String("removed"), - }, { - Filename: ptr.String("renamed.doc"), - Status: ptr.String("renamed"), - }, - } + metrics.ResetMetrics() - pushCommitFiles := []*github.CommitFile{ - { - Filename: ptr.String("modified.yaml"), - Status: ptr.String("modified"), - }, { - Filename: ptr.String("added.doc"), - Status: ptr.String("added"), - }, { - Filename: ptr.String("removed.yaml"), - Status: ptr.String("removed"), - }, { - Filename: ptr.String("renamed.doc"), - Status: ptr.String("renamed"), - }, - } if tt.event.TriggerTarget == "pull_request" { mux.HandleFunc(fmt.Sprintf("/repos/%s/%s/pulls/%d/files", tt.event.Organization, tt.event.Repository, tt.event.PullRequestNumber), func(rw http.ResponseWriter, r *http.Request) { @@ -994,7 +969,7 @@ func TestGetFiles(t *testing.T) { rw.Header().Add("Link", fmt.Sprintf("; rel=\"next\"", tt.event.Organization, tt.event.Repository, tt.event.PullRequestNumber)) fmt.Fprint(rw, "[]") } else { - b, _ := json.Marshal(prCommitFiles) + b, _ := json.Marshal(tt.commitFiles) fmt.Fprint(rw, string(b)) } }) @@ -1003,17 +978,26 @@ func TestGetFiles(t *testing.T) { mux.HandleFunc(fmt.Sprintf("/repos/%s/%s/commits/%s", tt.event.Organization, tt.event.Repository, tt.event.SHA), func(rw http.ResponseWriter, _ *http.Request) { c := &github.RepositoryCommit{ - Files: pushCommitFiles, + Files: tt.commit.Files, } b, _ := json.Marshal(c) fmt.Fprint(rw, string(b)) }) } + metricsTags := map[string]string{"provider": "github", "event-type": string(tt.event.TriggerTarget)} + metricstest.CheckStatsNotReported(t, "pipelines_as_code_git_provider_api_request_count") + + log, _ := logger.GetLogger() ctx, _ := rtesting.SetupFakeContext(t) provider := &Provider{ ghClient: fakeclient, PaginedNumber: 1, + + // necessary for metrics + providerName: "github", + triggerEvent: string(tt.event.TriggerTarget), + Logger: log, } changedFiles, err := provider.GetFiles(ctx, tt.event) assert.NilError(t, err, nil) @@ -1032,6 +1016,11 @@ func TestGetFiles(t *testing.T) { assert.Equal(t, *tt.commit.Files[i].Filename, changedFiles.All[i]) } } + + // Check caching + metricstest.CheckCountData(t, "pipelines_as_code_git_provider_api_request_count", metricsTags, tt.wantAPIRequestCount) + _, _ = provider.GetFiles(ctx, tt.event) + metricstest.CheckCountData(t, "pipelines_as_code_git_provider_api_request_count", metricsTags, tt.wantAPIRequestCount) }) } } @@ -1382,18 +1371,6 @@ func TestIsHeadCommitOfBranch(t *testing.T) { } } -func resetMetrics() { - metricstest.Unregister( - "pipelines_as_code_pipelinerun_count", - "pipelines_as_code_pipelinerun_duration_seconds_sum", - "pipelines_as_code_running_pipelineruns_count", - "pipelines_as_code_git_provider_api_request_count", - ) - - // have to reset sync.Once to allow recreation of Recorder. - metrics.ResetRecorder() -} - func TestCreateComment(t *testing.T) { tests := []struct { name string diff --git a/pkg/provider/gitlab/gitlab.go b/pkg/provider/gitlab/gitlab.go index 7d68136b8b..8e402d7508 100644 --- a/pkg/provider/gitlab/gitlab.go +++ b/pkg/provider/gitlab/gitlab.go @@ -64,7 +64,8 @@ type Provider struct { triggerEvent string // memberCache caches membership/permission checks by user ID within the // current provider instance lifecycle to avoid repeated API calls. - memberCache map[int]bool + memberCache map[int]bool + changedFiles *changedfiles.ChangedFiles } func (v *Provider) Client() *gitlab.Client { @@ -500,25 +501,61 @@ func (v *Provider) GetFiles(_ context.Context, runevent *info.Event) (changedfil return changedfiles.ChangedFiles{}, fmt.Errorf("no gitlab client has been initialized, " + "exiting... (hint: did you forget setting a secret on your repo?)") } - if runevent.TriggerTarget == triggertype.PullRequest { - opt := &gitlab.ListMergeRequestDiffsOptions{ - ListOptions: gitlab.ListOptions{ - OrderBy: "id", - Pagination: "keyset", - PerPage: 20, - Sort: "asc", - }, - } - options := []gitlab.RequestOptionFunc{} - changedFiles := changedfiles.ChangedFiles{} + if v.changedFiles == nil { + switch runevent.TriggerTarget { + case triggertype.PullRequest: + opt := &gitlab.ListMergeRequestDiffsOptions{ + ListOptions: gitlab.ListOptions{ + OrderBy: "id", + Pagination: "keyset", + PerPage: 20, + Sort: "asc", + }, + } + options := []gitlab.RequestOptionFunc{} + changedFiles := changedfiles.ChangedFiles{} + + for { + mrchanges, resp, err := v.Client().MergeRequests.ListMergeRequestDiffs(v.targetProjectID, runevent.PullRequestNumber, opt, options...) + if err != nil { + // TODO: Should this return the files found so far? + return changedfiles.ChangedFiles{}, err + } - for { - mrchanges, resp, err := v.Client().MergeRequests.ListMergeRequestDiffs(v.targetProjectID, runevent.PullRequestNumber, opt, options...) + for _, change := range mrchanges { + changedFiles.All = append(changedFiles.All, change.NewPath) + if change.NewFile { + changedFiles.Added = append(changedFiles.Added, change.NewPath) + } + if change.DeletedFile { + changedFiles.Deleted = append(changedFiles.Deleted, change.NewPath) + } + if !change.RenamedFile && !change.DeletedFile && !change.NewFile { + changedFiles.Modified = append(changedFiles.Modified, change.NewPath) + } + if change.RenamedFile { + changedFiles.Renamed = append(changedFiles.Renamed, change.NewPath) + } + } + + // Exit the loop when we've seen all pages. + if resp.NextLink == "" { + break + } + + // Otherwise, set param to query the next page + options = []gitlab.RequestOptionFunc{ + gitlab.WithKeysetPaginationParameters(resp.NextLink), + } + } + v.changedFiles = &changedFiles + case triggertype.Push: + pushChanges, _, err := v.Client().Commits.GetCommitDiff(v.sourceProjectID, runevent.SHA, &gitlab.GetCommitDiffOptions{}) if err != nil { return changedfiles.ChangedFiles{}, err } - - for _, change := range mrchanges { + changedFiles := changedfiles.ChangedFiles{} + for _, change := range pushChanges { changedFiles.All = append(changedFiles.All, change.NewPath) if change.NewFile { changedFiles.Added = append(changedFiles.Added, change.NewPath) @@ -533,44 +570,12 @@ func (v *Provider) GetFiles(_ context.Context, runevent *info.Event) (changedfil changedFiles.Renamed = append(changedFiles.Renamed, change.NewPath) } } - - // Exit the loop when we've seen all pages. - if resp.NextLink == "" { - break - } - - // Otherwise, set param to query the next page - options = []gitlab.RequestOptionFunc{ - gitlab.WithKeysetPaginationParameters(resp.NextLink), - } - } - return changedFiles, nil - } - - if runevent.TriggerTarget == "push" { - pushChanges, _, err := v.Client().Commits.GetCommitDiff(v.sourceProjectID, runevent.SHA, &gitlab.GetCommitDiffOptions{}) - if err != nil { - return changedfiles.ChangedFiles{}, err - } - changedFiles := changedfiles.ChangedFiles{} - for _, change := range pushChanges { - changedFiles.All = append(changedFiles.All, change.NewPath) - if change.NewFile { - changedFiles.Added = append(changedFiles.Added, change.NewPath) - } - if change.DeletedFile { - changedFiles.Deleted = append(changedFiles.Deleted, change.NewPath) - } - if !change.RenamedFile && !change.DeletedFile && !change.NewFile { - changedFiles.Modified = append(changedFiles.Modified, change.NewPath) - } - if change.RenamedFile { - changedFiles.Renamed = append(changedFiles.Renamed, change.NewPath) - } + v.changedFiles = &changedFiles + default: + v.changedFiles = &changedfiles.ChangedFiles{} } - return changedFiles, nil } - return changedfiles.ChangedFiles{}, nil + return *v.changedFiles, nil } func (v *Provider) CreateToken(_ context.Context, _ []string, _ *info.Event) (string, error) { diff --git a/pkg/provider/gitlab/gitlab_test.go b/pkg/provider/gitlab/gitlab_test.go index a829554505..76e24b9dc6 100644 --- a/pkg/provider/gitlab/gitlab_test.go +++ b/pkg/provider/gitlab/gitlab_test.go @@ -21,11 +21,14 @@ import ( thelp "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/gitlab/test" testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients" "github.com/openshift-pipelines/pipelines-as-code/pkg/test/logger" + "github.com/openshift-pipelines/pipelines-as-code/pkg/test/metrics" gitlab "gitlab.com/gitlab-org/api/client-go" "go.uber.org/zap" zapobserver "go.uber.org/zap/zaptest/observer" "gotest.tools/v3/assert" + "knative.dev/pkg/metrics/metricstest" + _ "knative.dev/pkg/metrics/testing" rtesting "knative.dev/pkg/reconciler/testing" ) @@ -1152,60 +1155,30 @@ func TestGetFiles(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ctx, _ := rtesting.SetupFakeContext(t) + metrics.ResetMetrics() fakeclient, mux, teardown := thelp.Setup(t) defer teardown() - mergeFileChanges := []*gitlab.MergeRequestDiff{ - { - NewPath: "modified.yaml", - }, - { - NewPath: "added.doc", - NewFile: true, - }, - { - NewPath: "removed.yaml", - DeletedFile: true, - }, - { - NewPath: "renamed.doc", - RenamedFile: true, - }, - } if tt.event.TriggerTarget == "pull_request" { mux.HandleFunc(fmt.Sprintf("/projects/10/merge_requests/%d/diffs", tt.event.PullRequestNumber), func(rw http.ResponseWriter, _ *http.Request) { - jeez, err := json.Marshal(mergeFileChanges) + jeez, err := json.Marshal(tt.mrchanges) assert.NilError(t, err) _, _ = rw.Write(jeez) }) } - pushFileChanges := []*gitlab.Diff{ - { - NewPath: "modified.yaml", - }, - { - NewPath: "added.doc", - NewFile: true, - }, - { - NewPath: "removed.yaml", - DeletedFile: true, - }, - { - NewPath: "renamed.doc", - RenamedFile: true, - }, - } if tt.event.TriggerTarget == "push" { mux.HandleFunc(fmt.Sprintf("/projects/0/repository/commits/%s/diff", tt.event.SHA), func(rw http.ResponseWriter, _ *http.Request) { - jeez, err := json.Marshal(pushFileChanges) + jeez, err := json.Marshal(tt.pushChanges) assert.NilError(t, err) _, _ = rw.Write(jeez) }) } - providerInfo := &Provider{gitlabClient: fakeclient, sourceProjectID: tt.sourceProjectID, targetProjectID: tt.targetProjectID} + metricsTags := map[string]string{"provider": "api.gitlab.com", "event-type": string(tt.event.TriggerTarget)} + metricstest.CheckStatsNotReported(t, "pipelines_as_code_git_provider_api_request_count") + + providerInfo := &Provider{gitlabClient: fakeclient, sourceProjectID: tt.sourceProjectID, targetProjectID: tt.targetProjectID, triggerEvent: string(tt.event.TriggerTarget), apiURL: "api.gitlab.com"} changedFiles, err := providerInfo.GetFiles(ctx, tt.event) if tt.wantError != true { assert.NilError(t, err, nil) @@ -1225,6 +1198,17 @@ func TestGetFiles(t *testing.T) { assert.Equal(t, tt.pushChanges[i].NewPath, changedFiles.All[i]) } } + + // Check caching + metricstest.CheckCountData(t, "pipelines_as_code_git_provider_api_request_count", metricsTags, 1) + _, _ = providerInfo.GetFiles(ctx, tt.event) + if tt.wantError { + // No caching on error + metricstest.CheckCountData(t, "pipelines_as_code_git_provider_api_request_count", metricsTags, 2) + } else { + // Cache API results on success + metricstest.CheckCountData(t, "pipelines_as_code_git_provider_api_request_count", metricsTags, 1) + } }) } } diff --git a/pkg/provider/metrics/metrics_test.go b/pkg/provider/metrics/metrics_test.go index f9b2ff1770..d428b1b3e8 100644 --- a/pkg/provider/metrics/metrics_test.go +++ b/pkg/provider/metrics/metrics_test.go @@ -9,7 +9,7 @@ import ( "knative.dev/pkg/metrics/metricstest" "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" - "github.com/openshift-pipelines/pipelines-as-code/pkg/metrics" + metricsutils "github.com/openshift-pipelines/pipelines-as-code/pkg/test/metrics" _ "knative.dev/pkg/metrics/testing" ) @@ -47,15 +47,7 @@ func TestRecordAPIUsage(t *testing.T) { for _, testCase := range testCases { t.Run(testCase.desc, func(t *testing.T) { - defer func() { - metricstest.Unregister( - "pipelines_as_code_pipelinerun_count", - "pipelines_as_code_pipelinerun_duration_seconds_sum", - "pipelines_as_code_running_pipelineruns_count", - "pipelines_as_code_git_provider_api_request_count", - ) - metrics.ResetRecorder() - }() + defer metricsutils.ResetMetrics() observer, _ := zapobserver.New(zap.InfoLevel) fakelogger := zap.New(observer).Sugar() diff --git a/pkg/reconciler/emit_metrics_test.go b/pkg/reconciler/emit_metrics_test.go index d6c0aaa7fd..b27ad33491 100644 --- a/pkg/reconciler/emit_metrics_test.go +++ b/pkg/reconciler/emit_metrics_test.go @@ -6,6 +6,7 @@ import ( "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys" "github.com/openshift-pipelines/pipelines-as-code/pkg/metrics" + metricsutils "github.com/openshift-pipelines/pipelines-as-code/pkg/test/metrics" tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" "gotest.tools/v3/assert" corev1 "k8s.io/api/core/v1" @@ -86,7 +87,7 @@ func TestCountPipelineRun(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - unregisterMetrics() + metricsutils.ResetMetrics() m, err := metrics.NewRecorder() assert.NilError(t, err) r := &Reconciler{ @@ -224,7 +225,7 @@ func TestCalculatePipelineRunDuration(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - unregisterMetrics() + metricsutils.ResetMetrics() m, err := metrics.NewRecorder() assert.NilError(t, err) r := &Reconciler{ @@ -291,7 +292,7 @@ func TestCountRunningPRs(t *testing.T) { prl = append(prl, pr) } - unregisterMetrics() + metricsutils.ResetMetrics() m, err := metrics.NewRecorder() assert.NilError(t, err) r := &Reconciler{ @@ -306,15 +307,3 @@ func TestCountRunningPRs(t *testing.T) { } metricstest.CheckLastValueData(t, "pipelines_as_code_running_pipelineruns_count", tags, float64(numberOfRunningPRs)) } - -func unregisterMetrics() { - metricstest.Unregister( - "pipelines_as_code_pipelinerun_count", - "pipelines_as_code_pipelinerun_duration_seconds_sum", - "pipelines_as_code_running_pipelineruns_count", - "pipelines_as_code_git_provider_api_request_count", - ) - - // have to reset sync.Once to allow recreation of Recorder. - metrics.ResetRecorder() -} diff --git a/pkg/test/metrics/metrics.go b/pkg/test/metrics/metrics.go new file mode 100644 index 0000000000..11e94c770f --- /dev/null +++ b/pkg/test/metrics/metrics.go @@ -0,0 +1,18 @@ +package metrics + +import ( + "github.com/openshift-pipelines/pipelines-as-code/pkg/metrics" + "knative.dev/pkg/metrics/metricstest" +) + +func ResetMetrics() { + metricstest.Unregister( + "pipelines_as_code_pipelinerun_count", + "pipelines_as_code_pipelinerun_duration_seconds_sum", + "pipelines_as_code_running_pipelineruns_count", + "pipelines_as_code_git_provider_api_request_count", + ) + + // have to reset sync.Once to allow recreation of Recorder. + metrics.ResetRecorder() +} From 838d438ed9df1a805a14911693b1626d89cc18b0 Mon Sep 17 00:00:00 2001 From: Andrew Thorp Date: Wed, 5 Nov 2025 15:04:35 -0500 Subject: [PATCH 2/2] chore(lint): set golangci default switch case as exhaustive By default the exhaustive CI rule does not count a `default` switch case as exhaustive. Also fixes typo in misspell settings --- .golangci.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.golangci.yml b/.golangci.yml index cd3d897e08..38f1e214c1 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -58,7 +58,7 @@ linters: - zerologlint settings: misspell: - ignore-words: + ignore-rules: - cancelled extra-words: - typo: "canceled" @@ -72,6 +72,8 @@ linters: - fmt.Fprintln - (io.Closer).Close - updateConfigMap + exhaustive: + default-signifies-exhaustive: true gocritic: disabled-checks: - unlambda