Merge branch '344803-toggle-between-old-backend-and-aggregated-vsa' into 'master'

Add add toggle to enable/disable aggregated VSA

See merge request gitlab-org/gitlab!82389

app/models/analytics/cycle_analytics/aggregation.rb

@@ -14,7 +14,10 @@ class Analytics::CycleAnalytics::Aggregation < ApplicationRecord
     return unless enabled
     return if last_incremental_run_at.nil?
 
-    estimation = (last_incremental_run_at - earliest_last_run_at) + average_aggregation_duration
+    estimation = duration_until_the_next_aggregation_job +
+      average_aggregation_duration +
+      (last_incremental_run_at - earliest_last_run_at)
+
     estimation < 1 ? nil : estimation.from_now
   end
 
@@ -29,6 +32,11 @@ class Analytics::CycleAnalytics::Aggregation < ApplicationRecord
 
   private
 
+  # The aggregation job is scheduled every 10 minutes: */10 * * * *
+  def duration_until_the_next_aggregation_job
+    (10 - (DateTime.current.minute % 10)).minutes.seconds
+  end
+
   def average_aggregation_duration
     return 0.seconds if incremental_runtimes_in_seconds.empty?
 

ee/app/controllers/groups/analytics/cycle_analytics_controller.rb

@@ -7,7 +7,7 @@ class Groups::Analytics::CycleAnalyticsController < Groups::Analytics::Applicati
 
   increment_usage_counter Gitlab::UsageDataCounters::CycleAnalyticsCounter, :views, only: :show
 
-  before_action :load_group, only: :show
+  before_action :load_group, only: %I[show use_aggregated_backend]
   before_action :load_project, only: :show
   before_action :load_value_stream, only: :show
   before_action :request_params, only: :show
@@ -25,6 +25,13 @@ class Groups::Analytics::CycleAnalyticsController < Groups::Analytics::Applicati
     flash.now[:notice] = s_("ValueStreamAnalytics|Items in Value Stream Analytics are currently filtered by their creation time. There is an %{epic_link_start}epic%{epic_link_end} that will change the Value Stream Analytics date filter to use the end event time for the selected stage.").html_safe % { epic_link_start: epic_link_start, epic_link_end: "</a>".html_safe }
   end
 
+  def use_aggregated_backend
+    aggregation = Analytics::CycleAnalytics::Aggregation.safe_create_for_group(@group)
+    aggregation.update!(enabled: params[:enabled])
+
+    render json: { enabled: aggregation.enabled }
+  end
+
   private
 
   override :all_cycle_analytics_params

ee/config/routes/group.rb

@@ -27,7 +27,9 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
       resources :coverage_reports, only: :index
       resource :merge_request_analytics, only: :show
       resource :repository_analytics, only: :show
-      resource :cycle_analytics, only: :show, path: 'value_stream_analytics'
+      resource :cycle_analytics, only: :show, path: 'value_stream_analytics' do
+        put :use_aggregated_backend, on: :collection
+      end
       scope module: :cycle_analytics, as: 'cycle_analytics', path: 'value_stream_analytics' do
         resources :stages, only: [:index, :create, :update, :destroy] do
           member do

ee/spec/controllers/groups/analytics/cycle_analytics_controller_spec.rb

@@ -11,78 +11,125 @@ RSpec.describe Groups::Analytics::CycleAnalyticsController do
     sign_in(user)
   end
 
-  context 'when the license is available' do
-    before do
-      stub_licensed_features(cycle_analytics_for_groups: true)
-    end
+  context 'GET show' do
+    context 'when the license is available' do
+      before do
+        stub_licensed_features(cycle_analytics_for_groups: true)
+      end
 
-    it 'succeeds' do
-      get(:show, params: { group_id: group })
+      it 'succeeds' do
+        get(:show, params: { group_id: group })
 
-      expect(response).to be_successful
-    end
+        expect(response).to be_successful
+      end
 
-    it 'increments usage counter' do
-      expect(Gitlab::UsageDataCounters::CycleAnalyticsCounter).to receive(:count).with(:views)
+      it 'increments usage counter' do
+        expect(Gitlab::UsageDataCounters::CycleAnalyticsCounter).to receive(:count).with(:views)
 
-      get(:show, params: { group_id: group })
+        get(:show, params: { group_id: group })
 
-      expect(response).to be_successful
-    end
+        expect(response).to be_successful
+      end
+
+      it 'renders `show` template when feature flag is enabled' do
+        get(:show, params: { group_id: group })
+
+        expect(response).to render_template :show
+      end
 
-    it 'renders `show` template when feature flag is enabled' do
-      get(:show, params: { group_id: group })
+      context 'when the initial, default value stream is requested' do
+        let(:value_stream_id) { Analytics::CycleAnalytics::Stages::BaseService::DEFAULT_VALUE_STREAM_NAME }
 
