Delete UserActivities and related workers

app/services/users/activity_service.rb

@@ -1,12 +1,19 @@
 module Users
   class ActivityService
+    LEASE_TIMEOUT = 1.minute.to_i
+
     def initialize(author, activity)
-      @author = author.respond_to?(:user) ? author.user : author
+      @user = if author.respond_to?(:username)
+                author
+              elsif author.respond_to?(:user)
+                author.user
+              end
+
       @activity = activity
     end
 
     def execute
-      return unless @author && @author.is_a?(User)
+      return unless @user
 
       record_activity
     end
@@ -14,9 +21,14 @@ module Users
     private
 
     def record_activity
-      Gitlab::UserActivities.record(@author.id) if Gitlab::Database.read_write?
+      return if Gitlab::Database.read_only?
 
-      Rails.logger.debug("Recorded activity: #{@activity} for User ID: #{@author.id} (username: #{@author.username})")
+      lease = Gitlab::ExclusiveLease.new("acitvity_service:#{@user.id}",
+                                         timeout: LEASE_TIMEOUT)
+      return unless lease.try_obtain
+
+      @user.update_attribute(:last_activity_on, Date.today)
+      Rails.logger.debug("Recorded activity: #{@activity} for User ID: #{@user.id} (username: #{@user.username})")
     end
   end
 end

app/workers/all_queues.yml

@@ -13,7 +13,6 @@
 - cronjob:repository_archive_cache
 - cronjob:repository_check_dispatch
 - cronjob:requests_profiles
-- cronjob:schedule_update_user_activity
 - cronjob:stuck_ci_jobs
 - cronjob:stuck_import_jobs
 - cronjob:stuck_merge_jobs
@@ -114,7 +113,6 @@
 - storage_migrator
 - system_hook_push
 - update_merge_requests
-- update_user_activity
 - upload_checksum
 - web_hook
 - repository_update_remote_mirror

app/workers/schedule_update_user_activity_worker.rb

@@ -1,12 +0,0 @@
-# frozen_string_literal: true
-
-class ScheduleUpdateUserActivityWorker
-  include ApplicationWorker
-  include CronjobQueue
-
-  def perform(batch_size = 500)
-    Gitlab::UserActivities.new.each_slice(batch_size) do |batch|
-      UpdateUserActivityWorker.perform_async(Hash[batch])
-    end
-  end
-end

app/workers/update_user_activity_worker.rb

@@ -1,27 +0,0 @@
-# frozen_string_literal: true
-
-class UpdateUserActivityWorker
-  include ApplicationWorker
-
-  def perform(pairs)
-    pairs = cast_data(pairs)
-    ids = pairs.keys
-    conditions = 'WHEN id = ? THEN ? ' * ids.length
-
-    User.where(id: ids)
-      .update_all([
-        "last_activity_on = CASE #{conditions} ELSE last_activity_on END",
-        *pairs.to_a.flatten
-      ])
-
-    Gitlab::UserActivities.new.delete(*ids)
-  end
-
-  private
-
-  def cast_data(pairs)
-    pairs.each_with_object({}) do |(key, value), new_pairs|
-      new_pairs[key.to_i] = Time.at(value.to_i).to_s(:db)
-    end
-  end
-end

changelogs/unreleased/43312-remove_user_activity_workers.yml

@@ -0,0 +1,5 @@
+---
+title: Delete UserActivities and related workers
+merge_request: 20597
+author:
+type: performance

config/initializers/1_settings.rb

@@ -318,10 +318,6 @@ Settings.cron_jobs['gitlab_usage_ping_worker'] ||= Settingslogic.new({})
 Settings.cron_jobs['gitlab_usage_ping_worker']['cron'] ||= Settings.__send__(:cron_for_usage_ping)
 Settings.cron_jobs['gitlab_usage_ping_worker']['job_class'] = 'GitlabUsagePingWorker'
 
-Settings.cron_jobs['schedule_update_user_activity_worker'] ||= Settingslogic.new({})
-Settings.cron_jobs['schedule_update_user_activity_worker']['cron'] ||= '30 0 * * *'
-Settings.cron_jobs['schedule_update_user_activity_worker']['job_class'] = 'ScheduleUpdateUserActivityWorker'
-
 Settings.cron_jobs['remove_old_web_hook_logs_worker'] ||= Settingslogic.new({})
 Settings.cron_jobs['remove_old_web_hook_logs_worker']['cron'] ||= '40 0 * * *'
 Settings.cron_jobs['remove_old_web_hook_logs_worker']['job_class'] = 'RemoveOldWebHookLogsWorker'

