Merge branch 'osw-introduce-merge-request-statistics' into 'master'

Improve closed/merged events queries performance on Projects::MergeRequestsController#show.json

See merge request gitlab-org/gitlab-ce!15642

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,42 @@ export default class MergeRequestStore {
     return this.state === stateKey.nothingToMerge;
   }
 
-  static getEventObject(event) {
+  static buildMetrics(metrics) {
+    if (!metrics) {
+      return {};
+    }
+
     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,27 @@ 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.
+    return unless merge_request.merged? || merge_request.closed?
+    return merge_request.metrics if merge_request.merged? && merge_request.metrics&.merged_by_id
+    return merge_request.metrics if merge_request.closed? && merge_request.metrics&.latest_closed_by_id
+
+    build_metrics_from_events(merge_request)
+  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

spec/fixtures/api/schemas/entities/merge_request_metrics.json

+{
+  "type": "object",
+  "required": ["closed_at", "merged_at", "closed_by", "merged_by"],
+  "properties" : {
+    "closed_at": { "type": ["datetime", "null"] },
+    "merged_at": { "type": ["datetime", "null"] },
+    "closed_by": {
+      "oneOf": [
+        { "type": "null" },
+        { "$ref": "user.json" }
+      ]
+    },
+    "merged_by": {
+      "oneOf": [
+        { "type": "null" },
+        { "$ref": "user.json" }
+      ]
+    }
+  },
+  "additionalProperties": false
+}

spec/fixtures/api/schemas/entities/merge_request_widget.json

@@ -31,8 +31,12 @@
     "source_project_id": { "type": "integer" },
     "target_branch": { "type": "string" },
     "target_project_id": { "type": "integer" },
-    "merge_event": { "type": ["object", "null"] },
-    "closed_event": { "type": ["object", "null"] },
+    "metrics": {
+      "oneOf": [
+        { "type": "null" },
+        { "$ref": "merge_request_metrics.json" }
+      ]
+    },
     "author": { "type": ["object", "null"] },
     "merge_user": { "type": ["object", "null"] },
     "diff_head_sha": { "type": ["string", "null"] },

spec/fixtures/api/schemas/entities/user.json

+{
+  "type": "object",
+  "required": [
+    "id",
+    "state",
+    "avatar_url",
+    "web_url",
+    "path"
+  ],
+  "properties": {
+    "id": { "type": "integer" },
+    "state": { "type": "string" },
+    "avatar_url": { "type": "string" },
+    "web_url": { "type": "string" },
+    "path": { "type": "string" }
+  }
+}

spec/javascripts/vue_mr_widget/components/states/mr_widget_closed_spec.js

@@ -4,13 +4,16 @@ import closedComponent from '~/vue_merge_request_widget/components/states/mr_wid
 const mr = {
   targetBranch: 'good-branch',
   targetBranchPath: '/good-branch',
-  closedEvent: {
-    author: {
+  metrics: {
+    mergedBy: {},
+    mergedAt: 'mergedUpdatedAt',
+    closedBy: {
       name: 'Fatih Acet',
       username: 'fatihacet',
     },
-    updatedAt: 'closedEventUpdatedAt',
-    formattedUpdatedAt: '',
+    closedAt: 'closedEventUpdatedAt',
+    readableMergedAt: '',
+    readableClosedAt: '',
   },
   updatedAt: 'mrUpdatedAt',
   closedAt: '1 day ago',
@@ -56,7 +59,7 @@ describe('MRWidgetClosed', () => {
 
     it('should have correct elements', () => {
       expect(el.querySelector('h4').textContent).toContain('Closed by');
-      expect(el.querySelector('h4').textContent).toContain(mr.closedEvent.author.name);
+      expect(el.querySelector('h4').textContent).toContain(mr.metrics.closedBy.name);
       expect(el.textContent).toContain('The changes were not merged into');
       expect(el.querySelector('.label-branch').getAttribute('href')).toEqual(mr.targetBranchPath);
       expect(el.querySelector('.label-branch').textContent).toContain(mr.targetBranch);

spec/javascripts/vue_mr_widget/components/states/mr_widget_merged_spec.js

@@ -14,10 +14,13 @@ const createComponent = () => {
     canRevertInCurrentMR: true,
     canRemoveSourceBranch: true,
     sourceBranchRemoved: true,
-    mergedEvent: {
-      author: {},
-      updatedAt: 'mergedUpdatedAt',
-      formattedUpdatedAt: '',
+    metrics: {
+      mergedBy: {},
+      mergedAt: 'mergedUpdatedAt',
+      readableMergedAt: '',
+      closedBy: {},
+      closedAt: 'mergedUpdatedAt',
+      readableClosedAt: '',
     },
     updatedAt: 'mrUpdatedAt',
     targetBranch,

spec/javascripts/vue_mr_widget/mock_data.js

@@ -33,8 +33,8 @@ export default {
   "source_project_id": 19,
   "target_branch": "master",
   "target_project_id": 19,
-  "merge_event": {
-    "author": {
+  "metrics": {
+    "merged_by": {
       "name": "Administrator",
       "username": "root",
       "id": 1,
@@ -42,9 +42,10 @@ export default {
       "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon",
       "web_url": "http://localhost:3000/root"
     },
-    "updated_at": "2017-04-07T15:39:25.696Z"
+    "merged_at": "2017-04-07T15:39:25.696Z",
+    "closed_by": null,
+    "closed_at": null
   },
-  "closed_event": null,
   "author": {
     "name": "Administrator",
     "username": "root",

spec/lib/gitlab/background_migration/populate_merge_request_metrics_with_events_data_spec.rb

+require 'rails_helper'
+
+describe Gitlab::BackgroundMigration::PopulateMergeRequestMetricsWithEventsData, :migration, schema: 20171128214150 do
+  describe '#perform' do
+    let(:mr_with_event) { create(:merge_request) }
+    let!(:merged_event) { create(:event, :merged, target: mr_with_event) }
+    let!(:closed_event) { create(:event, :closed, target: mr_with_event) }
+
+    before do
+      # Make sure no metrics are created and kept through after_* callbacks.
+      mr_with_event.metrics.destroy!
+    end
+
+    it 'inserts metrics and updates closed and merged events' do
+      subject.perform(mr_with_event.id, mr_with_event.id)
+
+      mr_with_event.reload
+
+      expect(mr_with_event.metrics).to have_attributes(latest_closed_by_id: closed_event.author_id,
+                                                       merged_by_id: merged_event.author_id)
+      expect(mr_with_event.metrics.latest_closed_at.to_s).to eq(closed_event.updated_at.to_s)
+    end
+  end
+
+  describe '#insert_metrics_for_range' do
+    let!(:mrs_without_metrics) { create_list(:merge_request, 3) }
+    let!(:mrs_with_metrics) { create_list(:merge_request, 2) }
+
+    before do
+      # Make sure no metrics are created and kept through after_* callbacks.
+      mrs_without_metrics.each { |m| m.metrics.destroy! }
+    end
+
+    it 'inserts merge_request_metrics for merge_requests without one' do
+      expect { subject.insert_metrics_for_range(MergeRequest.first.id, MergeRequest.last.id) }
+        .to change(MergeRequest::Metrics, :count).from(2).to(5)
+
+      mrs_without_metrics.each do |mr_without_metrics|
+        expect(mr_without_metrics.reload.metrics).to be_present
+      end
+    end
+
+    it 'does not inserts merge_request_metrics for MRs out of given range' do
+      expect { subject.insert_metrics_for_range(mrs_with_metrics.first.id, mrs_with_metrics.last.id) }
+        .not_to change(MergeRequest::Metrics, :count).from(2)
+    end
+  end
+
+  describe '#update_metrics_with_events_data' do
+    context 'closed events data update' do
+      let(:users) { create_list(:user, 3) }
+      let(:mrs_with_event) { create_list(:merge_request, 3) }
+
+      before do
+        create_list(:event, 2, :closed, author: users.first, target: mrs_with_event.first)
+        create_list(:event, 3, :closed, author: users.second, target: mrs_with_event.second)
+        create(:event, :closed, author: users.third, target: mrs_with_event.third)
+      end
+
+      it 'migrates multiple MR metrics with closed event data' do
+        mr_without_event = create(:merge_request)
+        create(:event, :merged)
+
+        subject.update_metrics_with_events_data(mrs_with_event.first.id, mrs_with_event.last.id)
+
+        mrs_with_event.each do |mr_with_event|
+          latest_event = Event.where(action: 3, target: mr_with_event).last
+
+          mr_with_event.metrics.reload
+
+          expect(mr_with_event.metrics.latest_closed_by).to eq(latest_event.author)
+          expect(mr_with_event.metrics.latest_closed_at.to_s).to eq(latest_event.updated_at.to_s)
+        end
+
+        expect(mr_without_event.metrics.reload).to have_attributes(latest_closed_by_id: nil,
+                                                                   latest_closed_at: nil)
+      end
+
+      it 'does not updates metrics out of given range' do
+        out_of_range_mr = create(:merge_request)
+        create(:event, :closed, author: users.last, target: out_of_range_mr)
+
+        expect { subject.perform(mrs_with_event.first.id, mrs_with_event.second.id) }
+          .not_to change { out_of_range_mr.metrics.reload.merged_by }
+          .from(nil)
+      end
+    end
+
+    context 'merged events data update' do
+      let(:users) { create_list(:user, 3) }
+      let(:mrs_with_event) { create_list(:merge_request, 3) }
+
+      before do
+        create_list(:event, 2, :merged, author: users.first, target: mrs_with_event.first)
+        create_list(:event, 3, :merged, author: users.second, target: mrs_with_event.second)
+        create(:event, :merged, author: users.third, target: mrs_with_event.third)
+      end
+
+      it 'migrates multiple MR metrics with merged event data' do
+        mr_without_event = create(:merge_request)
+        create(:event, :merged)
+
+        subject.update_metrics_with_events_data(mrs_with_event.first.id, mrs_with_event.last.id)
+
+        mrs_with_event.each do |mr_with_event|
+          latest_event = Event.where(action: Event::MERGED, target: mr_with_event).last
+
+          expect(mr_with_event.metrics.reload.merged_by).to eq(latest_event.author)
+        end
+
+        expect(mr_without_event.metrics.reload).to have_attributes(merged_by_id: nil)
+      end
+
+      it 'does not updates metrics out of given range' do
+        out_of_range_mr = create(:merge_request)
+        create(:event, :merged, author: users.last, target: out_of_range_mr)
+
+        expect { subject.perform(mrs_with_event.first.id, mrs_with_event.second.id) }
+          .not_to change { out_of_range_mr.metrics.reload.merged_by }
+          .from(nil)
+      end
+    end
+  end
+end

spec/migrations/schedule_populate_merge_request_metrics_with_events_data_spec.rb

+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20171128214150_schedule_populate_merge_request_metrics_with_events_data.rb')
+
+describe SchedulePopulateMergeRequestMetricsWithEventsData, :migration, :sidekiq do
+  let!(:mrs) { create_list(:merge_request, 3) }
+
+  it 'correctly schedules background migrations' do
+    stub_const("#{described_class.name}::BATCH_SIZE", 2)
+
+    Sidekiq::Testing.fake! do
+      Timecop.freeze do
+        migrate!
+
+        expect(described_class::MIGRATION)
+          .to be_scheduled_migration(10.minutes, mrs.first.id, mrs.second.id)
+
+        expect(described_class::MIGRATION)
+          .to be_scheduled_migration(20.minutes, mrs.third.id, mrs.third.id)
+
+        expect(BackgroundMigrationWorker.jobs.size).to eq(2)
+      end
+    end
+  end
+end

spec/models/issue_spec.rb

@@ -23,6 +23,32 @@ describe Issue do
     it { is_expected.to have_db_index(:deleted_at) }
   end
 
+  describe 'callbacks' do
+    describe '#ensure_metrics' do
+      it 'creates metrics after saving' do
+        issue = create(:issue)
+
+        expect(issue.metrics).to be_persisted
+        expect(Issue::Metrics.count).to eq(1)
+      end
+
+      it 'does not create duplicate metrics for an issue' do
+        issue = create(:issue)
+
+        issue.close!
+
+        expect(issue.metrics).to be_persisted
+        expect(Issue::Metrics.count).to eq(1)
+      end
+
+      it 'records current metrics' do
+        expect_any_instance_of(Issue::Metrics).to receive(:record!)
+
+        create(:issue)
+      end
+    end
+  end
+
   describe '#order_by_position_and_priority' do
     let(:project) { create :project }
     let(:p1) { create(:label, title: 'P1', project: project, priority: 1) }

spec/models/merge_request/metrics_spec.rb

 require 'spec_helper'
 
 describe MergeRequest::Metrics do
-  subject { create(:merge_request) }
+  subject { described_class.new }
 
-  describe "when recording the default set of metrics on merge request save" do
-    it "records the merge time" do
-      time = Time.now
-      Timecop.freeze(time) { subject.mark_as_merged }
-      metrics = subject.metrics
-
-      expect(metrics).to be_present
-      expect(metrics.merged_at).to be_like_time(time)
-    end
+  describe 'associations' do
+    it { is_expected.to belong_to(:merge_request) }
+    it { is_expected.to belong_to(:latest_closed_by).class_name('User') }
+    it { is_expected.to belong_to(:merged_by).class_name('User') }
   end
 end

spec/models/merge_request_spec.rb

@@ -65,6 +65,25 @@ describe MergeRequest do
     end
   end
 
+  describe 'callbacks' do
+    describe '#ensure_merge_request_metrics' do
+      it 'creates metrics after saving' do
+        merge_request = create(:merge_request)
+
+        expect(merge_request.metrics).to be_persisted
+        expect(MergeRequest::Metrics.count).to eq(1)
+      end
+
+      it 'does not duplicate metrics for a merge request' do
+        merge_request = create(:merge_request)
+
+        merge_request.mark_as_merged!
+
+        expect(MergeRequest::Metrics.count).to eq(1)
+      end
+    end
+  end
+
   describe 'respond to' do
     it { is_expected.to respond_to(:unchecked?) }
     it { is_expected.to respond_to(:can_be_merged?) }

spec/requests/api/notes_spec.rb

@@ -464,7 +464,7 @@ describe API::Notes do
 
   describe "POST /projects/:id/noteable/:noteable_id/notes to test observer on create" do
     it "creates an activity event when an issue note is created" do
-      expect(Event).to receive(:create)
+      expect(Event).to receive(:create!)
 
       post api("/projects/#{project.id}/issues/#{issue.iid}/notes", user), body: 'hi!'
     end

spec/requests/api/project_milestones_spec.rb

@@ -16,7 +16,7 @@ describe API::ProjectMilestones do
 
   describe 'PUT /projects/:id/milestones/:milestone_id to test observer on close' do
     it 'creates an activity event when an milestone is closed' do
-      expect(Event).to receive(:create)
+      expect(Event).to receive(:create!)
 
       put api("/projects/#{project.id}/milestones/#{milestone.id}", user),
           state_event: 'close'

spec/requests/api/v3/milestones_spec.rb

@@ -161,7 +161,7 @@ describe API::V3::Milestones do
 
   describe 'PUT /projects/:id/milestones/:milestone_id to test observer on close' do
     it 'creates an activity event when an milestone is closed' do
-      expect(Event).to receive(:create)
+      expect(Event).to receive(:create!)
 
       put v3_api("/projects/#{project.id}/milestones/#{milestone.id}", user),
           state_event: 'close'

spec/requests/api/v3/notes_spec.rb

@@ -302,7 +302,7 @@ describe API::V3::Notes do
 
   describe "POST /projects/:id/noteable/:noteable_id/notes to test observer on create" do
     it "creates an activity event when an issue note is created" do
-      expect(Event).to receive(:create)
+      expect(Event).to receive(:create!)
 
       post v3_api("/projects/#{project.id}/issues/#{issue.id}/notes", user), body: 'hi!'
     end

spec/serializers/event_entity_spec.rb

-require 'spec_helper'
-
-describe EventEntity do
-  subject { described_class.represent(create(:event)).as_json }
-
-  it 'exposes author' do
-    expect(subject).to include(:author)
-  end
-
-  it 'exposes core elements of event' do
-    expect(subject).to include(:updated_at)
-  end
-end

spec/serializers/merge_request_widget_entity_spec.rb

@@ -35,6 +35,81 @@ describe MergeRequestWidgetEntity do
     end
   end
 
+  describe 'metrics' do
+    context 'when metrics record exists with merged data' do
+      before do
+        resource.mark_as_merged!
+        resource.metrics.update!(merged_by: user)
+      end
+
+      it 'matches merge request metrics schema' do
+        expect(subject[:metrics].with_indifferent_access)
+          .to match_schema('entities/merge_request_metrics')
+      end
+
+      it 'returns values from metrics record' do
+        expect(subject.dig(:metrics, :merged_by, :id))
+          .to eq(resource.metrics.merged_by_id)
+      end
+    end
+
+    context 'when metrics record exists with closed data' do
+      before do
+        resource.close!
+        resource.metrics.update!(latest_closed_by: user)
+      end
+
+      it 'matches merge request metrics schema' do
+        expect(subject[:metrics].with_indifferent_access)
+          .to match_schema('entities/merge_request_metrics')
+      end
+
+      it 'returns values from metrics record' do
+        expect(subject.dig(:metrics, :closed_by, :id))
+          .to eq(resource.metrics.latest_closed_by_id)
+      end
+    end
+
+    context 'when metrics does not exists' do
+      before do
+        resource.mark_as_merged!
+        resource.metrics.destroy!
+        resource.reload
+      end
+
+      context 'when events exists' do
+        let!(:closed_event) { create(:event, :closed, project: project, target: resource) }
+        let!(:merge_event) { create(:event, :merged, project: project, target: resource) }
+
+        it 'matches merge request metrics schema' do
+          expect(subject[:metrics].with_indifferent_access)
+            .to match_schema('entities/merge_request_metrics')
+        end
+
+        it 'returns values from events record' do
+          expect(subject.dig(:metrics, :merged_by, :id))
+            .to eq(merge_event.author_id)
+
+          expect(subject.dig(:metrics, :closed_by, :id))
+            .to eq(closed_event.author_id)
+
+          expect(subject.dig(:metrics, :merged_at).to_s)
+            .to eq(merge_event.updated_at.to_s)
+
+          expect(subject.dig(:metrics, :closed_at).to_s)
+            .to eq(closed_event.updated_at.to_s)
+        end
+      end
+
+      context 'when events does not exists' do
+        it 'matches merge request metrics schema' do
+          expect(subject[:metrics].with_indifferent_access)
+            .to match_schema('entities/merge_request_metrics')
+        end
+      end
+    end
+  end
+
   it 'has email_patches_path' do
     expect(subject[:email_patches_path])
       .to eq("/#{resource.project.full_path}/merge_requests/#{resource.iid}.patch")

spec/services/create_deployment_service_spec.rb

@@ -266,7 +266,7 @@ describe CreateDeploymentService do
 
     context "while updating the 'first_deployed_to_production_at' time" do
       before do
-        merge_request.mark_as_merged
+        merge_request.metrics.update!(merged_at: Time.now)
       end
 
       context "for merge requests merged before the current deploy" do

spec/services/merge_requests/close_service_spec.rb

@@ -52,6 +52,19 @@ describe MergeRequests::CloseService do
       end
     end
 
+    it 'updates metrics' do
+      metrics = merge_request.metrics
+      metrics_service = double(MergeRequestMetricsService)
+      allow(MergeRequestMetricsService)
+        .to receive(:new)
+        .with(metrics)
+        .and_return(metrics_service)
+
+      expect(metrics_service).to receive(:close)
+
+      described_class.new(project, user, {}).execute(merge_request)
+    end
+
     it 'refreshes the number of open merge requests for a valid MR', :use_clean_rails_memory_store_caching do
       service = described_class.new(project, user, {})
 

spec/services/merge_requests/post_merge_service_spec.rb

@@ -22,5 +22,18 @@ describe MergeRequests::PostMergeService do
       expect { service.execute(merge_request) }
         .to change { project.open_merge_requests_count }.from(1).to(0)
     end
+
+    it 'updates metrics' do
+      metrics = merge_request.metrics
+      metrics_service = double(MergeRequestMetricsService)
+      allow(MergeRequestMetricsService)
+        .to receive(:new)
+        .with(metrics)
+        .and_return(metrics_service)
+
+      expect(metrics_service).to receive(:merge)
+
+      described_class.new(project, user, {}).execute(merge_request)
+    end
   end
 end

spec/services/merge_requests/reopen_service_spec.rb

@@ -47,6 +47,19 @@ describe MergeRequests::ReopenService do
       end
     end
 
+    it 'updates metrics' do
+      metrics = merge_request.metrics
+      service = double(MergeRequestMetricsService)
+      allow(MergeRequestMetricsService)
+        .to receive(:new)
+        .with(metrics)
+        .and_return(service)
+
+      expect(service).to receive(:reopen)
+
+      described_class.new(project, user, {}).execute(merge_request)
+    end
+
     it 'refreshes the number of open merge requests for a valid MR' do
       service = described_class.new(project, user, {})
 

spec/services/update_merge_request_metrics_service_spec.rb

+require 'rails_helper'
+
+describe MergeRequestMetricsService do
+  let(:metrics) { create(:merge_request).metrics }
+
+  describe '#merge' do
+    it 'updates metrics' do
+      user = create(:user)
+      service = described_class.new(metrics)
+      event = double(Event, author_id: user.id, created_at: Time.now)
+
+      service.merge(event)
+
+      expect(metrics.merged_by).to eq(user)
+      expect(metrics.merged_at).to eq(event.created_at)
+    end
+  end
+
+  describe '#close' do
+    it 'updates metrics' do
+      user = create(:user)
+      service = described_class.new(metrics)
+      event = double(Event, author_id: user.id, created_at: Time.now)
+
+      service.close(event)
+
+      expect(metrics.latest_closed_by).to eq(user)
+      expect(metrics.latest_closed_at).to eq(event.created_at)
+    end
+  end
+
+  describe '#reopen' do
+    it 'updates metrics' do
+      service = described_class.new(metrics)
+
+      service.reopen
+
+      expect(metrics.latest_closed_by).to be_nil
+      expect(metrics.latest_closed_at).to be_nil
+    end
+  end
+end