-      expect(response).to render_template :show
+        before do
+          get(:show, params: { group_id: group, value_stream_id: value_stream_id })
+        end
+
+        it 'renders the default in memory value stream' do
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(assigns[:value_stream].name).to eq(value_stream_id)
+        end
+
+        context 'when invalid name is given' do
+          let(:value_stream_id) { 'not_default' }
+
+          it 'renders 404 error' do
+            expect(response).to have_gitlab_http_status(:not_found)
+          end
+        end
+      end
     end
 
-    context 'when the initial, default value stream is requested' do
-      let(:value_stream_id) { Analytics::CycleAnalytics::Stages::BaseService::DEFAULT_VALUE_STREAM_NAME }
+    context 'when the license is missing' do
+      it 'renders 403 error' do
+        get(:show, params: { group_id: group })
 
-      before do
-        get(:show, params: { group_id: group, value_stream_id: value_stream_id })
+        expect(response).to have_gitlab_http_status(:forbidden)
       end
+    end
+
+    context 'when non-existent group is given' do
+      it 'renders 404 error' do
+        get(:show, params: { group_id: 'unknown' })
 
-      it 'renders the default in memory value stream' do
-        expect(response).to have_gitlab_http_status(:ok)
-        expect(assigns[:value_stream].name).to eq(value_stream_id)
+        expect(response).to have_gitlab_http_status(:not_found)
       end
+    end
 
-      context 'when invalid name is given' do
-        let(:value_stream_id) { 'not_default' }
+    context 'with group and value stream params' do
+      let(:value_stream) { create(:cycle_analytics_group_value_stream, group: group) }
 
-        it 'renders 404 error' do
-          expect(response).to have_gitlab_http_status(:not_found)
+      it 'builds request params with group and value stream' do
+        expect_next_instance_of(Gitlab::Analytics::CycleAnalytics::RequestParams) do |instance|
+          expect(instance).to have_attributes(group: group, value_stream: value_stream)
         end
+
+        get(:show, params: { group_id: group, value_stream_id: value_stream })
       end
     end
   end
 
-  context 'when the license is missing' do
-    it 'renders 403 error' do
-      get(:show, params: { group_id: group })
+  context 'GET use_aggregated_backend' do
+    context 'when the license is not available' do
+      it 'renders 403 error' do
+        put(:use_aggregated_backend, params: { group_id: group, enabled: false })
 
-      expect(response).to have_gitlab_http_status(:forbidden)
+        expect(response).to have_gitlab_http_status(:forbidden)
+      end
     end
-  end
 
-  context 'when non-existent group is given' do
-    it 'renders 403 error' do
-      get(:show, params: { group_id: 'unknown' })
+    context 'when the license is available' do
+      before do
+        stub_licensed_features(cycle_analytics_for_groups: true)
+      end
 
-      expect(response).to have_gitlab_http_status(:not_found)
-    end
-  end
+      context 'when non-existent group is given' do
+        it 'renders 404 error' do
+          get(:use_aggregated_backend, params: { group_id: 'unknown' })
+
+          expect(response).to have_gitlab_http_status(:not_found)
+        end
+      end
 
-  context 'with group and value stream params' do
-    let(:value_stream) { create(:cycle_analytics_group_value_stream, group: group) }
+      it 'succeeds' do
+        put(:use_aggregated_backend, params: { group_id: group, enabled: false })
 
-    it 'builds request params with group and value stream' do
-      expect_next_instance_of(Gitlab::Analytics::CycleAnalytics::RequestParams) do |instance|
-        expect(instance).to have_attributes(group: group, value_stream: value_stream)
+        expect(response).to be_successful
+        expect(json_response['enabled']).to eq(false)
       end
 
-      get(:show, params: { group_id: group, value_stream_id: value_stream })
+      context 'when the aggregation record is already created' do
+        before do
+          aggregation = Analytics::CycleAnalytics::Aggregation.safe_create_for_group(group)
+          aggregation.update!(enabled: false)
+        end
+
+        it 'succeeds' do
+          put(:use_aggregated_backend, params: { group_id: group, enabled: true })
+
+          expect(response).to be_successful
+          expect(json_response['enabled']).to eq(true)
+        end
+      end
     end
   end
 end

ee/spec/lib/gitlab/analytics/cycle_analytics/request_params_spec.rb

@@ -242,4 +242,33 @@ RSpec.describe Gitlab::Analytics::CycleAnalytics::RequestParams do
       })
     end
   end
