Merge branch '216155-retry-sending-the-usage-ping' into 'master'

Retry sending Usage Ping in case of network errors

See merge request gitlab-org/gitlab!35083

app/services/submit_usage_ping_service.rb

@@ -13,24 +13,25 @@ class SubmitUsagePingService
                percentage_projects_prometheus_active leader_service_desk_issues instance_service_desk_issues
                percentage_service_desk_issues].freeze
 
+  SubmissionError = Class.new(StandardError)
+
   def execute
-    return false unless Gitlab::CurrentSettings.usage_ping_enabled?
-    return false if User.single_user&.requires_usage_stats_consent?
+    return unless Gitlab::CurrentSettings.usage_ping_enabled?
+    return if User.single_user&.requires_usage_stats_consent?
+
+    payload = Gitlab::UsageData.to_json(force_refresh: true)
+    raise SubmissionError.new('Usage data is blank') if payload.blank?
 
     response = Gitlab::HTTP.post(
       URL,
-      body: Gitlab::UsageData.to_json(force_refresh: true),
+      body: payload,
       allow_local_requests: true,
       headers: { 'Content-type' => 'application/json' }
     )
 
-    store_metrics(response)
+    raise SubmissionError.new("Unsuccessful response code: #{response.code}") unless response.success?
 
-    true
-  rescue Gitlab::HTTP::Error => e
-    Gitlab::AppLogger.info("Unable to contact GitLab, Inc.: #{e}")
-
-    false
+    store_metrics(response)
   end
 
   private

app/workers/gitlab_usage_ping_worker.rb

 # frozen_string_literal: true
 
 class GitlabUsagePingWorker # rubocop:disable Scalability/IdempotentWorker
+  LEASE_KEY = 'gitlab_usage_ping_worker:ping'
   LEASE_TIMEOUT = 86400
 
   include ApplicationWorker
-  # rubocop:disable Scalability/CronWorkerContext
-  # This worker does not perform work scoped to a context
-  include CronjobQueue
-  # rubocop:enable Scalability/CronWorkerContext
+  include CronjobQueue # rubocop:disable Scalability/CronWorkerContext
+  include Gitlab::ExclusiveLeaseHelpers
 
   feature_category :collection
-
-  # Retry for up to approximately three hours then give up.
-  sidekiq_options retry: 10, dead: false
+  sidekiq_options retry: 3, dead: false
+  sidekiq_retry_in { |count| (count + 1) * 8.hours.to_i }
 
   def perform
     # Multiple Sidekiq workers could run this. We should only do this at most once a day.
-    return unless try_obtain_lease
-
-    # Splay the request over a minute to avoid thundering herd problems.
-    sleep(rand(0.0..60.0).round(3))
-
-    SubmitUsagePingService.new.execute
-  end
-
-  private
+    in_lock(LEASE_KEY, ttl: LEASE_TIMEOUT) do
+      # Splay the request over a minute to avoid thundering herd problems.
+      sleep(rand(0.0..60.0).round(3))
 
-  def try_obtain_lease
-    Gitlab::ExclusiveLease.new('gitlab_usage_ping_worker:ping', timeout: LEASE_TIMEOUT).try_obtain
+      SubmitUsagePingService.new.execute
+    end
   end
 end

spec/services/submit_usage_ping_service_spec.rb

@@ -49,17 +49,22 @@ RSpec.describe SubmitUsagePingService do
   let(:with_conv_index_params) { { conv_index: score_params[:score] } }
   let(:without_dev_ops_score_params) { { dev_ops_score: {} } }
 
-  context 'when usage ping is disabled' do
-    before do
-      stub_application_setting(usage_ping_enabled: false)
-    end
+  shared_examples 'does not run' do
+    it do
+      expect(Gitlab::HTTP).not_to receive(:post)
+      expect(Gitlab::UsageData).not_to receive(:to_json)
 
-    it 'does not run' do
-      expect(HTTParty).not_to receive(:post)
+      subject.execute
+    end
+  end
 
-      result = subject.execute
+  shared_examples 'does not send a blank usage ping payload' do
+    it do
+      expect(Gitlab::HTTP).not_to receive(:post)
 
-      expect(result).to eq false
+      expect { subject.execute }.to raise_error(described_class::SubmissionError) do |error|
+        expect(error.message).to include('Usage data is blank')
+      end
     end
   end
 
@@ -75,33 +80,47 @@ RSpec.describe SubmitUsagePingService do
     end
   end
 
+  context 'when usage ping is disabled' do
+    before do
+      stub_application_setting(usage_ping_enabled: false)
+    end
+
+    it_behaves_like 'does not run'
+  end
+
   context 'when usage ping is enabled' do
     before do
       stub_usage_data_connections
       stub_application_setting(usage_ping_enabled: true)
     end
 