config/sidekiq_queues.yml

@@ -62,7 +62,6 @@
   - [default, 1]
   - [pages, 1]
   - [system_hook_push, 1]
-  - [update_user_activity, 1]
   - [propagate_service_template, 1]
   - [background_migration, 1]
   - [gcp_cluster, 1]
@@ -77,4 +76,3 @@
   - [repository_remove_remote, 1]
   - [create_note_diff_file, 1]
   - [delete_diff_files, 1]
- 

lib/gitlab/user_activities.rb

@@ -1,34 +0,0 @@
-module Gitlab
-  class UserActivities
-    include Enumerable
-
-    KEY = 'users:activities'.freeze
-    BATCH_SIZE = 500
-
-    def self.record(key, time = Time.now)
-      Gitlab::Redis::SharedState.with do |redis|
-        redis.hset(KEY, key, time.to_i)
-      end
-    end
-
-    def delete(*keys)
-      Gitlab::Redis::SharedState.with do |redis|
-        redis.hdel(KEY, keys)
-      end
-    end
-
-    def each
-      cursor = 0
-      loop do
-        cursor, pairs =
-          Gitlab::Redis::SharedState.with do |redis|
-            redis.hscan(KEY, cursor, count: BATCH_SIZE)
-          end
-
-        Hash[pairs].each { |pair| yield pair }
-
-        break if cursor == '0'
-      end
-    end
-  end
-end

spec/controllers/sessions_controller_spec.rb

@@ -50,8 +50,6 @@ describe SessionsController do
       end
 
       context 'when using valid password', :clean_gitlab_redis_shared_state do
-        include UserActivitiesHelpers
-
         let(:user) { create(:user) }
         let(:user_params) { { login: user.username, password: user.password } }
 
@@ -77,7 +75,7 @@ describe SessionsController do
         it 'updates the user activity' do
           expect do
             post(:create, user: user_params)
-          end.to change { user_activity(user) }
+          end.to change { user.reload.last_activity_on }.to(Date.today)
         end
       end
 

spec/lib/gitlab/user_activities_spec.rb