+
+  describe 'use_aggregated_data_collector param' do
+    subject(:value) { described_class.new(params).to_data_collector_params[:use_aggregated_data_collector] }
+
+    context 'when the use_vsa_aggregated_tables feature flag is on' do
+      before do
+        stub_feature_flags(use_vsa_aggregated_tables: true)
+      end
+
+      it { is_expected.to eq(true) }
+
+      context 'when the aggregation is disabled by the user' do
+        before do
+          aggregation = ::Analytics::CycleAnalytics::Aggregation.safe_create_for_group(root_group)
+          aggregation.update!(enabled: false)
+        end
+
+        it { is_expected.to eq(false) }
+      end
+    end
+
+    context 'when the use_vsa_aggregated_tables feature flag is off' do
+      before do
+        stub_feature_flags(use_vsa_aggregated_tables: false)
+      end
+
+      it { is_expected.to eq(false) }
+    end
+  end
 end

lib/gitlab/analytics/cycle_analytics/request_params.rb

@@ -80,7 +80,7 @@ module Gitlab
             direction: direction&.to_sym,
             page: page,
             end_event_filter: end_event_filter.to_sym,
-            use_aggregated_data_collector: Feature.enabled?(:use_vsa_aggregated_tables, group || project, default_enabled: :yaml)
+            use_aggregated_data_collector: use_aggregated_backend?
           }.merge(attributes.symbolize_keys.slice(*FINDER_PARAM_NAMES))
         end
 
@@ -104,8 +104,13 @@ module Gitlab
 
         private
 
+        def use_aggregated_backend?
+          group.present? && # for now it's only available on the group-level
+            aggregation.enabled &&
+            Feature.enabled?(:use_vsa_aggregated_tables, group, default_enabled: :yaml)
+        end
+
         def aggregation_attributes
-          aggregation = ::Analytics::CycleAnalytics::Aggregation.safe_create_for_group(group)
           {
             enabled: aggregation.enabled.to_s,
             last_run_at: aggregation.last_incremental_run_at&.iso8601,
@@ -113,6 +118,10 @@ module Gitlab
           }
         end
 
+        def aggregation
+          @aggregation ||= ::Analytics::CycleAnalytics::Aggregation.safe_create_for_group(group)
+        end
+
         def group_data_attributes
           {
             id: group.id,

spec/models/analytics/cycle_analytics/aggregation_spec.rb

@@ -76,9 +76,12 @@ RSpec.describe Analytics::CycleAnalytics::Aggregation, type: :model do
 
   describe '#estimated_next_run_at' do
     around do |example|
-      freeze_time { example.run }
+      travel_to(Time.utc(2019, 3, 17, 13, 3)) { example.run }
     end
 
+    # aggregation runs on every 10 minutes
+    let(:minutes_until_next_aggregation) { 7.minutes }
+
     context 'when aggregation was not yet executed for the given group' do
       let(:aggregation) { create(:cycle_analytics_aggregation, last_incremental_run_at: nil) }
 
@@ -91,7 +94,7 @@ RSpec.describe Analytics::CycleAnalytics::Aggregation, type: :model do
       let!(:aggregation) { create(:cycle_analytics_aggregation, last_incremental_run_at: 5.minutes.ago) }
 
       it 'returns the duration between the previous run timestamp and the earliest last_incremental_run_at' do
-        expect(aggregation.estimated_next_run_at).to eq(10.minutes.from_now)
+        expect(aggregation.estimated_next_run_at).to eq((10.minutes + minutes_until_next_aggregation).from_now)
       end
 
       context 'when the aggregation has persisted previous runtimes' do
@@ -100,7 +103,7 @@ RSpec.describe Analytics::CycleAnalytics::Aggregation, type: :model do
         end
 
         it 'adds the runtime to the estimation' do
-          expect(aggregation.estimated_next_run_at).to eq((10.minutes.seconds + 60.seconds).from_now)
+          expect(aggregation.estimated_next_run_at).to eq((10.minutes.seconds + 60.seconds + minutes_until_next_aggregation).from_now)
         end
       end
     end
@@ -114,14 +117,8 @@ RSpec.describe Analytics::CycleAnalytics::Aggregation, type: :model do
     context 'when only one aggregation record present' do
       let!(:aggregation) { create(:cycle_analytics_aggregation, last_incremental_run_at: 5.minutes.ago) }
 
-      it 'returns nil' do
-        expect(aggregation.estimated_next_run_at).to eq(nil)
-      end
-    end
-
-    context 'when the aggregation is disabled' do
-      it 'returns nil' do
-        expect(described_class.new(enabled: false).estimated_next_run_at).to eq(nil)
+      it 'returns the minutes until the next aggregation' do
+        expect(aggregation.estimated_next_run_at).to eq(minutes_until_next_aggregation.from_now)
       end
     end
   end