Merge branch 'jswain_empty_repo_upload_tracking' into 'master'

Extract commit count logic to use elsewhere

See merge request gitlab-org/gitlab!59713

app/experiments/concerns/project_commit_count.rb

+# frozen_string_literal: true
+
+module ProjectCommitCount
+  include Gitlab::Git::WrapsGitalyErrors
+
+  def commit_count_for(project, default_count: 0, max_count: nil, **exception_details)
+    raw_repo = project.repository&.raw_repository
+    root_ref = raw_repo&.root_ref
+
+    return default_count unless root_ref
+
+    Gitlab::GitalyClient::CommitService.new(raw_repo).commit_count(root_ref, {
+      all: true, # include all branches
+      max_count: max_count # limit as an optimization
+    })
+  rescue StandardError => e
+    Gitlab::ErrorTracking.track_exception(e, exception_details)
+
+    default_count
+  end
+end

app/experiments/empty_repo_upload_experiment.rb

+# frozen_string_literal: true
+
+class EmptyRepoUploadExperiment < ApplicationExperiment # rubocop:disable Gitlab/NamespacedClass
+  include ProjectCommitCount
+
+  TRACKING_START_DATE = DateTime.parse('2021/4/20')
+  INITIAL_COMMIT_COUNT = 1
+
+  def track_initial_write
+    return unless should_track? # early return if we don't need to ask for commit counts
+    return unless context.project.created_at > TRACKING_START_DATE # early return for older projects
+    return unless commit_count == INITIAL_COMMIT_COUNT
+
+    track(:initial_write, project: context.project)
+  end
+
+  private
+
+  def commit_count
+    commit_count_for(context.project, max_count: INITIAL_COMMIT_COUNT, experiment: name)
+  end
+end

app/experiments/new_project_readme_experiment.rb

 # frozen_string_literal: true
 
 class NewProjectReadmeExperiment < ApplicationExperiment # rubocop:disable Gitlab/NamespacedClass
-  include Gitlab::Git::WrapsGitalyErrors
+  include ProjectCommitCount
 
   INITIAL_WRITE_LIMIT = 3
   EXPERIMENT_START_DATE = DateTime.parse('2021/1/20')
@@ -21,25 +21,18 @@ class NewProjectReadmeExperiment < ApplicationExperiment # rubocop:disable Gitla
   def track_initial_writes(project)
     return unless should_track? # early return if we don't need to ask for commit counts
     return unless project.created_at > EXPERIMENT_START_DATE # early return for older projects
-    return unless (commit_count = commit_count_for(project)) < INITIAL_WRITE_LIMIT
+    return unless (count = commit_count(project)) < INITIAL_WRITE_LIMIT
 
-    track(:write, property: project.created_at.to_s, value: commit_count)
+    track(:write, property: project.created_at.to_s, value: count)
   end
 
   private
 
-  def commit_count_for(project)
-    raw_repo = project.repository&.raw_repository
-    return INITIAL_WRITE_LIMIT unless raw_repo&.root_ref
-
-    begin
-      Gitlab::GitalyClient::CommitService.new(raw_repo).commit_count(raw_repo.root_ref, {
-        all: true, # include all branches
-        max_count: INITIAL_WRITE_LIMIT # limit as an optimization
-      })
-    rescue StandardError => e
-      Gitlab::ErrorTracking.track_exception(e, experiment: name)
-      INITIAL_WRITE_LIMIT
-    end
+  def commit_count(project)
+    commit_count_for(project,
+      default_count: INITIAL_WRITE_LIMIT,
+      max_count: INITIAL_WRITE_LIMIT,
+      experiment: name
+    )
   end
 end

app/workers/post_receive.rb

@@ -123,7 +123,7 @@ class PostReceive # rubocop:disable Scalability/IdempotentWorker
 
   def after_project_changes_hooks(project, user, refs, changes)
     experiment(:new_project_readme, actor: user).track_initial_writes(project)
-    experiment(:empty_repo_upload, project: project).track(:initial_write) if project.empty_repo?
+    experiment(:empty_repo_upload, project: project).track_initial_write
     repository_update_hook_data = Gitlab::DataBuilder::Repository.update(project, user, changes, refs)
     SystemHooksService.new.execute_hooks(repository_update_hook_data, :repository_update_hooks)
     Gitlab::UsageDataCounters::SourceCodeCounter.count(:pushes)

spec/experiments/concerns/project_commit_count_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ProjectCommitCount do
+  let(:klass) { Class.include(ProjectCommitCount) }
+  let(:instance) { klass.new }
+
+  describe '#commit_count_for' do
+    subject { instance.commit_count_for(project, default_count: 42, caller_info: :identifiable) }
+
+    let(:project) { create(:project, :repository) }
+
+    context 'when a root_ref exists' do
+      it 'returns commit count from GitlayClient' do
+        allow(Gitlab::GitalyClient).to receive(:call).and_call_original
+        allow(Gitlab::GitalyClient).to receive(:call).with(anything, :commit_service, :count_commits, anything, anything)
+          .and_return(double(count: 4))
+
+        expect(subject).to eq(4)
+      end
+    end
+
+    context 'when a root_ref does not exist' do
+      let(:project) { create(:project, :empty_repo) }
+
+      it 'returns the default_count' do
+        expect(subject).to eq(42)
+      end
+    end
+
+    it "handles exceptions by logging them with exception_details and returns the default_count" do
+      allow(Gitlab::GitalyClient).to receive(:call).and_call_original
+      allow(Gitlab::GitalyClient).to receive(:call).with(anything, :commit_service, :count_commits, anything, anything).and_raise(e = StandardError.new('_message_'))
+
+      expect(Gitlab::ErrorTracking).to receive(:track_exception).with(e, caller_info: :identifiable)
+
+      expect(subject).to eq(42)
+    end
+  end
+end

