Skip duration filter when it is unnecessarynew-cycle-analytics-query-backend

- Introduce DurationFilter class that decides whether we need
  additional filtering to avoid negative durations
- Improve a queries after checking the execution plan

10 files changed, 116 insertions, 14 deletions

diff --git a/db/migrate/20190905074652_index_timestamp_columns_for_issue_metrics.rb b/db/migrate/20190905074652_index_timestamp_columns_for_issue_metrics.rb
new file mode 100644
index 00000000000..e468b2decd9
--- /dev/null
+++ b/db/migrate/20190905074652_index_timestamp_columns_for_issue_metrics.rb
@@ -0,0 +1,29 @@
+# frozen_string_literal: true
+
+class IndexTimestampColumnsForIssueMetrics < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index(*index_arguments)
+  end
+
+  def down
+    remove_concurrent_index(*index_arguments)
+  end
+
+  private
+
+  def index_arguments
+    [
+      :issue_metrics,
+      [:issue_id, :first_mentioned_in_commit_at, :first_associated_with_milestone_at, :first_added_to_board_at],
+      {
+        name: 'index_issue_metrics_on_issue_id_and_timestamps'
+      }
+    ]
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 61f7787f192..6e293ee257a 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -1736,6 +1736,7 @@ ActiveRecord::Schema.define(version: 2019_09_05_223900) do
     t.datetime "first_added_to_board_at"
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
+    t.index ["issue_id", "first_mentioned_in_commit_at", "first_associated_with_milestone_at", "first_added_to_board_at"], name: "index_issue_metrics_on_issue_id_and_timestamps"
     t.index ["issue_id"], name: "index_issue_metrics"
   end
 
diff --git a/lib/gitlab/analytics/cycle_analytics/base_query_builder.rb b/lib/gitlab/analytics/cycle_analytics/base_query_builder.rb
index 0afe2dbd021..163f7e2ccef 100644
--- a/lib/gitlab/analytics/cycle_analytics/base_query_builder.rb
+++ b/lib/gitlab/analytics/cycle_analytics/base_query_builder.rb
@@ -5,7 +5,6 @@ module Gitlab
     module CycleAnalytics
       class BaseQueryBuilder
         include Gitlab::CycleAnalytics::MetricsTables
-        include StageQueryHelpers
 
         delegate :subject_model, to: :stage
 
@@ -14,6 +13,7 @@ module Gitlab
         def initialize(stage:, params: {})
           @stage = stage
           @params = params
+          @duration_filter = DurationFilter.new(stage: stage)
         end
 
         def run
@@ -22,16 +22,12 @@ module Gitlab
           query = filter_by_time_range(query)
           query = stage.start_event.apply_query_customization(query)
           query = stage.end_event.apply_query_customization(query)
-          exclude_negative_durations(query)
+          duration_filter.apply(query)
         end
 
         private
 
-        attr_reader :stage, :params
-
-        def exclude_negative_durations(query)
-          query.where(duration.gt(zero_interval))
-        end
+        attr_reader :stage, :params, :duration_filter
 
         def filter_by_parent_model(query)
           parent_class = stage.parent.class
diff --git a/lib/gitlab/analytics/cycle_analytics/duration_filter.rb b/lib/gitlab/analytics/cycle_analytics/duration_filter.rb
new file mode 100644
index 00000000000..c3a2ede4e57
--- /dev/null
+++ b/lib/gitlab/analytics/cycle_analytics/duration_filter.rb
@@ -0,0 +1,54 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module Analytics
+    module CycleAnalytics
+      # This class ensures that negative durations won't be returned by the query. Sometimes checking for negative duration is unnecessary, in that case the duration check won't be executed.
+      #
+      # Example: issues.closed_at - issues.created_at
+      # Check is not needed because issues.created_at will be always earlier than closed_at.
+      class DurationFilter
+        include StageQueryHelpers
+
+        def initialize(stage:)
+          @stage = stage
+        end
+
+        # rubocop: disable CodeReuse/ActiveRecord
+        def apply(query)
+          skip_duration_check? ? query : query.where(stage.end_event.timestamp_projection.gteq(stage.start_event.timestamp_projection))
+        end
+        # rubocop: enable CodeReuse/ActiveRecord
+
+        private
+
+        attr_reader :stage
+
+        def skip_duration_check?
+          starts_with_issue_creation? ||
+            starts_with_mr_creation? ||
+            mr_merged_at_with_deployment? ||
+            mr_build_started_and_finished?
+        end
+
+        def starts_with_issue_creation?
+          stage.start_event.is_a?(StageEvents::IssueCreated)
+        end
+
+        def starts_with_mr_creation?
+          stage.start_event.is_a?(StageEvents::MergeRequestCreated)
+        end
+
+        def mr_merged_at_with_deployment?
+          stage.start_event.is_a?(StageEvents::MergeRequestMerged) &&
+            stage.end_event.is_a?(StageEvents::MergeRequestFirstDeployedToProduction)
+        end
+
+        def mr_build_started_and_finished?
+          stage.start_event.is_a?(StageEvents::MergeRequestLastBuildStarted) &&
+            stage.end_event.is_a?(StageEvents::MergeRequestLastBuildFinished)
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/analytics/cycle_analytics/records_fetcher.rb b/lib/gitlab/analytics/cycle_analytics/records_fetcher.rb
index 2d4377099aa..9e2a2d57150 100644
--- a/lib/gitlab/analytics/cycle_analytics/records_fetcher.rb
+++ b/lib/gitlab/analytics/cycle_analytics/records_fetcher.rb
@@ -101,9 +101,9 @@ module Gitlab
             .joins(ci_build_join)
             .select(build_table[:id], round_duration_to_seconds.as('total_time'))
 
