Cache merged and closed events data in merge_request_metrics table

app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_closed.js

@@ -16,9 +16,9 @@ export default {
       <div class="media-body">
         <mr-widget-author-and-time
           actionText="Closed by"
-          :author="mr.closedEvent.author"
-          :dateTitle="mr.closedEvent.updatedAt"
-          :dateReadable="mr.closedEvent.formattedUpdatedAt"
+          :author="mr.metrics.closedBy"
+          :dateTitle="mr.metrics.closedAt"
+          :dateReadable="mr.metrics.readableClosedAt"
         />
         <section class="mr-info-list">
           <p>

app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.js

@@ -68,9 +68,9 @@ export default {
         <div class="space-children">
           <mr-widget-author-and-time
             actionText="Merged by"
-            :author="mr.mergedEvent.author"
-            :date-title="mr.mergedEvent.updatedAt"
-            :date-readable="mr.mergedEvent.formattedUpdatedAt" />
+            :author="mr.metrics.mergedBy"
+            :date-title="mr.metrics.mergedAt"
+            :date-readable="mr.metrics.readableMergedAt" />
           <a
             v-if="mr.canRevertInCurrentMR"
             v-tooltip

app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js

@@ -39,9 +39,8 @@ export default class MergeRequestStore {
     }
     this.updatedAt = data.updated_at;
-    this.mergedEvent = MergeRequestStore.getEventObject(data.merge_event);
-    this.closedEvent = MergeRequestStore.getEventObject(data.closed_event);
-    this.setToMWPSBy = MergeRequestStore.getAuthorObject({ author: data.merge_user || {} });
+    this.metrics = MergeRequestStore.buildMetrics(data.metrics);
+    this.setToMWPSBy = MergeRequestStore.formatUserObject(data.merge_user || {});
     this.mergeUserId = data.merge_user_id;
     this.currentUserId = gon.current_user_id;
     this.sourceBranchPath = data.source_branch_path;
@@ -125,43 +124,38 @@ export default class MergeRequestStore {
     return this.state === stateKey.nothingToMerge;
   }
-  static getEventObject(event) {
+  static buildMetrics(metrics) {
     return {
-      author: MergeRequestStore.getAuthorObject(event),
-      updatedAt: formatDate(MergeRequestStore.getEventUpdatedAtDate(event)),
-      formattedUpdatedAt: MergeRequestStore.getEventDate(event),
+      mergedBy: MergeRequestStore.formatUserObject(metrics.merged_by),
+      closedBy: MergeRequestStore.formatUserObject(metrics.closed_by),
+      mergedAt: formatDate(metrics.merged_at),
+      closedAt: formatDate(metrics.closed_at),
+      readableMergedAt: MergeRequestStore.getReadableDate(metrics.merged_at),
+      readableClosedAt: MergeRequestStore.getReadableDate(metrics.closed_at),
     };
   }
-  static getAuthorObject(event) {
-    if (!event) {
+  static formatUserObject(user) {
+    if (!user) {
       return {};
     }
     return {
-      name: event.author.name || '',
-      username: event.author.username || '',
-      webUrl: event.author.web_url || '',
-      avatarUrl: event.author.avatar_url || '',
+      name: user.name || '',
+      username: user.username || '',
+      webUrl: user.web_url || '',
+      avatarUrl: user.avatar_url || '',
     };
   }
-  static getEventUpdatedAtDate(event) {
-    if (!event) {
+  static getReadableDate(date) {
+    if (!date) {
       return '';
     }
-    return event.updated_at;
-  }
-
-  static getEventDate(event) {
     const timeagoInstance = new Timeago();
-    if (!event) {
-      return '';
-    }
-
-    return timeagoInstance.format(MergeRequestStore.getEventUpdatedAtDate(event));
+    return timeagoInstance.format(date);
   }
 }

app/models/concerns/issuable.rb

@@ -96,7 +96,7 @@ module Issuable
     strip_attributes :title
-    after_save :record_metrics, unless: :imported?
+    after_save :ensure_metrics, unless: :imported?
     # We want to use optimistic lock for cases when only title or description are involved
     # http://api.rubyonrails.org/classes/ActiveRecord/Locking/Optimistic.html
@@ -335,11 +335,6 @@ module Issuable
     false
   end
-  def record_metrics
-    metrics = self.metrics || create_metrics
-    metrics.record!
-  end
-
   ##
   # Override in issuable specialization
   #
@@ -347,6 +342,10 @@ module Issuable
     false
   end
+  def ensure_metrics
+    self.metrics || create_metrics
+  end
+
   ##
   # Overriden in MergeRequest
   #

app/models/issue.rb

@@ -276,6 +276,11 @@ class Issue < ActiveRecord::Base
   private
+  def ensure_metrics
+    super
+    metrics.record!
+  end
+
   # Returns `true` if the given User can read the current Issue.
   #
   # This method duplicates the same check of issue_policy.rb

app/models/merge_request/metrics.rb

 class MergeRequest::Metrics < ActiveRecord::Base
   belongs_to :merge_request
   belongs_to :pipeline, class_name: 'Ci::Pipeline', foreign_key: :pipeline_id
-
-  def record!
-    if merge_request.merged? && self.merged_at.blank?
-      self.merged_at = Time.now
-    end
-
-    self.save
-  end
+  belongs_to :latest_closed_by, class_name: 'User'
+  belongs_to :merged_by, class_name: 'User'
 end

app/serializers/event_entity.rb

-class EventEntity < Grape::Entity
-  expose :author, using: UserEntity
-  expose :updated_at
-end

app/serializers/merge_request_metrics_entity.rb

+class MergeRequestMetricsEntity < Grape::Entity
+  expose :latest_closed_at, as: :closed_at
+  expose :merged_at
+  expose :latest_closed_by, as: :closed_by, using: UserEntity
+  expose :merged_by, using: UserEntity
+end

app/serializers/merge_request_widget_entity.rb

@@ -17,9 +17,11 @@ class MergeRequestWidgetEntity < IssuableEntity
     merge_request.project.merge_requests_ff_only_enabled
   end
-  # Events
-  expose :merge_event, using: EventEntity
-  expose :closed_event, using: EventEntity
+  expose :metrics do |merge_request|
+    metrics = build_metrics(merge_request)
+
+    MergeRequestMetricsEntity.new(metrics).as_json
+  end
   # User entities
   expose :merge_user, using: UserEntity
@@ -178,4 +180,28 @@ class MergeRequestWidgetEntity < IssuableEntity
     @presenters ||= {}
     @presenters[merge_request] ||= MergeRequestPresenter.new(merge_request, current_user: current_user)
   end
+
+  # Once SchedulePopulateMergeRequestMetricsWithEventsData fully runs,
+  # we can remove this method and just serialize MergeRequest#metrics
+  # instead. See https://gitlab.com/gitlab-org/gitlab-ce/issues/41587
+  def build_metrics(merge_request)
+    # There's no need to query and serialize metrics data for merge requests that are not
+    # merged or closed.
+    case merge_request.state
+    when 'merged'
+      merge_request.metrics&.merged_by_id ? merge_request.metrics : build_metrics_from_events(merge_request)
+    when 'closed'
+      merge_request.metrics&.latest_closed_by_id ? merge_request.metrics : build_metrics_from_events(merge_request)
+    end
+  end
+
+  def build_metrics_from_events(merge_request)
+    closed_event = merge_request.closed_event
+    merge_event = merge_request.merge_event
+
+    MergeRequest::Metrics.new(latest_closed_at: closed_event&.updated_at,
+                              latest_closed_by: closed_event&.author,
+                              merged_at: merge_event&.updated_at,
+                              merged_by: merge_event&.author)
+  end
 end

app/services/event_create_service.rb

@@ -103,6 +103,6 @@ class EventCreateService
       author_id: current_user.id
     )
-    Event.create(attributes)
+    Event.create!(attributes)
   end
 end

app/services/merge_request_metrics_service.rb

+class MergeRequestMetricsService
+  delegate :update!, to: :@merge_request_metrics
+
+  def initialize(merge_request_metrics)
+    @merge_request_metrics = merge_request_metrics
+  end
+
+  def merge(event)
+    update!(merged_by_id: event.author_id, merged_at: event.created_at)
+  end
+
+  def close(event)
+    update!(latest_closed_by_id: event.author_id, latest_closed_at: event.created_at)
+  end
+
+  def reopen
+    update!(latest_closed_by_id: nil, latest_closed_at: nil)
+  end
+end

app/services/merge_requests/base_service.rb

@@ -24,6 +24,10 @@ module MergeRequests
     private
+    def merge_request_metrics_service(merge_request)
+      MergeRequestMetricsService.new(merge_request.metrics)
+    end
+
     def create_assignee_note(merge_request)
       SystemNoteService.change_assignee(
         merge_request, merge_request.project, current_user, merge_request.assignee)

app/services/merge_requests/close_service.rb

@@ -8,7 +8,7 @@ module MergeRequests
       merge_request.allow_broken = true
       if merge_request.close
-        event_service.close_mr(merge_request, current_user)
+        create_event(merge_request)
         create_note(merge_request)
         notification_service.close_mr(merge_request, current_user)
         todo_service.close_merge_request(merge_request, current_user)
@@ -19,5 +19,16 @@ module MergeRequests
       merge_request
     end
+
+    private
+
+    def create_event(merge_request)
+      # Making sure MergeRequest::Metrics updates are in sync with
+      # Event creation.
+      Event.transaction do
+        close_event = event_service.close_mr(merge_request, current_user)
+        merge_request_metrics_service(merge_request).close(close_event)
+      end
+    end
   end
 end

app/services/merge_requests/post_merge_service.rb

@@ -9,7 +9,7 @@ module MergeRequests
       close_issues(merge_request)
       todo_service.merge_merge_request(merge_request, current_user)
       merge_request.mark_as_merged
-      create_merge_event(merge_request, current_user)
+      create_event(merge_request)
       create_note(merge_request)
       notification_service.merge_mr(merge_request, current_user)
       execute_hooks(merge_request, 'merge')
@@ -34,5 +34,14 @@ module MergeRequests
     def create_merge_event(merge_request, current_user)
       EventCreateService.new.merge_mr(merge_request, current_user)
     end
+
+    def create_event(merge_request)
+      # Making sure MergeRequest::Metrics updates are in sync with
+      # Event creation.
+      Event.transaction do
+        merge_event = create_merge_event(merge_request, current_user)
+        merge_request_metrics_service(merge_request).merge(merge_event)
+      end
+    end
   end
 end

app/services/merge_requests/reopen_service.rb

@@ -4,7 +4,7 @@ module MergeRequests
       return merge_request unless can?(current_user, :update_merge_request, merge_request)
       if merge_request.reopen
-        event_service.reopen_mr(merge_request, current_user)
+        create_event(merge_request)
         create_note(merge_request, 'reopened')
         notification_service.reopen_mr(merge_request, current_user)
         execute_hooks(merge_request, 'reopen')
@@ -16,5 +16,16 @@ module MergeRequests
       merge_request
     end
+
+    private
+
+    def create_event(merge_request)
+      # Making sure MergeRequest::Metrics updates are in sync with
+      # Event creation.
+      Event.transaction do
+        event_service.reopen_mr(merge_request, current_user)
+        merge_request_metrics_service(merge_request).reopen
+      end
+    end
   end
 end

changelogs/unreleased/osw-introduce-merge-request-statistics.yml

+---
+title: Cache merged and closed events data in merge_request_metrics table
+merge_request:
+author:
+type: performance

db/migrate/20171127151038_add_events_related_columns_to_merge_request_metrics.rb

+class AddEventsRelatedColumnsToMergeRequestMetrics < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    change_table :merge_request_metrics do |t|
+      t.references :merged_by, references: :users
+      t.references :latest_closed_by, references: :users
+    end
+
+    add_column :merge_request_metrics, :latest_closed_at, :datetime_with_timezone
+
+    add_concurrent_foreign_key :merge_request_metrics, :users,
+                               column: :merged_by_id,
+                               on_delete: :nullify
+
+    add_concurrent_foreign_key :merge_request_metrics, :users,
+                               column: :latest_closed_by_id,
+                               on_delete: :nullify
+  end
+
+  def down
+    if foreign_keys_for(:merge_request_metrics, :merged_by_id).any?
+      remove_foreign_key :merge_request_metrics, column: :merged_by_id
+    end
+
+    if foreign_keys_for(:merge_request_metrics, :latest_closed_by_id).any?
+      remove_foreign_key :merge_request_metrics, column: :latest_closed_by_id
+    end
+
+    remove_columns :merge_request_metrics,
+      :merged_by_id, :latest_closed_by_id, :latest_closed_at
+  end
+end

db/post_migrate/20171128214150_schedule_populate_merge_request_metrics_with_events_data.rb

+# frozen_string_literal: true
+# rubocop:disable GitlabSecurity/SqlInjection
+
+class SchedulePopulateMergeRequestMetricsWithEventsData < ActiveRecord::Migration
+  DOWNTIME = false
+  BATCH_SIZE = 10_000
+  MIGRATION = 'PopulateMergeRequestMetricsWithEventsData'
+
+  disable_ddl_transaction!
+
+  class MergeRequest < ActiveRecord::Base
+    self.table_name = 'merge_requests'
+
+    include ::EachBatch
+  end
+
+  def up
+    merge_requests = MergeRequest.where("id IN (#{updatable_merge_requests_union_sql})").reorder(:id)
+
+    say 'Scheduling `PopulateMergeRequestMetricsWithEventsData` jobs'
+    # It will update around 4_000_000 records in batches of 10_000 merge
+    # requests (running between 10 minutes) and should take around 66 hours to complete.
+    # Apparently, production PostgreSQL is able to vacuum 10k-20k dead_tuples by
+    # minute, and at maximum, each of these jobs should UPDATE 20k records.
+    #
+    # More information about the updates in `PopulateMergeRequestMetricsWithEventsData` class.
+    #
+    merge_requests.each_batch(of: BATCH_SIZE) do |relation, index|
+      range = relation.pluck('MIN(id)', 'MAX(id)').first
+
+      BackgroundMigrationWorker.perform_in(index * 10.minutes, MIGRATION, range)
+    end
+  end
+
+  def down
+    execute "update merge_request_metrics set latest_closed_at = null"
+    execute "update merge_request_metrics set latest_closed_by_id = null"
+    execute "update merge_request_metrics set merged_by_id = null"
+  end
+
+  private
+
+  # On staging:
+  # Planning time: 0.682 ms
+  # Execution time: 22033.158 ms
+  #
+  def updatable_merge_requests_union_sql
+    metrics_not_exists_clause =
+      'NOT EXISTS (SELECT 1 FROM merge_request_metrics WHERE merge_request_metrics.merge_request_id = merge_requests.id)'
+
+    without_metrics_data = <<-SQL.strip_heredoc
+      merge_request_metrics.merged_by_id IS NULL OR
+      merge_request_metrics.latest_closed_by_id IS NULL OR
+      merge_request_metrics.latest_closed_at IS NULL
+    SQL
+
+    mrs_without_metrics_record = MergeRequest
+      .where(metrics_not_exists_clause)
+      .select(:id)
+
+    mrs_without_events_data = MergeRequest
+      .joins('INNER JOIN merge_request_metrics ON merge_requests.id = merge_request_metrics.merge_request_id')
+      .where(without_metrics_data)
+      .select(:id)
+
+    Gitlab::SQL::Union.new([mrs_without_metrics_record, mrs_without_events_data]).to_sql
+  end
+end

db/schema.rb

@@ -1056,6 +1056,9 @@ ActiveRecord::Schema.define(version: 20171220191323) do
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
     t.integer "pipeline_id"
+    t.integer "merged_by_id"
+    t.integer "latest_closed_by_id"
+    t.datetime_with_timezone "latest_closed_at"
   end
   add_index "merge_request_metrics", ["first_deployed_to_production_at"], name: "index_merge_request_metrics_on_first_deployed_to_production_at", using: :btree
@@ -1995,6 +1998,8 @@ ActiveRecord::Schema.define(version: 20171220191323) do
   add_foreign_key "merge_request_diffs", "merge_requests", name: "fk_8483f3258f", on_delete: :cascade
   add_foreign_key "merge_request_metrics", "ci_pipelines", column: "pipeline_id", on_delete: :cascade
   add_foreign_key "merge_request_metrics", "merge_requests", on_delete: :cascade
+  add_foreign_key "merge_request_metrics", "users", column: "latest_closed_by_id", name: "fk_ae440388cc", on_delete: :nullify
+  add_foreign_key "merge_request_metrics", "users", column: "merged_by_id", name: "fk_7f28d925f3", on_delete: :nullify
   add_foreign_key "merge_requests", "ci_pipelines", column: "head_pipeline_id", name: "fk_fd82eae0b9", on_delete: :nullify
   add_foreign_key "merge_requests", "merge_request_diffs", column: "latest_merge_request_diff_id", name: "fk_06067f5644", on_delete: :nullify
   add_foreign_key "merge_requests", "milestones", name: "fk_6a5165a692", on_delete: :nullify

lib/gitlab/background_migration/populate_merge_request_metrics_with_events_data.rb

+# frozen_string_literal: true
+# rubocop:disable Metrics/LineLength
+# rubocop:disable Metrics/MethodLength
+# rubocop:disable Metrics/ClassLength
+# rubocop:disable Style/Documentation
+
+module Gitlab
+  module BackgroundMigration
+    class PopulateMergeRequestMetricsWithEventsData
+      def perform(min_merge_request_id, max_merge_request_id)
+        insert_metrics_for_range(min_merge_request_id, max_merge_request_id)
+        update_metrics_with_events_data(min_merge_request_id, max_merge_request_id)
+      end
+
+      # Inserts merge_request_metrics records for merge_requests without it for
+      # a given merge request batch.
+      def insert_metrics_for_range(min, max)
+        metrics_not_exists_clause =
+          <<-SQL.strip_heredoc
+            NOT EXISTS (SELECT 1 FROM merge_request_metrics
+                        WHERE merge_request_metrics.merge_request_id = merge_requests.id)
+          SQL
+
+        MergeRequest.where(metrics_not_exists_clause).where(id: min..max).each_batch do |batch|
+          select_sql = batch.select(:id, :created_at, :updated_at).to_sql
+
+          execute("INSERT INTO merge_request_metrics (merge_request_id, created_at, updated_at) #{select_sql}")
+        end
+      end
+
+      def update_metrics_with_events_data(min, max)
+        if Gitlab::Database.postgresql?
+          # Uses WITH syntax in order to update merged and closed events with a single UPDATE.
+          # WITH is not supported by MySQL.
+          update_events_for_range(min, max)
+        else
+          update_merged_events_for_range(min, max)
+          update_closed_events_for_range(min, max)
+        end
+      end
+
+      private
+
+      # Updates merge_request_metrics latest_closed_at, latest_closed_by_id and merged_by_id
+      # based on the latest event records on events table for a given merge request batch.
+      def update_events_for_range(min, max)
+        sql = <<-SQL.strip_heredoc
+          WITH events_for_update AS (
+            SELECT DISTINCT ON (target_id, action) target_id, action, author_id, updated_at
+             FROM events
+            WHERE target_id BETWEEN #{min} AND #{max}
+              AND target_type = 'MergeRequest'
+              AND action IN (#{Event::CLOSED},#{Event::MERGED})
+            ORDER BY target_id, action, id DESC
+          )
+          UPDATE merge_request_metrics met
+            SET latest_closed_at = latest_closed.updated_at,
+                latest_closed_by_id = latest_closed.author_id,
+                merged_by_id = latest_merged.author_id
+           FROM (SELECT * FROM events_for_update WHERE action = #{Event::CLOSED}) AS latest_closed
+           FULL OUTER JOIN
+                (SELECT * FROM events_for_update WHERE action = #{Event::MERGED}) AS latest_merged
+          USING (target_id)
+          WHERE target_id = merge_request_id;
+        SQL
+
+        execute(sql)
+      end
+
+      # Updates merge_request_metrics latest_closed_at, latest_closed_by_id based on the latest closed
+      # records on events table for a given merge request batch.
+      def update_closed_events_for_range(min, max)
+        sql =
+          <<-SQL.strip_heredoc
+            UPDATE merge_request_metrics metrics,
+              (#{select_events(min, max, Event::CLOSED)}) closed_events
+            SET metrics.latest_closed_by_id = closed_events.author_id,
+                metrics.latest_closed_at = closed_events.updated_at #{where_matches_closed_events};
+          SQL
+
+        execute(sql)
+      end
+
+      # Updates merge_request_metrics merged_by_id based on the latest merged
+      # records on events table for a given merge request batch.
+      def update_merged_events_for_range(min, max)
+        sql =
+          <<-SQL.strip_heredoc
+            UPDATE merge_request_metrics metrics,
+              (#{select_events(min, max, Event::MERGED)}) merged_events
+            SET metrics.merged_by_id = merged_events.author_id #{where_matches_merged_events};
+          SQL
+
+        execute(sql)
+      end
+
+      def execute(sql)
+        @connection ||= ActiveRecord::Base.connection
+        @connection.execute(sql)
+      end
+
+      def select_events(min, max, action)
+        select_max_event_id = <<-SQL.strip_heredoc
+          SELECT max(id)
+          FROM events
+          WHERE action = #{action}
+          AND target_type = 'MergeRequest'
+          AND target_id BETWEEN #{min} AND #{max}
+          GROUP BY target_id
+        SQL
+
+        <<-SQL.strip_heredoc
+          SELECT author_id, updated_at, target_id
+          FROM events
+          WHERE id IN(#{select_max_event_id})
+        SQL
+      end
+
+      def where_matches_closed_events
+        <<-SQL.strip_heredoc
+          WHERE metrics.merge_request_id = closed_events.target_id
+          AND metrics.latest_closed_at IS NULL
+          AND metrics.latest_closed_by_id IS NULL
+        SQL
+      end
+
+      def where_matches_merged_events
+        <<-SQL.strip_heredoc
+          WHERE metrics.merge_request_id = merged_events.target_id
+          AND metrics.merged_by_id IS NULL
+        SQL
+      end
+    end
+  end
+end