spec/experiments/empty_repo_upload_experiment_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe EmptyRepoUploadExperiment, :experiment do
+  subject { described_class.new(project: project) }
+
+  let(:project) { create(:project, :repository) }
+
+  describe '#track_initial_write' do
+    it "tracks an event for the first commit on a project" do
+      expect(subject).to receive(:commit_count_for).with(project, max_count: described_class::INITIAL_COMMIT_COUNT, experiment: 'empty_repo_upload').and_return(1)
+
+      expect(subject).to receive(:track).with(:initial_write, project: project).and_call_original
+
+      subject.track_initial_write
+    end
+
+    it "doesn't track an event for projects with a commit count more than 1" do
+      expect(subject).to receive(:commit_count_for).and_return(2)
+
+      expect(subject).not_to receive(:track)
+
+      subject.track_initial_write
+    end
+
+    it "doesn't track when we generally shouldn't" do
+      allow(subject).to receive(:should_track?).and_return(false)
+
+      expect(subject).not_to receive(:track)
+
+      subject.track_initial_write
+    end
+
+    it "doesn't track if the project is older" do
+      expect(project).to receive(:created_at).and_return(described_class::TRACKING_START_DATE - 1.minute)
+
+      expect(subject).not_to receive(:track)
+
+      subject.track_initial_write
+    end
+  end
+end

spec/experiments/new_project_readme_experiment_spec.rb

@@ -29,24 +29,15 @@ RSpec.describe NewProjectReadmeExperiment, :experiment do
   context "when tracking initial writes" do
     let!(:project) { create(:project, :repository) }
 
-    def stub_gitaly_count(count = 1)
-      allow(Gitlab::GitalyClient).to receive(:call).and_call_original
-      allow(Gitlab::GitalyClient).to receive(:call).with(anything, :commit_service, :count_commits, anything, anything)
-        .and_return(double(count: count))
-    end
-
-    before do
-      stub_gitaly_count
-    end
-
     it "tracks an event for the first commit on a project with a repository" do
+      expect(subject).to receive(:commit_count_for).with(project, default_count: described_class::INITIAL_WRITE_LIMIT, max_count: described_class::INITIAL_WRITE_LIMIT, experiment: 'new_project_readme').and_return(1)
       expect(subject).to receive(:track).with(:write, property: project.created_at.to_s, value: 1).and_call_original
 
       subject.track_initial_writes(project)
     end
 
     it "tracks an event for the second commit on a project with a repository" do
-      stub_gitaly_count(2)
+      allow(subject).to receive(:commit_count_for).and_return(2)
 
       expect(subject).to receive(:track).with(:write, property: project.created_at.to_s, value: 2).and_call_original
 
@@ -54,7 +45,7 @@ RSpec.describe NewProjectReadmeExperiment, :experiment do
     end
 
     it "doesn't track if the repository has more then 2 commits" do
-      stub_gitaly_count(3)
+      allow(subject).to receive(:commit_count_for).and_return(3)
 
       expect(subject).not_to receive(:track)
 
@@ -76,14 +67,5 @@ RSpec.describe NewProjectReadmeExperiment, :experiment do
 
       subject.track_initial_writes(project)
     end
-
-    it "handles exceptions by logging them" do
-      allow(Gitlab::GitalyClient).to receive(:call).with(anything, :commit_service, :count_commits, anything, anything)
-        .and_raise(e = StandardError.new('_message_'))
-
-      expect(Gitlab::ErrorTracking).to receive(:track_exception).with(e, experiment: 'new_project_readme')
-
-      subject.track_initial_writes(project)
-    end
   end
 end

spec/workers/post_receive_spec.rb

@@ -94,30 +94,12 @@ RSpec.describe PostReceive do
         perform
       end
 
-      it 'tracks an event for the empty_repo_upload experiment', :snowplow do
-        allow_next_instance_of(ApplicationExperiment) do |e|
-          allow(e).to receive(:should_track?).and_return(true)
-          allow(e).to receive(:track_initial_writes)
+      it 'tracks an event for the empty_repo_upload experiment', :experiment do
+        expect_next_instance_of(EmptyRepoUploadExperiment) do |e|
+          expect(e).to receive(:track_initial_write)
         end
 
         perform
-
-        expect_snowplow_event(category: 'empty_repo_upload', action: 'initial_write', context: [{
-          schema: 'iglu:com.gitlab/gitlab_experiment/jsonschema/1-0-0',
-          data: anything
-        }])
-      end
-
-      it 'does not track an event for the empty_repo_upload experiment when project is not empty', :snowplow do
-        allow(empty_project).to receive(:empty_repo?).and_return(false)
-        allow_next_instance_of(ApplicationExperiment) do |e|
-          allow(e).to receive(:should_track?).and_return(true)
-          allow(e).to receive(:track_initial_writes)
-        end
-
-        perform
-
-        expect_no_snowplow_event
       end
     end