+    context 'and user requires usage stats consent' do
+      before do
+        allow(User).to receive(:single_user).and_return(double(:user, requires_usage_stats_consent?: true))
+      end
+
+      it_behaves_like 'does not run'
+    end
+
     it 'sends a POST request' do
-      response = stub_response(without_dev_ops_score_params)
+      response = stub_response(body: without_dev_ops_score_params)
 
       subject.execute
 
       expect(response).to have_been_requested
     end
 
-    it 'refreshes usage data statistics before submitting' do
-      stub_response(without_dev_ops_score_params)
+    it 'forces a refresh of usage data statistics before submitting' do
+      stub_response(body: without_dev_ops_score_params)
 
-      expect(Gitlab::UsageData).to receive(:to_json)
-        .with(force_refresh: true)
-        .and_call_original
+      expect(Gitlab::UsageData).to receive(:to_json).with(force_refresh: true).and_call_original
 
       subject.execute
     end
 
     context 'when conv_index data is passed' do
       before do
-        stub_response(with_conv_index_params)
+        stub_response(body: with_conv_index_params)
       end
 
       it_behaves_like 'saves DevOps score data from the response'
@@ -109,18 +128,47 @@ RSpec.describe SubmitUsagePingService do
 
     context 'when DevOps score data is passed' do
       before do
-        stub_response(with_dev_ops_score_params)
+        stub_response(body: with_dev_ops_score_params)
       end
 
       it_behaves_like 'saves DevOps score data from the response'
     end
+
+    context 'and usage ping response has unsuccessful status' do
+      before do
+        stub_response(body: nil, status: 504)
+      end
+
+      it 'raises an exception' do
+        expect { subject.execute }.to raise_error(described_class::SubmissionError) do |error|
+          expect(error.message).to include('Unsuccessful response code: 504')
+        end
+      end
+    end
+
+    context 'and usage data is empty string' do
+      before do
+        allow(Gitlab::UsageData).to receive(:to_json).and_return("")
+      end
+
+      it_behaves_like 'does not send a blank usage ping payload'
+    end
+
+    context 'and usage data is nil' do
+      before do
+        allow(Gitlab::UsageData).to receive(:to_json).and_return(nil)
+      end
+
+      it_behaves_like 'does not send a blank usage ping payload'
+    end
   end
 
-  def stub_response(body)
+  def stub_response(body:, status: 201)
     stub_full_request('https://version.gitlab.com/usage_data', method: :post)
       .to_return(
         headers: { 'Content-Type' => 'application/json' },
-        body: body.to_json
+        body: body.to_json,
+        status: status
       )
   end
 end

spec/workers/gitlab_usage_ping_worker_spec.rb

@@ -2,16 +2,42 @@
 
 require 'spec_helper'
 
-RSpec.describe GitlabUsagePingWorker do
-  subject { described_class.new }
+RSpec.describe GitlabUsagePingWorker, :clean_gitlab_redis_shared_state do
+  before do
+    allow_next_instance_of(SubmitUsagePingService) { |service| allow(service).to receive(:execute) }
+    allow(subject).to receive(:sleep)
+  end
 
   it 'delegates to SubmitUsagePingService' do
-    allow(subject).to receive(:try_obtain_lease).and_return(true)
+    expect_next_instance_of(SubmitUsagePingService) { |service| expect(service).to receive(:execute) }
 
-    expect_next_instance_of(SubmitUsagePingService) do |instance|
-      expect(instance).to receive(:execute)
-    end
+    subject.perform
+  end
+
+  it "obtains a #{described_class::LEASE_TIMEOUT} second exclusive lease" do
+    expect(Gitlab::ExclusiveLeaseHelpers::SleepingLock)
+      .to receive(:new)
+      .with(described_class::LEASE_KEY, hash_including(timeout: described_class::LEASE_TIMEOUT))
+      .and_call_original
 
     subject.perform
   end
+
+  it 'sleeps for between 0 and 60 seconds' do
+    expect(subject).to receive(:sleep).with(0..60)
+
+    subject.perform
+  end
+
+  context 'when lease is not obtained' do
+    before do
+      Gitlab::ExclusiveLease.new(described_class::LEASE_KEY, timeout: described_class::LEASE_TIMEOUT).try_obtain
+    end
+
+    it 'does not invoke SubmitUsagePingService' do
+      allow_next_instance_of(SubmitUsagePingService) { |service| expect(service).not_to receive(:execute) }
+
+      expect { subject.perform }.to raise_error(Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError)
+    end
+  end
 end