feat(perf): Cache changed files during event processing

.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

pkg/provider/bitbucketdatacenter/bitbucketdatacenter.go

@@ -42,6 +42,7 @@ type Provider struct {
 	projectKey                string
 	repo                      *v1alpha1.Repository
 	triggerEvent              string
+	cachedChangedFiles        *changedfiles.ChangedFiles
 }
 
 func (v Provider) Client() *scm.Client {
@@ -370,13 +371,28 @@ func (v *Provider) GetConfig() *info.ProviderConfig {
 	}
 }
 
+// GetFiles gets and caches the list of files changed by a given event.
 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 {
+	if v.cachedChangedFiles == nil {
+		changes, err := v.fetchChangedFiles(ctx, runevent)
+		if err != nil {
+			return changedfiles.ChangedFiles{}, err
+		}
+		v.cachedChangedFiles = &changes
+	}
+	return *v.cachedChangedFiles, nil
+}
+
+func (v *Provider) fetchChangedFiles(ctx context.Context, runevent *info.Event) (changedfiles.ChangedFiles, error) {
+	changedFiles := changedfiles.ChangedFiles{}
+
+	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)
+			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)
 			}
@@ -407,14 +423,10 @@ func (v *Provider) GetFiles(ctx context.Context, runevent *info.Event) (changedf
 
 			opts.Page++
 		}
-		return changedFiles, nil
-	}
-
-	if runevent.TriggerTarget == triggertype.Push {
+	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)
+			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)
 			}
@@ -441,9 +453,10 @@ func (v *Provider) GetFiles(ctx context.Context, runevent *info.Event) (changedf
 
 			opts.Page++
 		}
-		return changedFiles, nil
+	default:
+		// No action necessary
 	}
-	return changedfiles.ChangedFiles{}, nil
+	return changedFiles, nil
 }
 
 func (v *Provider) CreateToken(_ context.Context, _ []string, _ *info.Event) (string, error) {

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)
+			}
 		})
 	}
 }

pkg/provider/github/github.go

@@ -55,7 +55,8 @@ type Provider struct {
 	PaginedNumber int
 	userType      string // The type of user i.e bot or not
 	skippedRun
-	triggerEvent string
+	triggerEvent       string
+	cachedChangedFiles *changedfiles.ChangedFiles
 }
 
 type skippedRun struct {
@@ -517,11 +518,24 @@ func (v *Provider) getPullRequest(ctx context.Context, runevent *info.Event) (*i
 	return runevent, nil
 }
 
-// GetFiles get a files from pull request.
+// GetFiles gets and caches the list of files changed by a given event.
 func (v *Provider) GetFiles(ctx context.Context, runevent *info.Event) (changedfiles.ChangedFiles, error) {
-	if runevent.TriggerTarget == triggertype.PullRequest {
+	if v.cachedChangedFiles == nil {
+		changes, err := v.fetchChangedFiles(ctx, runevent)
+		if err != nil {
+			return changedfiles.ChangedFiles{}, err
+		}
+		v.cachedChangedFiles = &changes
+	}
+	return *v.cachedChangedFiles, nil
+}
+
+func (v *Provider) fetchChangedFiles(ctx context.Context, runevent *info.Event) (changedfiles.ChangedFiles, error) {
+	changedFiles := changedfiles.ChangedFiles{}
+
+	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)
@@ -549,11 +563,7 @@ func (v *Provider) GetFiles(ctx context.Context, runevent *info.Event) (changedf
 			}
 			opt.Page = resp.NextPage
 		}
-		return changedFiles, nil
-	}
-
-	if runevent.TriggerTarget == "push" {
-		changedFiles := changedfiles.ChangedFiles{}
+	case triggertype.Push:
 		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{})
 		})
@@ -575,9 +585,10 @@ func (v *Provider) GetFiles(ctx context.Context, runevent *info.Event) (changedf
 				changedFiles.Renamed = append(changedFiles.Renamed, *rC.Files[i].Filename)
 			}
 		}
-		return changedFiles, nil
+	default:
+		// No action necessary
 	}
-	return changedfiles.ChangedFiles{}, nil
+	return changedFiles, nil
 }
 
 // getObject Get an object from a repository.

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,51 +953,23 @@ 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) {
 					if r.URL.Query().Get("page") == "" || r.URL.Query().Get("page") == "1" {
 						rw.Header().Add("Link", fmt.Sprintf("<https://api.github.com/repos/%s/%s/pulls/%d/files?page=2>; 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