Merge branch 'fix/user-activities-performance' into 'master'

Get rid of user activities table and replace it with Redis

Fixes https://gitlab.com/gitlab-org/gitlab-ee/issues/1158

See merge request !915

app/models/user.rb

@@ -97,7 +97,6 @@ class User < ActiveRecord::Base
   has_many :notification_settings,    dependent: :destroy
   has_many :award_emoji,              dependent: :destroy
   has_many :path_locks,               dependent: :destroy
-  has_one :user_activity, dependent: :destroy
 
   # Protected Branch Access
   has_many :protected_branch_merge_access_levels, dependent: :destroy, class_name: ProtectedBranch::MergeAccessLevel
@@ -154,7 +153,6 @@ class User < ActiveRecord::Base
   alias_attribute :private_token, :authentication_token
 
   delegate :path, to: :namespace, allow_nil: true, prefix: true
-  delegate :last_activity_at, to: :user_activity, allow_nil: true
 
   state_machine :state, initial: :active do
     event :block do
@@ -969,6 +967,12 @@ class User < ActiveRecord::Base
     end
   end
 
+  def record_activity
+    Gitlab::Redis.with do |redis|
+      redis.zadd('user/activities', Time.now.to_i, self.username)
+    end
+  end
+
   private
 
   # Returns a union query of projects that the user is authorized to access

app/models/user_activity.rb

-class UserActivity < ActiveRecord::Base
-  belongs_to :user, inverse_of: :user_activity
-
-  validates :user, uniqueness: true, presence: true
-  validates :last_activity_at, presence: true
-
-  # Updated version of http://apidock.com/rails/ActiveRecord/Timestamp/touch
-  # That accepts a new record.
-  def touch
-    current_time = current_time_from_proper_timezone
-
-    if persisted?
-      update_column(:last_activity_at, current_time)
-    else
-      self.last_activity_at = current_time
-      save!(validate: false)
-    end
-  end
-end

app/services/users/activity_service.rb

@@ -14,13 +14,9 @@ module Users
     private
 
     def record_activity
-      user_activity.touch unless Gitlab::Geo.secondary?
+      @author.record_activity unless Gitlab::Geo.secondary?
 
       Rails.logger.debug("Recorded activity: #{@activity} for User ID: #{@author.id} (username: #{@author.username}")
     end
-
-    def user_activity
-      UserActivity.find_or_initialize_by(user: @author)
-    end
   end
 end

db/post_migrate/20161128170531_drop_user_activities_table.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class DropUserActivitiesTable < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  # When using the methods "add_concurrent_index" or "add_column_with_default"
+  # you must disable the use of transactions as these methods can not run in an
+  # existing transaction. When using "add_concurrent_index" make sure that this
+  # method is the _only_ method called in the migration, any other changes
+  # should go in a separate migration. This ensures that upon failure _only_ the
+  # index creation fails and can be retried or reverted easily.
+  #
+  # To disable transactions uncomment the following line and remove these
+  # comments:
+  # disable_ddl_transaction!
+
+  def change
+    drop_table :user_activities
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20161118183841) do
+ActiveRecord::Schema.define(version: 20161128170531) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -1337,13 +1337,6 @@ ActiveRecord::Schema.define(version: 20161118183841) do
   add_index "u2f_registrations", ["key_handle"], name: "index_u2f_registrations_on_key_handle", using: :btree
   add_index "u2f_registrations", ["user_id"], name: "index_u2f_registrations_on_user_id", using: :btree
 
-  create_table "user_activities", force: :cascade do |t|
-    t.integer "user_id"
-    t.datetime "last_activity_at", null: false
-  end
-
-  add_index "user_activities", ["user_id"], name: "index_user_activities_on_user_id", unique: true, using: :btree
-
   create_table "user_agent_details", force: :cascade do |t|
     t.string "user_agent", null: false
     t.string "ip_address", null: false
@@ -1483,9 +1476,9 @@ ActiveRecord::Schema.define(version: 20161118183841) do
   add_foreign_key "path_locks", "projects"
   add_foreign_key "path_locks", "users"
   add_foreign_key "personal_access_tokens", "users"
-  add_foreign_key "protected_branch_merge_access_levels", "namespaces", column: "group_id"
   add_foreign_key "project_authorizations", "projects", on_delete: :cascade
   add_foreign_key "project_authorizations", "users", on_delete: :cascade
