repository: Add log only for new repository calculation methods

The RepositorySize is a high risk API, since an inaccurate number
(especially if it's much higher) can cause projects to be locked due to
usage quotas. In order to safely roll out a new calculation, add a log
only mode such that a feature flag has to be enabled in order to send
the value back to the Rails client. Without this feature flag, we merely
log the new calculation and continue to send the value calculated with
du.

3 files changed, 82 insertions, 25 deletions

diff --git a/internal/gitaly/service/repository/size.go b/internal/gitaly/service/repository/size.go
index 6b3002c15..9f19b5372 100644
--- a/internal/gitaly/service/repository/size.go
+++ b/internal/gitaly/service/repository/size.go
@@ -8,6 +8,7 @@ import (
 	"strconv"
 
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
+	"github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/command"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git/catfile"
@@ -23,28 +24,32 @@ import (
 
 func (s *server) RepositorySize(ctx context.Context, in *gitalypb.RepositorySizeRequest) (*gitalypb.RepositorySizeResponse, error) {
 	repo := s.localrepo(in.GetRepository())
-	var size int64
+	var newSize int64
 	var err error
+	var logger *logrus.Entry
 
 	path, err := repo.Path()
 	if err != nil {
 		return nil, err
 	}
 
-	duSize := getPathSize(ctx, path)
+	size := getPathSize(ctx, path)
+
+	logger = ctxlogrus.Extract(ctx).WithField("repo_size_du", size)
 
 	if featureflag.RevlistForRepoSize.IsEnabled(ctx) {
-		size, err = calculateSizeWithRevlist(ctx, repo)
+		newSize, err = calculateSizeWithRevlist(ctx, repo)
 		if err != nil {
 			return nil, fmt.Errorf("calculating repository size with git-rev-list: %w,", err)
 		}
 
-		ctxlogrus.Extract(ctx).
-			WithField("repo_size_revlist", size).
-			WithField("repo_size_du", duSize).
-			Info("repository size calculated")
+		logger.WithField("repo_size_revlist", newSize).Info("repository size calculated")
+
+		if featureflag.UseNewRepoSize.IsEnabled(ctx) {
+			size = newSize
+		}
 	} else if featureflag.CatfileRepoSize.IsEnabled(ctx) {
-		size, err = calculateSizeWithCatfile(
+		newSize, err = calculateSizeWithCatfile(
 			ctx,
 			repo,
 			s.locator,
@@ -57,12 +62,11 @@ func (s *server) RepositorySize(ctx context.Context, in *gitalypb.RepositorySize
 			return nil, fmt.Errorf("calculating repository size with git-cat-file: %w", err)
 		}
 
-		ctxlogrus.Extract(ctx).
-			WithField("repo_size_catfile", size).
-			WithField("repo_size_du", duSize).
-			Info("repository size calculated")
-	} else {
-		size = duSize
+		logger.WithField("repo_size_catfile", newSize).Info("repository size calculated")
+
+		if featureflag.UseNewRepoSize.IsEnabled(ctx) {
+			size = newSize
+		}
 	}
 
 	return &gitalypb.RepositorySizeResponse{Size: size}, nil
diff --git a/internal/gitaly/service/repository/size_test.go b/internal/gitaly/service/repository/size_test.go
index 98f129bea..521ce9de0 100644
--- a/internal/gitaly/service/repository/size_test.go
+++ b/internal/gitaly/service/repository/size_test.go
@@ -7,6 +7,8 @@ import (
 	"context"
 	"testing"
 
+	"github.com/sirupsen/logrus"
+	"github.com/sirupsen/logrus/hooks/test"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/git"
@@ -18,6 +20,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testserver"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/protobuf/proto"
@@ -33,6 +36,7 @@ func TestRepositorySize_SuccessfulRequest(t *testing.T) {
 	featureSet := testhelper.NewFeatureSets(
 		featureflag.RevlistForRepoSize,
 		featureflag.CatfileRepoSize,
+		featureflag.UseNewRepoSize,
 	)
 
 	featureSet.Run(t, testSuccessfulRepositorySizeRequest)
@@ -97,8 +101,9 @@ func testSuccessfulRepositorySizeRequestPoolMember(t *testing.T, ctx context.Con
 	response, err = repoClient.RepositorySize(ctx, sizeRequest)
 	require.NoError(t, err)
 
-	if featureflag.RevlistForRepoSize.IsEnabled(ctx) ||
-		featureflag.CatfileRepoSize.IsEnabled(ctx) {
+	if featureflag.UseNewRepoSize.IsEnabled(ctx) &&
+		(featureflag.RevlistForRepoSize.IsEnabled(ctx) ||
+			featureflag.CatfileRepoSize.IsEnabled(ctx)) {
 		assert.Equal(t, int64(0), response.GetSize())
 	} else {
 		assert.Less(t, response.GetSize(), sizeBeforePool)
@@ -106,7 +111,8 @@ func testSuccessfulRepositorySizeRequestPoolMember(t *testing.T, ctx context.Con
 }
 
 func testSuccessfulRepositorySizeRequest(t *testing.T, ctx context.Context) {
-	cfg, repo, repoPath, client := setupRepositoryService(ctx, t)
+	logger, hook := test.NewNullLogger()
+	cfg, repo, repoPath, client := setupRepositoryService(ctx, t, testserver.WithLogger(logger))
 
 	request := &gitalypb.RepositorySizeRequest{Repository: repo}
 	response, err := client.RepositorySize(ctx, request)
@@ -136,14 +142,51 @@ func testSuccessfulRepositorySizeRequest(t *testing.T, ctx context.Context) {
 	responseAfterRefs, err := client.RepositorySize(ctx, request)
 	require.NoError(t, err)
 
-	if featureflag.RevlistForRepoSize.IsEnabled(ctx) {
-		assert.Equal(
-			t,
-			response.Size,
-			responseAfterRefs.Size,
-			"excluded refs do not contribute to the repository size",
-		)
-	} else {
+	// We expect two log entries with "repository size calculated"
+	// since we called RepositorySize twice, but we are really interested
+	// in the results of the second log.
+	var entries []*logrus.Entry
+	switch {
+	case featureflag.RevlistForRepoSize.IsEnabled(ctx):
+		for _, entry := range hook.AllEntries() {
+			_, ok := entry.Data["repo_size_revlist"]
+			if ok {
+				entries = append(entries, entry)
+			}
+		}
+
+		require.Len(t, entries, 2)
+		sizeInLog, ok := entries[1].Data["repo_size_revlist"]
+		require.True(t, ok)
+		require.Equal(t, "repository size calculated", entries[1].Message)
+
+		if featureflag.UseNewRepoSize.IsEnabled(ctx) {
+			assert.Equal(
+				t,
+				response.Size,
+				responseAfterRefs.Size,
+				"excluded refs do not contribute to the repository size",
+			)
+			require.Equal(t, responseAfterRefs.Size, sizeInLog)
+		}
+	case featureflag.CatfileRepoSize.IsEnabled(ctx):
+		for _, entry := range hook.AllEntries() {
+			_, ok := entry.Data["repo_size_catfile"]
+			if ok {
+				entries = append(entries, entry)
+			}
+		}
+
+		require.Len(t, entries, 2)
+		sizeInLog, ok := entries[1].Data["repo_size_catfile"]
+		require.True(t, ok)
+		require.Equal(t, "repository size calculated", entries[1].Message)
+
+		if featureflag.UseNewRepoSize.IsEnabled(ctx) {
+			require.Less(t, response.Size, sizeInLog)
+			assert.Less(t, response.Size, responseAfterRefs.Size)
+		}
+	default:
 		assert.Less(t, response.Size, responseAfterRefs.Size)
 	}
 }
diff --git a/internal/metadata/featureflag/ff_use_new_calculation_for_repo_size.go b/internal/metadata/featureflag/ff_use_new_calculation_for_repo_size.go
new file mode 100644
index 000000000..960e213c1
--- /dev/null
+++ b/internal/metadata/featureflag/ff_use_new_calculation_for_repo_size.go
@@ -0,0 +1,10 @@
+package featureflag
+
+// UseNewRepoSize will send the new repository size values in RepositorySize to
+// the client.
+var UseNewRepoSize = NewFeatureFlag(
+	"use_new_repository_size",
+	"v15.4.0",
+	"https://gitlab.com/gitlab-org/gitaly/-/issues/4448",
+	false,
+)