Merge branch 'pedropombeiro/492116/allow-periods-longer-than-1-week' into 'master'

GraphQL: Allow periods longer than one week for PipelineAnalytics

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/167447



Merged-by: Max Woolf <mwoolf@gitlab.com>
Approved-by: david hamp-gonsalves <dhamp-gonsalves@gitlab.com>
Approved-by: Hordur Freyr Yngvason <hfyngvason@gitlab.com>
Approved-by: Max Woolf <mwoolf@gitlab.com>
Reviewed-by: Pedro Pombeiro <noreply@pedro.pombei.ro>
Reviewed-by: Hordur Freyr Yngvason <hfyngvason@gitlab.com>
Reviewed-by: Max Woolf <mwoolf@gitlab.com>
Co-authored-by: Pedro Pombeiro <noreply@pedro.pombei.ro>

app/services/ci/collect_pipeline_analytics_service.rb

@@ -2,8 +2,6 @@
 
 module Ci
   class CollectPipelineAnalyticsService
-    TIME_BUCKETS_LIMIT = 1.week.in_hours + 1 # +1 to add some error margin
-
     STATUS_GROUP_TO_STATUSES = { success: %w[success], failed: %w[failed], other: %w[canceled skipped] }.freeze
     STATUS_GROUPS = STATUS_GROUP_TO_STATUSES.keys.freeze
     STATUS_TO_STATUS_GROUP = STATUS_GROUP_TO_STATUSES.flat_map { |k, v| v.product([k]) }.to_h
@@ -25,9 +23,8 @@ def execute
 
       return ServiceResponse.error(message: 'Not allowed') unless allowed?
 
-      if (@to_time - @from_time) / 1.hour > TIME_BUCKETS_LIMIT
-        return ServiceResponse.error(message: "Maximum of #{TIME_BUCKETS_LIMIT} 1-hour intervals can be requested")
-      end
+      error_message = clickhouse_model.validate_time_window(@from_time, @to_time)
+      return ServiceResponse.error(message: error_message) if error_message
 
       ServiceResponse.success(payload: { aggregate: calculate_aggregate })
     end
@@ -39,7 +36,11 @@ def allowed?
     end
 
     def clickhouse_model
-      ::ClickHouse::Models::Ci::FinishedPipelinesHourly
+      if ::ClickHouse::Models::Ci::FinishedPipelinesHourly.time_window_valid?(@from_time, @to_time)
+        return ::ClickHouse::Models::Ci::FinishedPipelinesHourly
+      end
+
+      ::ClickHouse::Models::Ci::FinishedPipelinesDaily
     end
 
     def calculate_aggregate

lib/click_house/models/ci/finished_pipelines_base.rb

@@ -4,6 +4,14 @@ module ClickHouse # rubocop:disable Gitlab/BoundedContexts -- Existing module
   module Models
     module Ci
       class FinishedPipelinesBase < ClickHouse::Models::BaseModel
+        def self.time_window_valid?(from_time, to_time)
+          raise NotImplementedError, "subclasses of #{self.class.name} must implement #{__method__}"
+        end
+
+        def self.validate_time_window(from_time, to_time)
+          raise NotImplementedError, "subclasses of #{self.class.name} must implement #{__method__}"
+        end
+
         def self.for_project(project)
           new.for_project(project)
         end

lib/click_house/models/ci/finished_pipelines_daily.rb

@@ -4,9 +4,21 @@ module ClickHouse # rubocop:disable Gitlab/BoundedContexts -- Existing module
   module Models
     module Ci
       class FinishedPipelinesDaily < FinishedPipelinesBase
+        TIME_BUCKETS_LIMIT = 366
+
         def self.table_name
           'ci_finished_pipelines_daily'
         end
