Skip to content

Commit 658148f

Browse files
aThorp96aThorp96
committed
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.
1 parent a8212d3 commit 658148f

File tree

9 files changed

+265
-273
lines changed

9 files changed

+265
-273
lines changed

‎pkg/provider/bitbucketdatacenter/bitbucketdatacenter.go‎

Lines changed: 65 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ type Provider struct {
4242
projectKey string
4343
repo *v1alpha1.Repository
4444
triggerEvent string
45+
changedFiles *changedfiles.ChangedFiles
4546
}
4647

4748
func (v Provider) Client() *scm.Client {
@@ -371,79 +372,82 @@ func (v *Provider) GetConfig() *info.ProviderConfig {
371372
}
372373

373374
func (v *Provider) GetFiles(ctx context.Context, runevent *info.Event) (changedfiles.ChangedFiles, error) {
374-
OrgAndRepo := fmt.Sprintf("%s/%s", runevent.Organization, runevent.Repository)
375-
if runevent.TriggerTarget == triggertype.PullRequest {
376-
opts := &scm.ListOptions{Page: 1, Size: apiResponseLimit}
377-
changedFiles := changedfiles.ChangedFiles{}
378-
for {
379-
changes, _, err := v.Client().PullRequests.ListChanges(ctx, OrgAndRepo, runevent.PullRequestNumber, opts)
380-
if err != nil {
381-
return changedfiles.ChangedFiles{}, fmt.Errorf("failed to list changes for pull request: %w", err)
382-
}
383-
384-
for _, c := range changes {
385-
changedFiles.All = append(changedFiles.All, c.Path)
386-
if c.Added {
387-
changedFiles.Added = append(changedFiles.Added, c.Path)
388-
}
389-
if c.Modified {
390-
changedFiles.Modified = append(changedFiles.Modified, c.Path)
391-
}
392-
if c.Renamed {
393-
changedFiles.Renamed = append(changedFiles.Renamed, c.Path)
375+
if v.changedFiles == nil {
376+
OrgAndRepo := fmt.Sprintf("%s/%s", runevent.Organization, runevent.Repository)
377+
switch runevent.TriggerTarget {
378+
case triggertype.PullRequest:
379+
opts := &scm.ListOptions{Page: 1, Size: apiResponseLimit}
380+
changedFiles := changedfiles.ChangedFiles{}
381+
for {
382+
changes, _, err := v.Client().PullRequests.ListChanges(ctx, OrgAndRepo, runevent.PullRequestNumber, opts)
383+
if err != nil {
384+
return changedfiles.ChangedFiles{}, fmt.Errorf("failed to list changes for pull request: %w", err)
394385
}
395-
if c.Deleted {
396-
changedFiles.Deleted = append(changedFiles.Deleted, c.Path)
397-
}
398-
}
399386

400-
// In the Jenkins-x/go-scm package, the `isLastPage` field is not available, and the value of
401-
// `response.Page.Last` is set to `0`. Therefore, to determine if there are more items to fetch,
402-
// we can check if the length of the currently fetched items is less than the specified limit.
403-
// If the length is less than the limit, it indicates that there are no more items to retrieve.
404-
if len(changes) < apiResponseLimit {
405-
break
406-
}
387+
for _, c := range changes {
388+
changedFiles.All = append(changedFiles.All, c.Path)
389+
if c.Added {
390+
changedFiles.Added = append(changedFiles.Added, c.Path)
391+
}
392+
if c.Modified {
393+
changedFiles.Modified = append(changedFiles.Modified, c.Path)
394+
}
395+
if c.Renamed {
396+
changedFiles.Renamed = append(changedFiles.Renamed, c.Path)
397+
}
398+
if c.Deleted {
399+
changedFiles.Deleted = append(changedFiles.Deleted, c.Path)
400+
}
401+
}
407402

408-
opts.Page++
409-
}
410-
return changedFiles, nil
411-
}
403+
// In the Jenkins-x/go-scm package, the `isLastPage` field is not available, and the value of
404+
// `response.Page.Last` is set to `0`. Therefore, to determine if there are more items to fetch,
405+
// we can check if the length of the currently fetched items is less than the specified limit.
406+
// If the length is less than the limit, it indicates that there are no more items to retrieve.
407+
if len(changes) < apiResponseLimit {
408+
break
409+
}
412410

413-
if runevent.TriggerTarget == triggertype.Push {
414-
opts := &scm.ListOptions{Page: 1, Size: apiResponseLimit}
415-
changedFiles := changedfiles.ChangedFiles{}
416-
for {
417-
changes, _, err := v.Client().Git.ListChanges(ctx, OrgAndRepo, runevent.SHA, opts)
418-
if err != nil {
419-
return changedfiles.ChangedFiles{}, fmt.Errorf("failed to list changes for commit %s: %w", runevent.SHA, err)
411+
opts.Page++
420412
}
421-
422-
for _, c := range changes {
423-
changedFiles.All = append(changedFiles.All, c.Path)
424-
if c.Added {
425-
changedFiles.Added = append(changedFiles.Added, c.Path)
426-
}
427-
if c.Modified {
428-
changedFiles.Modified = append(changedFiles.Modified, c.Path)
413+
v.changedFiles = &changedFiles
414+
case triggertype.Push:
415+
opts := &scm.ListOptions{Page: 1, Size: apiResponseLimit}
416+
changedFiles := changedfiles.ChangedFiles{}
417+
for {
418+
changes, _, err := v.Client().Git.ListChanges(ctx, OrgAndRepo, runevent.SHA, opts)
419+
if err != nil {
420+
return changedfiles.ChangedFiles{}, fmt.Errorf("failed to list changes for commit %s: %w", runevent.SHA, err)
429421
}
430-
if c.Renamed {
431-
changedFiles.Renamed = append(changedFiles.Renamed, c.Path)
422+
423+
for _, c := range changes {
424+
changedFiles.All = append(changedFiles.All, c.Path)
425+
if c.Added {
426+
changedFiles.Added = append(changedFiles.Added, c.Path)
427+
}
428+
if c.Modified {
429+
changedFiles.Modified = append(changedFiles.Modified, c.Path)
430+
}
431+
if c.Renamed {
432+
changedFiles.Renamed = append(changedFiles.Renamed, c.Path)
433+
}
434+
if c.Deleted {
435+
changedFiles.Deleted = append(changedFiles.Deleted, c.Path)
436+
}
432437
}
433-
if c.Deleted {
434-
changedFiles.Deleted = append(changedFiles.Deleted, c.Path)
438+
439+
if len(changes) < apiResponseLimit {
440+
break
435441
}
436-
}
437442

438-
if len(changes) < apiResponseLimit {
439-
break
443+
opts.Page++
440444
}
441-
442-
opts.Page++
445+
v.changedFiles = &changedFiles
446+
default:
447+
v.changedFiles = &changedfiles.ChangedFiles{}
443448
}
444-
return changedFiles, nil
445449
}
446-
return changedfiles.ChangedFiles{}, nil
450+
return *v.changedFiles, nil
447451
}
448452

449453
func (v *Provider) CreateToken(_ context.Context, _ []string, _ *info.Event) (string, error) {

‎pkg/provider/bitbucketdatacenter/bitbucketdatacenter_test.go‎

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,14 @@ import (
2222
"github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype"
2323
"github.com/openshift-pipelines/pipelines-as-code/pkg/provider"
2424
bbtest "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/bitbucketdatacenter/test"
25+
"github.com/openshift-pipelines/pipelines-as-code/pkg/test/metrics"
2526

2627
"github.com/jenkins-x/go-scm/scm"
2728
"go.uber.org/zap"
2829
zapobserver "go.uber.org/zap/zaptest/observer"
2930
"gotest.tools/v3/assert"
31+
"knative.dev/pkg/metrics/metricstest"
32+
_ "knative.dev/pkg/metrics/testing"
3033
rtesting "knative.dev/pkg/reconciler/testing"
3134
)
3235

@@ -796,6 +799,7 @@ func TestGetFiles(t *testing.T) {
796799
ctx, _ := rtesting.SetupFakeContext(t)
797800
client, mux, tearDown, tURL := bbtest.SetupBBDataCenterClient()
798801
defer tearDown()
802+
metrics.ResetMetrics()
799803

800804
stats := &bbtest.DiffStats{
801805
Values: tt.changeFiles,
@@ -821,13 +825,17 @@ func TestGetFiles(t *testing.T) {
821825
}
822826
})
823827
}
824-
v := &Provider{client: client, baseURL: tURL}
828+
829+
metricsTags := map[string]string{"provider": "bitbucket-datacenter", "event-type": string(tt.event.TriggerTarget)}
830+
metricstest.CheckStatsNotReported(t, "pipelines_as_code_git_provider_api_request_count")
831+
832+
v := &Provider{client: client, baseURL: tURL, triggerEvent: string(tt.event.TriggerTarget)}
825833
changedFiles, err := v.GetFiles(ctx, tt.event)
826834
if tt.wantError {
827835
assert.Equal(t, err.Error(), tt.errMsg)
828-
return
836+
} else {
837+
assert.NilError(t, err, nil)
829838
}
830-
assert.NilError(t, err, nil)
831839
assert.Equal(t, tt.wantAddedFilesCount, len(changedFiles.Added))
832840
assert.Equal(t, tt.wantDeletedFilesCount, len(changedFiles.Deleted))
833841
assert.Equal(t, tt.wantModifiedFilesCount, len(changedFiles.Modified))
@@ -844,6 +852,17 @@ func TestGetFiles(t *testing.T) {
844852
assert.Equal(t, tt.changeFiles[i].Path.ToString, changedFiles.All[i])
845853
}
846854
}
855+
856+
// Check caching
857+
metricstest.CheckCountData(t, "pipelines_as_code_git_provider_api_request_count", metricsTags, 1)
858+
_, _ = v.GetFiles(ctx, tt.event)
859+
if tt.wantError {
860+
// No caching on error
861+
metricstest.CheckCountData(t, "pipelines_as_code_git_provider_api_request_count", metricsTags, 2)
862+
} else {
863+
// Cache API results on success
864+
metricstest.CheckCountData(t, "pipelines_as_code_git_provider_api_request_count", metricsTags, 1)
865+
}
847866
})
848867
}
849868
}

‎pkg/provider/github/github.go‎

Lines changed: 53 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ type Provider struct {
5656
userType string // The type of user i.e bot or not
5757
skippedRun
5858
triggerEvent string
59+
changedFiles *changedfiles.ChangedFiles
5960
}
6061

6162
type skippedRun struct {
@@ -517,67 +518,70 @@ func (v *Provider) getPullRequest(ctx context.Context, runevent *info.Event) (*i
517518
return runevent, nil
518519
}
519520

520-
// GetFiles get a files from pull request.
521+
// GetFiles gets the files changed by a given event.
521522
func (v *Provider) GetFiles(ctx context.Context, runevent *info.Event) (changedfiles.ChangedFiles, error) {
522-
if runevent.TriggerTarget == triggertype.PullRequest {
523-
opt := &github.ListOptions{PerPage: v.PaginedNumber}
524-
changedFiles := changedfiles.ChangedFiles{}
525-
for {
526-
repoCommit, resp, err := wrapAPI(v, "list_pull_request_files", func() ([]*github.CommitFile, *github.Response, error) {
527-
return v.Client().PullRequests.ListFiles(ctx, runevent.Organization, runevent.Repository, runevent.PullRequestNumber, opt)
523+
if v.changedFiles == nil {
524+
switch runevent.TriggerTarget {
525+
case triggertype.PullRequest:
526+
opt := &github.ListOptions{PerPage: v.PaginedNumber}
527+
changedFiles := changedfiles.ChangedFiles{}
528+
for {
529+
repoCommit, resp, err := wrapAPI(v, "list_pull_request_files", func() ([]*github.CommitFile, *github.Response, error) {
530+
return v.Client().PullRequests.ListFiles(ctx, runevent.Organization, runevent.Repository, runevent.PullRequestNumber, opt)
531+
})
532+
if err != nil {
533+
return changedfiles.ChangedFiles{}, err
534+
}
535+
for j := range repoCommit {
536+
changedFiles.All = append(changedFiles.All, *repoCommit[j].Filename)
537+
if *repoCommit[j].Status == "added" {
538+
changedFiles.Added = append(changedFiles.Added, *repoCommit[j].Filename)
539+
}
540+
if *repoCommit[j].Status == "removed" {
541+
changedFiles.Deleted = append(changedFiles.Deleted, *repoCommit[j].Filename)
542+
}
543+
if *repoCommit[j].Status == "modified" {
544+
changedFiles.Modified = append(changedFiles.Modified, *repoCommit[j].Filename)
545+
}
546+
if *repoCommit[j].Status == "renamed" {
547+
changedFiles.Renamed = append(changedFiles.Renamed, *repoCommit[j].Filename)
548+
}
549+
}
550+
if resp.NextPage == 0 {
551+
break
552+
}
553+
opt.Page = resp.NextPage
554+
}
555+
v.changedFiles = &changedFiles
556+
case triggertype.Push:
557+
changedFiles := changedfiles.ChangedFiles{}
558+
rC, _, err := wrapAPI(v, "get_commit_files", func() (*github.RepositoryCommit, *github.Response, error) {
559+
return v.Client().Repositories.GetCommit(ctx, runevent.Organization, runevent.Repository, runevent.SHA, &github.ListOptions{})
528560
})
529561
if err != nil {
530562
return changedfiles.ChangedFiles{}, err
531563
}
532-
for j := range repoCommit {
533-
changedFiles.All = append(changedFiles.All, *repoCommit[j].Filename)
534-
if *repoCommit[j].Status == "added" {
535-
changedFiles.Added = append(changedFiles.Added, *repoCommit[j].Filename)
564+
for i := range rC.Files {
565+
changedFiles.All = append(changedFiles.All, *rC.Files[i].Filename)
566+
if *rC.Files[i].Status == "added" {
567+
changedFiles.Added = append(changedFiles.Added, *rC.Files[i].Filename)
536568
}
537-
if *repoCommit[j].Status == "removed" {
538-
changedFiles.Deleted = append(changedFiles.Deleted, *repoCommit[j].Filename)
569+
if *rC.Files[i].Status == "removed" {
570+
changedFiles.Deleted = append(changedFiles.Deleted, *rC.Files[i].Filename)
539571
}
540-
if *repoCommit[j].Status == "modified" {
541-
changedFiles.Modified = append(changedFiles.Modified, *repoCommit[j].Filename)
572+
if *rC.Files[i].Status == "modified" {
573+
changedFiles.Modified = append(changedFiles.Modified, *rC.Files[i].Filename)
542574
}
543-
if *repoCommit[j].Status == "renamed" {
544-
changedFiles.Renamed = append(changedFiles.Renamed, *repoCommit[j].Filename)
575+
if *rC.Files[i].Status == "renamed" {
576+
changedFiles.Renamed = append(changedFiles.Renamed, *rC.Files[i].Filename)
545577
}
546578
}
547-
if resp.NextPage == 0 {
548-
break
549-
}
550-
opt.Page = resp.NextPage
551-
}
552-
return changedFiles, nil
553-
}
554-
555-
if runevent.TriggerTarget == "push" {
556-
changedFiles := changedfiles.ChangedFiles{}
557-
rC, _, err := wrapAPI(v, "get_commit_files", func() (*github.RepositoryCommit, *github.Response, error) {
558-
return v.Client().Repositories.GetCommit(ctx, runevent.Organization, runevent.Repository, runevent.SHA, &github.ListOptions{})
559-
})
560-
if err != nil {
561-
return changedfiles.ChangedFiles{}, err
562-
}
563-
for i := range rC.Files {
564-
changedFiles.All = append(changedFiles.All, *rC.Files[i].Filename)
565-
if *rC.Files[i].Status == "added" {
566-
changedFiles.Added = append(changedFiles.Added, *rC.Files[i].Filename)
567-
}
568-
if *rC.Files[i].Status == "removed" {
569-
changedFiles.Deleted = append(changedFiles.Deleted, *rC.Files[i].Filename)
570-
}
571-
if *rC.Files[i].Status == "modified" {
572-
changedFiles.Modified = append(changedFiles.Modified, *rC.Files[i].Filename)
573-
}
574-
if *rC.Files[i].Status == "renamed" {
575-
changedFiles.Renamed = append(changedFiles.Renamed, *rC.Files[i].Filename)
576-
}
579+
v.changedFiles = &changedFiles
580+
default:
581+
v.changedFiles = &changedfiles.ChangedFiles{}
577582
}
578-
return changedFiles, nil
579583
}
580-
return changedfiles.ChangedFiles{}, nil
584+
return *v.changedFiles, nil
581585
}
582586

583587
// getObject Get an object from a repository.

0 commit comments

Comments
 (0)