@@ -1,127 +0,0 @@
-require 'spec_helper'
-
-describe Gitlab::UserActivities, :clean_gitlab_redis_shared_state do
-  let(:now) { Time.now }
-
-  describe '.record' do
-    context 'with no time given' do
-      it 'uses Time.now and records an activity in SharedState' do
-        Timecop.freeze do
-          now # eager-load now
-          described_class.record(42)
-        end
-
-        Gitlab::Redis::SharedState.with do |redis|
-          expect(redis.hscan(described_class::KEY, 0)).to eq(['0', [['42', now.to_i.to_s]]])
-        end
-      end
-    end
-
-    context 'with a time given' do
-      it 'uses the given time and records an activity in SharedState' do
-        described_class.record(42, now)
-
-        Gitlab::Redis::SharedState.with do |redis|
-          expect(redis.hscan(described_class::KEY, 0)).to eq(['0', [['42', now.to_i.to_s]]])
-        end
-      end
-    end
-  end
-
-  describe '.delete' do
-    context 'with a single key' do
-      context 'and key exists' do
-        it 'removes the pair from SharedState' do
-          described_class.record(42, now)
-
-          Gitlab::Redis::SharedState.with do |redis|
-            expect(redis.hscan(described_class::KEY, 0)).to eq(['0', [['42', now.to_i.to_s]]])
-          end
-
-          subject.delete(42)
-
-          Gitlab::Redis::SharedState.with do |redis|
-            expect(redis.hscan(described_class::KEY, 0)).to eq(['0', []])
-          end
-        end
-      end
-
-      context 'and key does not exist' do
-        it 'removes the pair from SharedState' do
-          Gitlab::Redis::SharedState.with do |redis|
-            expect(redis.hscan(described_class::KEY, 0)).to eq(['0', []])
-          end
-
-          subject.delete(42)
-
-          Gitlab::Redis::SharedState.with do |redis|
-            expect(redis.hscan(described_class::KEY, 0)).to eq(['0', []])
-          end
-        end
-      end
-    end
-
-    context 'with multiple keys' do
-      context 'and all keys exist' do
-        it 'removes the pair from SharedState' do
-          described_class.record(41, now)
-          described_class.record(42, now)
-
-          Gitlab::Redis::SharedState.with do |redis|
-            expect(redis.hscan(described_class::KEY, 0)).to eq(['0', [['41', now.to_i.to_s], ['42', now.to_i.to_s]]])
-          end
-
-          subject.delete(41, 42)
-
-          Gitlab::Redis::SharedState.with do |redis|
-            expect(redis.hscan(described_class::KEY, 0)).to eq(['0', []])
-          end
-        end
-      end
-
-      context 'and some keys does not exist' do
-        it 'removes the existing pair from SharedState' do
-          described_class.record(42, now)
-
-          Gitlab::Redis::SharedState.with do |redis|
-            expect(redis.hscan(described_class::KEY, 0)).to eq(['0', [['42', now.to_i.to_s]]])
-          end
-
-          subject.delete(41, 42)
-
-          Gitlab::Redis::SharedState.with do |redis|
-            expect(redis.hscan(described_class::KEY, 0)).to eq(['0', []])
-          end
-        end
-      end
-    end
-  end
-
-  describe 'Enumerable' do
-    before do
-      described_class.record(40, now)
-      described_class.record(41, now)
-      described_class.record(42, now)
-    end
-
-    it 'allows to read the activities sequentially' do
-      expected = { '40' => now.to_i.to_s, '41' => now.to_i.to_s, '42' => now.to_i.to_s }
-
-      actual = described_class.new.each_with_object({}) do |(key, time), actual|
-        actual[key] = time
-      end
-
-      expect(actual).to eq(expected)
-    end
-
-    context 'with many records' do
-      before do
-        1_000.times { |i| described_class.record(i, now) }
-      end
-
-      it 'is possible to loop through all the records' do
-        expect(described_class.new.count).to eq(1_000)
-      end
-    end
-  end
-end

spec/requests/api/internal_spec.rb

@@ -279,7 +279,7 @@ describe API::Internal do
           expect(json_response["status"]).to be_truthy
           expect(json_response["repository_path"]).to eq('/')
           expect(json_response["gl_repository"]).to eq("wiki-#{project.id}")
-          expect(user).not_to have_an_activity_record
+          expect(user.reload.last_activity_on).to be_nil
         end
       end
 
@@ -291,7 +291,7 @@ describe API::Internal do
           expect(json_response["status"]).to be_truthy
           expect(json_response["repository_path"]).to eq('/')
           expect(json_response["gl_repository"]).to eq("wiki-#{project.id}")
-          expect(user).to have_an_activity_record
+          expect(user.reload.last_activity_on).to eql(Date.today)
         end
       end
 
@@ -309,7 +309,7 @@ describe API::Internal do
           expect(json_response["gitaly"]["repository"]["relative_path"]).to eq(project.repository.gitaly_repository.relative_path)
           expect(json_response["gitaly"]["address"]).to eq(Gitlab::GitalyClient.address(project.repository_storage))
           expect(json_response["gitaly"]["token"]).to eq(Gitlab::GitalyClient.token(project.repository_storage))
-          expect(user).to have_an_activity_record
+          expect(user.reload.last_activity_on).to eql(Date.today)
         end
       end
 
@@ -328,7 +328,7 @@ describe API::Internal do
             expect(json_response["gitaly"]["repository"]["relative_path"]).to eq(project.repository.gitaly_repository.relative_path)
             expect(json_response["gitaly"]["address"]).to eq(Gitlab::GitalyClient.address(project.repository_storage))
             expect(json_response["gitaly"]["token"]).to eq(Gitlab::GitalyClient.token(project.repository_storage))
-            expect(user).not_to have_an_activity_record
+            expect(user.reload.last_activity_on).to be_nil
           end
         end
       end
@@ -345,7 +345,7 @@ describe API::Internal do
 
           expect(response).to have_gitlab_http_status(200)
           expect(json_response["status"]).to be_falsey
-          expect(user).not_to have_an_activity_record
+          expect(user.reload.last_activity_on).to be_nil
         end
       end
 