-          result = execute_query(q).to_a
+          results = execute_query(q).to_a
 
-          Gitlab::CycleAnalytics::Updater.update!(result, from: 'id', to: 'build', klass: ::Ci::Build)
+          Gitlab::CycleAnalytics::Updater.update!(results, from: 'id', to: 'build', klass: ::Ci::Build.includes({ project: [:namespace], user: [], pipeline: [] }))
         end
 
         def ordered_and_limited_query
diff --git a/lib/gitlab/analytics/cycle_analytics/stage_events/issue_stage_end.rb b/lib/gitlab/analytics/cycle_analytics/stage_events/issue_stage_end.rb
index f09c98e537c..77e4092b9ab 100644
--- a/lib/gitlab/analytics/cycle_analytics/stage_events/issue_stage_end.rb
+++ b/lib/gitlab/analytics/cycle_analytics/stage_events/issue_stage_end.rb
@@ -26,7 +26,7 @@ module Gitlab
 
           # rubocop: disable CodeReuse/ActiveRecord
           def apply_query_customization(query)
-            query.joins(:metrics)
+            query.joins(:metrics).where(issue_metrics_table[:first_added_to_board_at].not_eq(nil).or(issue_metrics_table[:first_associated_with_milestone_at].not_eq(nil)))
           end
           # rubocop: enable CodeReuse/ActiveRecord
         end
diff --git a/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_first_deployed_to_production.rb b/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_first_deployed_to_production.rb
index 56a331c028c..3d7482eaaf0 100644
--- a/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_first_deployed_to_production.rb
+++ b/lib/gitlab/analytics/cycle_analytics/stage_events/merge_request_first_deployed_to_production.rb
@@ -23,7 +23,7 @@ module Gitlab
 
           # rubocop: disable CodeReuse/ActiveRecord
           def apply_query_customization(query)
-            query.joins(:metrics)
+            query.joins(:metrics).where(timestamp_projection.gteq(mr_table[:created_at]))
           end
           # rubocop: enable CodeReuse/ActiveRecord
         end
diff --git a/lib/gitlab/analytics/cycle_analytics/stage_events/production_stage_end.rb b/lib/gitlab/analytics/cycle_analytics/stage_events/production_stage_end.rb
index 2fd71216e11..607371a32e8 100644
--- a/lib/gitlab/analytics/cycle_analytics/stage_events/production_stage_end.rb
+++ b/lib/gitlab/analytics/cycle_analytics/stage_events/production_stage_end.rb
@@ -23,7 +23,7 @@ module Gitlab
 
           # rubocop: disable CodeReuse/ActiveRecord
           def apply_query_customization(query)
-            query.joins(merge_requests_closing_issues: { merge_request: [:metrics] })
+            query.joins(merge_requests_closing_issues: { merge_request: [:metrics] }).where(mr_metrics_table[:first_deployed_to_production_at].gteq(mr_table[:created_at]))
           end
           # rubocop: enable CodeReuse/ActiveRecord
         end
diff --git a/spec/lib/gitlab/analytics/cycle_analytics/duration_filter_spec.rb b/spec/lib/gitlab/analytics/cycle_analytics/duration_filter_spec.rb
new file mode 100644
index 00000000000..33309181db4
--- /dev/null
+++ b/spec/lib/gitlab/analytics/cycle_analytics/duration_filter_spec.rb
@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Analytics::CycleAnalytics::DurationFilter do
+  let(:stage_params) { {} }
+  let(:stage) { Analytics::CycleAnalytics::ProjectStage.new(stage_params) }
+  subject { described_class.new(stage: stage) }
+
+  describe 'when duration filtering is skipped' do
+    %I[issue test review staging production].each do |stage_name|
+      it "for '#{stage_name}' stage" do
+        stage_params.merge!(Gitlab::Analytics::CycleAnalytics::DefaultStages.public_send("params_for_#{stage_name}_stage"))
+
+        input_query = stage.subject_model.all
+        output_query = subject.apply(input_query)
+
+        expect(input_query).to eq(output_query)
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/analytics/cycle_analytics/records_fetcher_spec.rb b/spec/lib/gitlab/analytics/cycle_analytics/records_fetcher_spec.rb
index 8530b50be0a..0e34bf1ffc2 100644
--- a/spec/lib/gitlab/analytics/cycle_analytics/records_fetcher_spec.rb
+++ b/spec/lib/gitlab/analytics/cycle_analytics/records_fetcher_spec.rb
@@ -78,8 +78,8 @@ describe Gitlab::Analytics::CycleAnalytics::RecordsFetcher do
     end
 
     describe 'special case' do
-      let(:mr1) { create(:merge_request, source_project: project, allow_broken: true) }
-      let(:mr2) { create(:merge_request, source_project: project, allow_broken: true) }
+      let(:mr1) { create(:merge_request, source_project: project, allow_broken: true, created_at: 20.days.ago) }
+      let(:mr2) { create(:merge_request, source_project: project, allow_broken: true, created_at: 20.days.ago) }
       let(:ci_build1) { create(:ci_build) }
       let(:ci_build2) { create(:ci_build) }
       let(:default_stages) { Gitlab::Analytics::CycleAnalytics::DefaultStages }