+
+        def self.time_window_valid?(from_time, to_time)
+          (to_time - from_time) / 1.day < TIME_BUCKETS_LIMIT
+        end
+
+        def self.validate_time_window(from_time, to_time)
+          return if time_window_valid?(from_time, to_time)
+
+          "Maximum of #{TIME_BUCKETS_LIMIT} days can be requested"
+        end
       end
     end
   end

lib/click_house/models/ci/finished_pipelines_hourly.rb

@@ -4,9 +4,21 @@ module ClickHouse # rubocop:disable Gitlab/BoundedContexts -- Existing module
   module Models
     module Ci
       class FinishedPipelinesHourly < FinishedPipelinesBase
+        TIME_BUCKETS_LIMIT = 1.week.in_hours.to_i + 1 # +1 to add some error margin
+
         def self.table_name
           'ci_finished_pipelines_hourly'
         end
+
+        def self.time_window_valid?(from_time, to_time)
+          (to_time - from_time) / 1.hour < TIME_BUCKETS_LIMIT
+        end
+
+        def self.validate_time_window(from_time, to_time)
+          return if time_window_valid?(from_time, to_time)
+
+          "Maximum of #{TIME_BUCKETS_LIMIT} hours can be requested"
+        end
       end
     end
   end

spec/lib/click_house/models/ci/finished_pipelines_daily_spec.rb

@@ -4,4 +4,40 @@
 
 RSpec.describe ClickHouse::Models::Ci::FinishedPipelinesDaily, feature_category: :fleet_visibility do
   it_behaves_like 'a ci_finished_pipelines aggregation model', :ci_finished_pipelines_daily
+
+  describe '.time_window_valid?', :freeze_time do
+    subject(:time_window_valid?) { described_class.time_window_valid?(from_time, to_time) }
+
+    context 'with time window of less than 366 days' do
+      let(:from_time) { 1.second.after(366.days.ago) }
+      let(:to_time) { Time.current }
+
+      it { is_expected.to eq true }
+    end
+
+    context 'with time window of 366 days' do
+      let(:from_time) { 366.days.ago }
+      let(:to_time) { Time.current }
+
+      it { is_expected.to eq false }
+    end
+  end
+
+  describe '.validate_time_window', :freeze_time do
+    subject(:validate_time_window) { described_class.validate_time_window(from_time, to_time) }
+
+    context 'with time window of less than 366 days' do
+      let(:from_time) { 1.second.after(366.days.ago) }
+      let(:to_time) { Time.current }
+
+      it { is_expected.to be_nil }
+    end
+
+    context 'with time window of 366 days' do
+      let(:from_time) { 366.days.ago }
+      let(:to_time) { Time.current }
+
+      it { is_expected.to eq("Maximum of 366 days can be requested") }
+    end
+  end
 end

spec/lib/click_house/models/ci/finished_pipelines_hourly_spec.rb

@@ -4,4 +4,40 @@
 
 RSpec.describe ClickHouse::Models::Ci::FinishedPipelinesHourly, feature_category: :fleet_visibility do
   it_behaves_like 'a ci_finished_pipelines aggregation model', :ci_finished_pipelines_hourly
+
+  describe '.time_window_valid?', :freeze_time do
+    subject(:time_window_valid?) { described_class.time_window_valid?(from_time, to_time) }
+
+    context 'with time window of one week and one hour' do
+      let(:from_time) { (1.hour - 1.second).before(1.week.ago) }
+      let(:to_time) { Time.current }
+
+      it { is_expected.to eq true }
+    end
+
+    context 'with time window of one week and 1 hour' do
+      let(:from_time) { 1.hour.before(1.week.ago) }
+      let(:to_time) { Time.current }
+
+      it { is_expected.to eq false }
+    end
+  end
+
+  describe '.validate_time_window', :freeze_time do
+    subject(:validate_time_window) { described_class.validate_time_window(from_time, to_time) }
+
+    context 'with time window of one week and one hour' do
+      let(:from_time) { (1.hour - 1.second).before(1.week.ago) }
+      let(:to_time) { Time.current }
+
+      it { is_expected.to be_nil }
+    end
+
+    context 'with time window of one week and 1 hour' do
+      let(:from_time) { 1.hour.before(1.week.ago) }
+      let(:to_time) { Time.current }
+
+      it { is_expected.to eq("Maximum of 169 hours can be requested") }
+    end
+  end
 end