@@ -355,7 +355,7 @@ describe API::Internal do
 
           expect(response).to have_gitlab_http_status(200)
           expect(json_response["status"]).to be_falsey
-          expect(user).not_to have_an_activity_record
+          expect(user.reload.last_activity_on).to be_nil
         end
       end
     end
@@ -373,7 +373,7 @@ describe API::Internal do
 
           expect(response).to have_gitlab_http_status(200)
           expect(json_response["status"]).to be_falsey
-          expect(user).not_to have_an_activity_record
+          expect(user.reload.last_activity_on).to be_nil
         end
       end
 
@@ -383,7 +383,7 @@ describe API::Internal do
 
           expect(response).to have_gitlab_http_status(200)
           expect(json_response["status"]).to be_falsey
-          expect(user).not_to have_an_activity_record
+          expect(user.reload.last_activity_on).to be_nil
         end
       end
     end

spec/requests/git_http_spec.rb

@@ -5,7 +5,6 @@ describe 'Git HTTP requests' do
   include TermsHelper
   include GitHttpHelpers
   include WorkhorseHelpers
-  include UserActivitiesHelpers
 
   shared_examples 'pulls require Basic HTTP Authentication' do
     context "when no credentials are provided" do
@@ -440,10 +439,10 @@ describe 'Git HTTP requests' do
                 end
 
                 it 'updates the user last activity', :clean_gitlab_redis_shared_state do
-                  expect(user_activity(user)).to be_nil
+                  expect(user.last_activity_on).to be_nil
 
                   download(path, env) do |response|
-                    expect(user_activity(user)).to be_present
+                    expect(user.reload.last_activity_on).to eql(Date.today)
                   end
                 end
               end

spec/services/event_create_service_spec.rb

@@ -1,8 +1,6 @@
 require 'spec_helper'
 
 describe EventCreateService do
-  include UserActivitiesHelpers
-
   let(:service) { described_class.new }
 
   describe 'Issues' do
@@ -146,7 +144,7 @@ describe EventCreateService do
 
     it 'updates user last activity' do
       expect { service.push(project, user, push_data) }
-        .to change { user_activity(user) }
+        .to change {  user.last_activity_on }.to(Date.today)
     end
 
     it 'caches the last push event for the user' do

spec/services/users/activity_service_spec.rb

@@ -1,60 +1,61 @@
 require 'spec_helper'
 
 describe Users::ActivityService do
-  include UserActivitiesHelpers
+  include ExclusiveLeaseHelpers
 
-  let(:user) { create(:user) }
+  let(:user) { create(:user, last_activity_on: last_activity_on) }
 
-  subject(:service) { described_class.new(user, 'type') }
+  subject { described_class.new(user, 'type') }
 
   describe '#execute', :clean_gitlab_redis_shared_state do
     context 'when last activity is nil' do
-      before do
+      let(:last_activity_on) { nil }
-        service.execute
+
+      it 'updates last_activity_on for the user' do
+        expect { subject.execute }
+          .to change(user, :last_activity_on).from(last_activity_on).to(Date.today)
       end
+    end
 
-      it 'sets the last activity timestamp for the user' do
+    context 'when last activity is in the past' do
-        expect(last_hour_user_ids).to eq([user.id])
+      let(:last_activity_on) { Date.today - 1.week }
+
+      it 'updates last_activity_on for the user' do
+        expect { subject.execute }
+          .to change(user, :last_activity_on)
+                .from(last_activity_on)
+                .to(Date.today)
       end
+    end
 
-      it 'updates the same user' do
+    context 'when last activity is today' do
-        service.execute
+      let(:last_activity_on) { Date.today }
 
-        expect(last_hour_user_ids).to eq([user.id])
+      it 'does not update last_activity_on' do
-      end
+        expect { subject.execute }.not_to change(user, :last_activity_on)
-
-      it 'updates the timestamp of an existing user' do
-        Timecop.freeze(Date.tomorrow) do
-          expect { service.execute }.to change { user_activity(user) }.to(Time.now.to_i.to_s)
-        end
-      end
-
-      describe 'other user' do
-        it 'updates other user' do
-          other_user = create(:user)
-          described_class.new(other_user, 'type').execute
-
-          expect(last_hour_user_ids).to match_array([user.id, other_user.id])
-        end
       end
     end
 
     context 'when in GitLab read-only instance' do
