Fixed pagination of web hook logs

For reasons unknown, the logs of a web hook were paginated in memory.
This would result in the "Edit" page of a web hook timing out once it
has more than a few thousand log entries.

This commit makes the following changes:

1. We use LIMIT/OFFSET to paginate the data, instead of doing this in
   memory.

2. We limit the logs to the last two days, just like the documentation
   says (instead of retrieving everything).

3. We change the indexes on "web_hook_logs" so the query to get the data
   can perform a backwards index scan, without the need for a Filter.

These changes combined ensure that Projects::HooksController#edit no
longer times out.

7 files changed, 60 insertions, 5 deletions

diff --git a/app/controllers/admin/hooks_controller.rb b/app/controllers/admin/hooks_controller.rb
index fb788c47ef1..6944857bd33 100644
--- a/app/controllers/admin/hooks_controller.rb
+++ b/app/controllers/admin/hooks_controller.rb
@@ -52,8 +52,7 @@ class Admin::HooksController < Admin::ApplicationController
   end
 
   def hook_logs
-    @hook_logs ||=
-      Kaminari.paginate_array(hook.web_hook_logs.order(created_at: :desc)).page(params[:page])
+    @hook_logs ||= hook.web_hook_logs.recent.page(params[:page])
   end
 
   def hook_params
diff --git a/app/controllers/projects/hooks_controller.rb b/app/controllers/projects/hooks_controller.rb
index dd7aa1a67b9..6800d742b0a 100644
--- a/app/controllers/projects/hooks_controller.rb
+++ b/app/controllers/projects/hooks_controller.rb
@@ -58,8 +58,7 @@ class Projects::HooksController < Projects::ApplicationController
   end
 
   def hook_logs
-    @hook_logs ||=
-      Kaminari.paginate_array(hook.web_hook_logs.order(created_at: :desc)).page(params[:page])
+    @hook_logs ||= hook.web_hook_logs.recent.page(params[:page])
   end
 
   def hook_params
diff --git a/app/models/hooks/web_hook_log.rb b/app/models/hooks/web_hook_log.rb
index e72c125fb69..59a1f2aed69 100644
--- a/app/models/hooks/web_hook_log.rb
+++ b/app/models/hooks/web_hook_log.rb
@@ -7,6 +7,11 @@ class WebHookLog < ActiveRecord::Base
 
   validates :web_hook, presence: true
 
+  def self.recent
+    where('created_at >= ?', 2.days.ago.beginning_of_day)
+      .order(created_at: :desc)
+  end
+
   def success?
     response_status =~ /^2/
   end
diff --git a/changelogs/unreleased/web-hooks-log-pagination.yml b/changelogs/unreleased/web-hooks-log-pagination.yml
new file mode 100644
index 00000000000..fd9e4f9ca13
--- /dev/null
+++ b/changelogs/unreleased/web-hooks-log-pagination.yml
@@ -0,0 +1,5 @@
+---
+title: Fixed pagination of web hook logs
+merge_request:
+author:
+type: performance
diff --git a/db/migrate/20180628124813_alter_web_hook_logs_indexes.rb b/db/migrate/20180628124813_alter_web_hook_logs_indexes.rb
new file mode 100644
index 00000000000..1878e76811d
--- /dev/null
+++ b/db/migrate/20180628124813_alter_web_hook_logs_indexes.rb
@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AlterWebHookLogsIndexes < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  # "created_at" comes first so the Sidekiq worker pruning old webhook logs can
+  # use a composite index index.
+  #
+  # We leave the old standalone index on "web_hook_id" in place so future code
+  # that doesn't care about "created_at" can still use that index.
+  COLUMNS_TO_INDEX = %i[created_at web_hook_id]
+
+  def up
+    add_concurrent_index(:web_hook_logs, COLUMNS_TO_INDEX)
+  end
+
+  def down
+    remove_concurrent_index(:web_hook_logs, COLUMNS_TO_INDEX)
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 0112fc726d4..38c1710d73c 100644
--- a/db/schema.rb
+++ b/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: 20180626125654) do
+ActiveRecord::Schema.define(version: 20180628124813) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -2144,6 +2144,7 @@ ActiveRecord::Schema.define(version: 20180626125654) do
     t.datetime "updated_at", null: false
   end
 
+  add_index "web_hook_logs", ["created_at", "web_hook_id"], name: "index_web_hook_logs_on_created_at_and_web_hook_id", using: :btree
   add_index "web_hook_logs", ["web_hook_id"], name: "index_web_hook_logs_on_web_hook_id", using: :btree
 
   create_table "web_hooks", force: :cascade do |t|
diff --git a/spec/models/hooks/web_hook_log_spec.rb b/spec/models/hooks/web_hook_log_spec.rb
index 19bc88b1333..744a6ccae8b 100644
--- a/spec/models/hooks/web_hook_log_spec.rb
+++ b/spec/models/hooks/web_hook_log_spec.rb
@@ -9,6 +9,24 @@ describe WebHookLog do
 
   it { is_expected.to validate_presence_of(:web_hook) }
 
+  describe '.recent' do
+    let(:hook) { create(:project_hook) }
+
+    it 'does not return web hook logs that are too old' do
+      create(:web_hook_log, web_hook: hook, created_at: 91.days.ago)
+
+      expect(described_class.recent.size).to be_zero
+    end
+
+    it 'returns the web hook logs in descending order' do
+      hook1 = create(:web_hook_log, web_hook: hook, created_at: 2.hours.ago)
+      hook2 = create(:web_hook_log, web_hook: hook, created_at: 1.hour.ago)
+      hooks = described_class.recent.to_a
+
+      expect(hooks).to eq([hook2, hook1])
+    end
+  end
+
   describe '#success?' do
     let(:web_hook_log) { build(:web_hook_log, response_status: status) }