spec/requests/api/graphql/project/project_pipeline_statistics_spec.rb

@@ -72,7 +72,7 @@
     end
   end
 
-  describe 'aggregate' do
+  describe 'aggregate', :aggregate_failures do
     subject(:aggregate) do
       perform_request
 
@@ -86,6 +86,9 @@
     end
 
     it "contains expected data for the last week" do
+      perform_request
+
+      expect_graphql_errors_to_be_empty
       expect(aggregate).to eq({
         'label' => nil,
         'all' => '0',
@@ -97,6 +100,9 @@
 
     context 'when there are pipelines in last week', time_travel_to: Time.utc(2024, 5, 11) do
       it "contains expected data for the last week" do
+        perform_request
+
+        expect_graphql_errors_to_be_empty
         expect(aggregate).to eq({
           'label' => nil,
           'all' => '4',
@@ -112,6 +118,9 @@
       let(:to_time) { '2024-05-11T00:00:00+00:00' }
 
       it "contains expected data for the period" do
+        perform_request
+
+        expect_graphql_errors_to_be_empty
         expect(aggregate).to eq({
           'label' => nil,
           'all' => '2',
@@ -120,6 +129,35 @@
           'other' => '0'
         })
       end
+
+      context 'with time window spanning less than 1 year' do
+        let(:from_time) { '2023-05-11T00:00:00+00:00' }
+        let(:to_time) { '2024-05-10T23:59:59+00:00' }
+
+        it "contains expected data for the period" do
+          perform_request
+
+          expect_graphql_errors_to_be_empty
+          expect(aggregate).to eq({
+            'label' => nil,
+            'all' => '5',
+            'success' => '1',
+            'failed' => '2',
+            'other' => '1'
+          })
+        end
+      end
+
+      context 'with time window spanning 1 year' do
+        let(:from_time) { '2024-01-01T00:00:00+02:00' }
+        let(:to_time) { '2025-01-01T00:00:00+02:00' }
+
+        it "contains expected data for the period" do
+          perform_request
+
+          expect_graphql_errors_to_include("Maximum of 366 days can be requested")
+        end
+      end
     end
   end
 

spec/services/ci/collect_pipeline_analytics_service_spec.rb

@@ -105,6 +105,7 @@
 
     context 'when requesting statistics starting one second before beginning of week' do
       let(:from_time) { 1.second.before(starting_time) }
+      let(:to_time) { 1.second.before(ending_time) }
 
       it 'does not include job starting 1 second before start of week' do
         expect(result.errors).to eq([])
@@ -115,6 +116,7 @@
 
     context 'when requesting statistics starting one hour before beginning of week' do
       let(:from_time) { 1.hour.before(starting_time) }
+      let(:to_time) { 1.second.before(ending_time) }
 
       it 'includes job starting 1 second before start of week' do
         expect(result.errors).to eq([])
@@ -123,12 +125,11 @@
       end
     end
 
-    context 'when requesting hourly statistics that span more than one week' do
-      let(:from_time) { (1.hour + 1.second).before(starting_time) }
+    context 'when requesting statistics that span more than one year' do
+      let(:from_time) { (366.days + 1.second).before(starting_time) }
 
       it 'returns an error' do
-        expect(result.errors).to contain_exactly(
-          "Maximum of #{described_class::TIME_BUCKETS_LIMIT} 1-hour intervals can be requested")
+        expect(result.errors).to contain_exactly("Maximum of 366 days can be requested")
         expect(result.error?).to eq(true)
       end
     end