+  add_foreign_key "protected_branch_merge_access_levels", "namespaces", column: "group_id"
   add_foreign_key "protected_branch_merge_access_levels", "protected_branches"
   add_foreign_key "protected_branch_merge_access_levels", "users"
   add_foreign_key "protected_branch_push_access_levels", "namespaces", column: "group_id"
@@ -1495,5 +1488,4 @@ ActiveRecord::Schema.define(version: 20161118183841) do
   add_foreign_key "subscriptions", "projects", on_delete: :cascade
   add_foreign_key "trending_projects", "projects", on_delete: :cascade
   add_foreign_key "u2f_registrations", "users"
-  add_foreign_key "user_activities", "users", on_delete: :cascade
 end

lib/api/helpers/internal_helpers.rb

@@ -55,7 +55,6 @@ module API
 
       def log_user_activity(actor)
         commands = Gitlab::GitAccess::DOWNLOAD_COMMANDS +
-                    Gitlab::GitAccess::PUSH_COMMANDS +
                     Gitlab::GitAccess::GIT_ANNEX_COMMANDS
 
         ::Users::ActivityService.new(actor, 'Git SSH').execute if commands.include?(params[:action])

spec/controllers/sessions_controller_spec.rb

@@ -16,7 +16,9 @@ describe SessionsController do
         end
       end
 
-      context 'when using valid password' do
+      context 'when using valid password', :redis do
+        include UserActivitiesHelpers
+
         let(:user) { create(:user) }
 
         it 'authenticates user correctly' do
@@ -34,7 +36,7 @@ describe SessionsController do
         it 'updates the user activity' do
           expect do
             post(:create, user: { login: user.username, password: user.password })
-          end.to change { user.reload.last_activity_at }.from(nil)
+          end.to change { user_score }.from(0)
         end
       end
     end

spec/factories/user_activities.rb

-FactoryGirl.define do
-  factory :user_activity do
-    last_activity_at { Time.now }
-    user
-  end
-end

spec/requests/api/internal_spec.rb

@@ -195,7 +195,9 @@ describe API::API, api: true  do
     end
   end
 
-  describe "POST /internal/allowed" do
+  describe "POST /internal/allowed", :redis do
+    include UserActivitiesHelpers
+
     context "access granted" do
       before do
         project.team << [user, :developer]
@@ -213,7 +215,7 @@ describe API::API, api: true  do
           expect(response).to have_http_status(200)
           expect(json_response["status"]).to be_truthy
           expect(json_response["repository_path"]).to eq(project.wiki.repository.path_to_repo)
-          expect(key.user.reload.last_activity_at.to_i).to eq(Time.now.to_i)
+          expect(user_score).to be_zero
         end
       end
 
@@ -224,7 +226,7 @@ describe API::API, api: true  do
           expect(response).to have_http_status(200)
           expect(json_response["status"]).to be_truthy
           expect(json_response["repository_path"]).to eq(project.wiki.repository.path_to_repo)
-          expect(key.user.reload.last_activity_at.to_i).to eq(Time.now.to_i)
+          expect(user_score).not_to be_zero
         end
       end
 
@@ -235,7 +237,7 @@ describe API::API, api: true  do
           expect(response).to have_http_status(200)
           expect(json_response["status"]).to be_truthy
           expect(json_response["repository_path"]).to eq(project.repository.path_to_repo)
-          expect(key.user.reload.last_activity_at.to_i).to eq(Time.now.to_i)
+          expect(user_score).not_to be_zero
         end
       end
 
@@ -246,7 +248,7 @@ describe API::API, api: true  do
           expect(response).to have_http_status(200)
           expect(json_response["status"]).to be_truthy
           expect(json_response["repository_path"]).to eq(project.repository.path_to_repo)
-          expect(key.user.reload.last_activity_at.to_i).to eq(Time.now.to_i)
+          expect(user_score).to be_zero
         end
 
         context 'project as /namespace/project' do
@@ -282,7 +284,7 @@ describe API::API, api: true  do
 
           expect(response).to have_http_status(200)
           expect(json_response["status"]).to be_falsey
-          expect(key.user.reload.last_activity_at).to be_nil
+          expect(user_score).to be_zero
         end
       end
 
@@ -292,7 +294,7 @@ describe API::API, api: true  do
 
           expect(response).to have_http_status(200)
           expect(json_response["status"]).to be_falsey
-          expect(key.user.reload.last_activity_at).to be_nil
+          expect(user_score).to be_zero
         end
       end
     end