+      let(:last_activity_on) { nil }
+
       before do
         allow(Gitlab::Database).to receive(:read_only?).and_return(true)
       end
 
-      it 'does not update last_activity_at' do
+      it 'does not update last_activity_on' do
-        service.execute
+        expect { subject.execute }.not_to change(user, :last_activity_on)
+      end
+    end
 
-        expect(last_hour_user_ids).to eq([])
+    context 'when a lease could not be obtained' do
+      let(:last_activity_on) { nil }
+
+      it 'does not update last_activity_on' do
+        stub_exclusive_lease_taken("acitvity_service:#{user.id}", timeout: 1.minute.to_i)
+
+        expect { subject.execute }.not_to change(user, :last_activity_on)
       end
     end
   end
-
-  def last_hour_user_ids
-    Gitlab::UserActivities.new
-      .select { |k, v| v >= 1.hour.ago.to_i.to_s }
-      .map { |k, _| k.to_i }
-  end
 end

spec/support/helpers/user_activities_helpers.rb

@@ -1,7 +0,0 @@
-module UserActivitiesHelpers
-  def user_activity(user)
-    Gitlab::UserActivities.new
-      .find { |k, _| k == user.id.to_s }&.
-      second
-  end
-end

spec/support/matchers/user_activity_matchers.rb

@@ -1,5 +0,0 @@
-RSpec::Matchers.define :have_an_activity_record do |expected|
-  match do |user|
-    expect(Gitlab::UserActivities.new.find { |k, _| k == user.id.to_s }).to be_present
-  end
-end

spec/workers/schedule_update_user_activity_worker_spec.rb

@@ -1,25 +0,0 @@
-require 'spec_helper'
-
-describe ScheduleUpdateUserActivityWorker, :clean_gitlab_redis_shared_state do
-  let(:now) { Time.now }
-
-  before do
-    Gitlab::UserActivities.record('1', now)
-    Gitlab::UserActivities.record('2', now)
-  end
-
-  it 'schedules UpdateUserActivityWorker once' do
-    expect(UpdateUserActivityWorker).to receive(:perform_async).with({ '1' => now.to_i.to_s, '2' => now.to_i.to_s })
-
-    subject.perform
-  end
-
-  context 'when specifying a batch size' do
-    it 'schedules UpdateUserActivityWorker twice' do
-      expect(UpdateUserActivityWorker).to receive(:perform_async).with({ '1' => now.to_i.to_s })
-      expect(UpdateUserActivityWorker).to receive(:perform_async).with({ '2' => now.to_i.to_s })
-
-      subject.perform(1)
-    end
-  end
-end

spec/workers/update_user_activity_worker_spec.rb

@@ -1,35 +0,0 @@
-require 'spec_helper'
-
-describe UpdateUserActivityWorker, :clean_gitlab_redis_shared_state do
-  let(:user_active_2_days_ago) { create(:user, current_sign_in_at: 10.months.ago) }
-  let(:user_active_yesterday_1) { create(:user) }
-  let(:user_active_yesterday_2) { create(:user) }
-  let(:user_active_today) { create(:user) }
-  let(:data) do
-    {
-      user_active_2_days_ago.id.to_s => 2.days.ago.at_midday.to_i.to_s,
-      user_active_yesterday_1.id.to_s => 1.day.ago.at_midday.to_i.to_s,
-      user_active_yesterday_2.id.to_s => 1.day.ago.at_midday.to_i.to_s,
-      user_active_today.id.to_s => Time.now.to_i.to_s
-    }
-  end
-
-  it 'updates users.last_activity_on' do
-    subject.perform(data)
-
-    aggregate_failures do
-      expect(user_active_2_days_ago.reload.last_activity_on).to eq(2.days.ago.to_date)
-      expect(user_active_yesterday_1.reload.last_activity_on).to eq(1.day.ago.to_date)
-      expect(user_active_yesterday_2.reload.last_activity_on).to eq(1.day.ago.to_date)
-      expect(user_active_today.reload.reload.last_activity_on).to eq(Date.today)
-    end
-  end
-
-  it 'deletes the pairs from SharedState' do
-    data.each { |id, time| Gitlab::UserActivities.record(id, time) }
-
-    subject.perform(data)
-
-    expect(Gitlab::UserActivities.new.to_a).to be_empty
-  end
-end