@@ -310,7 +312,7 @@ describe API::API, api: true  do
 
           expect(response).to have_http_status(200)
           expect(json_response["status"]).to be_falsey
-          expect(key.user.reload.last_activity_at).to be_nil
+          expect(user_score).to be_zero
         end
       end
 
@@ -320,7 +322,7 @@ describe API::API, api: true  do
 
           expect(response).to have_http_status(200)
           expect(json_response["status"]).to be_falsey
-          expect(key.user.reload.last_activity_at).to be_nil
+          expect(user_score).to be_zero
         end
       end
     end

spec/requests/git_http_spec.rb

@@ -204,7 +204,9 @@ describe 'Git HTTP requests', lib: true do
               end
             end
 
-            context "when the user isn't blocked" do
+            context "when the user isn't blocked", :redis do
+              include UserActivitiesHelpers
+
               it "responds with status 200" do
                 download(path, env) do |response|
                   expect(response.status).to eq(200)
@@ -212,8 +214,8 @@ describe 'Git HTTP requests', lib: true do
               end
 
               it 'updates the user last activity' do
-                download(path, env) do |response|
-                  expect(user.reload.last_activity_at).not_to be_nil
+                download(path, env) do |_response|
+                  expect(user_score).not_to be_zero
                 end
               end
             end

spec/services/event_create_service_spec.rb

 require 'spec_helper'
 
 describe EventCreateService, services: true do
+  include UserActivitiesHelpers
+
   let(:service) { EventCreateService.new }
 
   describe 'Issues' do
@@ -111,7 +113,7 @@ describe EventCreateService, services: true do
     end
   end
 
-  describe '#push' do
+  describe '#push', :redis do
     let(:project) { create(:empty_project) }
     let(:user) { create(:user) }
 
@@ -120,7 +122,7 @@ describe EventCreateService, services: true do
     end
 
     it 'updates user last activity' do
-      expect { service.push(project, user, {}) }.to change { user.last_activity_at }
+      expect { service.push(project, user, {}) }.to change { user_score }
     end
   end
 

spec/services/users/activity_service_spec.rb

 require 'spec_helper'
 
 describe Users::ActivityService, services: true do
+  include UserActivitiesHelpers
+
   let(:user) { create(:user) }
+
   subject(:service) { described_class.new(user, 'type') }
 
-  describe '#execute' do
+  describe '#execute', :redis do
     context 'when last activity is nil' do
-      it 'sets the last activity timestamp' do
+      before do
         service.execute
+      end
 
-        expect(user.last_activity_at).not_to be_nil
+      it 'sets the last activity timestamp for the user' do
+        expect(last_hour_members).to eq([user.username])
       end
+
+      it 'updates the same user' do
+        service.execute
+
+        expect(last_hour_members).to eq([user.username])
       end
 
-    context 'when activity_at is not nil' do
-      it 'updates the activity multiple times' do
-        activity = create(:user_activity, user: user)
+      it 'updates the timestamp of an existing user' do
+        Timecop.freeze(Date.tomorrow) do
+          expect { service.execute }.to change { user_score }.to(Time.now.to_i)
+        end
+      end
+
+      describe 'other user' do
+        it 'updates other user' do
+          other_user = create(:user)
+          described_class.new(other_user, 'type').execute
 
-        Timecop.travel(activity.last_activity_at + 1.minute) do
-          expect { service.execute }.to change { user.reload.last_activity_at }
+          expect(last_hour_members).to match_array([user.username, other_user.username])
         end
       end
     end
@@ -27,7 +43,9 @@ describe Users::ActivityService, services: true do
       before { allow(Gitlab::Geo).to receive(:secondary?).and_return(true) }
 
       it 'does not update last_activity_at' do
-        expect { service.execute }.not_to change { user.reload.last_activity_at }
+        service.execute
+
+        expect(last_hour_members).to eq([])
       end
     end
   end

spec/support/user_activities_helpers.rb

+module UserActivitiesHelpers
+  def last_hour_members
+    Gitlab::Redis.with do |redis|
+      redis.zrangebyscore(user_activities_key, 1.hour.ago.to_i, Time.now.to_i)
+    end
+  end
+
+  def user_score
+    Gitlab::Redis.with do |redis|
+      redis.zscore(user_activities_key, user.username).to_i
+    end
+  end
+
+  def user_activities_key
+    'user/activities'
+  end
+end