From 3413ba8faf3f82ac2e1ad68f385a199c5002812a Mon Sep 17 00:00:00 2001 From: Pavel Shutsin Date: Thu, 29 Oct 2020 15:30:54 +0300 Subject: [PATCH] Add total and closed counters to issues analytics Adds backend support for closed issues count and accumulated open issues count for issues analytics chart. --- ...201103095752_add_issues_closed_at_index.rb | 17 ++++ db/schema_migrations/20201103095752 | 1 + db/structure.sql | 2 + .../groups/issues_analytics_controller.rb | 8 +- .../analytics/issues_analytics_controller.rb | 8 +- ee/app/models/analytics/issues_analytics.rb | 77 +++++++++++++++++++ ee/app/models/issuables_analytics.rb | 3 + ...issues-analytics-accumulated-open-data.yml | 5 ++ .../new_issues_analytics_chart_data.yml | 7 ++ .../issues_analytics_controller_spec.rb | 2 +- .../issues_analytics_controller_spec.rb | 2 +- .../models/analytics/issues_analytics_spec.rb | 75 ++++++++++++++++++ .../shared_issues_analytics_examples.rb | 26 ++++++- 13 files changed, 224 insertions(+), 9 deletions(-) create mode 100644 db/migrate/20201103095752_add_issues_closed_at_index.rb create mode 100644 db/schema_migrations/20201103095752 create mode 100644 ee/app/models/analytics/issues_analytics.rb create mode 100644 ee/changelogs/unreleased/247271-add-issues-analytics-accumulated-open-data.yml create mode 100644 ee/config/feature_flags/development/new_issues_analytics_chart_data.yml create mode 100644 ee/spec/models/analytics/issues_analytics_spec.rb diff --git a/db/migrate/20201103095752_add_issues_closed_at_index.rb b/db/migrate/20201103095752_add_issues_closed_at_index.rb new file mode 100644 index 00000000000000..7a8ee4e8d67caa --- /dev/null +++ b/db/migrate/20201103095752_add_issues_closed_at_index.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class AddIssuesClosedAtIndex < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_index(:issues, [:project_id, :closed_at]) + end + + def down + remove_concurrent_index_by_name(:issues, 'index_issues_on_project_id_and_closed_at') + end +end diff --git a/db/schema_migrations/20201103095752 b/db/schema_migrations/20201103095752 new file mode 100644 index 00000000000000..4888f7c5dfbab9 --- /dev/null +++ b/db/schema_migrations/20201103095752 @@ -0,0 +1 @@ +3427cf92dc785f399329b00a3dded01dd2a6386cafbd0ef4b732bfcf522ce615 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 8cfc534ff53cf4..87c771644b9c9d 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -20926,6 +20926,8 @@ CREATE INDEX index_issues_on_milestone_id ON issues USING btree (milestone_id); CREATE INDEX index_issues_on_moved_to_id ON issues USING btree (moved_to_id) WHERE (moved_to_id IS NOT NULL); +CREATE INDEX index_issues_on_project_id_and_closed_at ON issues USING btree (project_id, closed_at); + CREATE UNIQUE INDEX index_issues_on_project_id_and_external_key ON issues USING btree (project_id, external_key) WHERE (external_key IS NOT NULL); CREATE UNIQUE INDEX index_issues_on_project_id_and_iid ON issues USING btree (project_id, iid); diff --git a/ee/app/controllers/groups/issues_analytics_controller.rb b/ee/app/controllers/groups/issues_analytics_controller.rb index 60cbd2c4c54628..fe74393914f910 100644 --- a/ee/app/controllers/groups/issues_analytics_controller.rb +++ b/ee/app/controllers/groups/issues_analytics_controller.rb @@ -16,8 +16,12 @@ def show format.html format.json do - @chart_data = - IssuablesAnalytics.new(issuables: issuables_collection, months_back: params[:months_back]).data + @chart_data = if Feature.enabled?(:new_issues_analytics_chart_data, group) + Analytics::IssuesAnalytics.new(issues: issuables_collection, months_back: params[:months_back]) + .monthly_counters + else + IssuablesAnalytics.new(issuables: issuables_collection, months_back: params[:months_back]).data + end render json: @chart_data end diff --git a/ee/app/controllers/projects/analytics/issues_analytics_controller.rb b/ee/app/controllers/projects/analytics/issues_analytics_controller.rb index 108e4f9990326b..d6897340352955 100644 --- a/ee/app/controllers/projects/analytics/issues_analytics_controller.rb +++ b/ee/app/controllers/projects/analytics/issues_analytics_controller.rb @@ -15,8 +15,12 @@ def show format.html format.json do - @chart_data = - IssuablesAnalytics.new(issuables: issuables_collection, months_back: params[:months_back]).data + @chart_data = if Feature.enabled?(:new_issues_analytics_chart_data, project.namespace) + Analytics::IssuesAnalytics.new(issues: issuables_collection, months_back: params[:months_back]) + .monthly_counters + else + IssuablesAnalytics.new(issuables: issuables_collection, months_back: params[:months_back]).data + end render json: @chart_data end diff --git a/ee/app/models/analytics/issues_analytics.rb b/ee/app/models/analytics/issues_analytics.rb new file mode 100644 index 00000000000000..ede0e0fb02855d --- /dev/null +++ b/ee/app/models/analytics/issues_analytics.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +# Gathers issues stats per month returning a hash +# with the format: {"2017-12"=>{"created" => 5, "closed" => 3, "accumulated_open" => 18}, ...} +class Analytics::IssuesAnalytics + attr_reader :issues, :start_date, :months_back + + DATE_FORMAT = "%Y-%m".freeze + + def initialize(issues:, months_back: nil) + @issues = issues + @months_back = months_back.present? ? (months_back.to_i - 1) : 12 + @start_date = @months_back.months.ago.beginning_of_month.to_date + end + + def monthly_counters + observation_months.each_with_object({}) do |month, result| + result[month.strftime(DATE_FORMAT)] = counters(month: month) + end + end + + private + + def observation_months + @observation_months ||= (0..months_back).map do |offset| + start_date + offset.months + end + end + + def counters(month:) + { + created: created[month], + closed: closed[month], + accumulated_open: accumulated_open(month) + } + end + + def created + @created ||= load_monthly_info_on(field: 'created_at') + end + + def closed + @closed ||= load_monthly_info_on(field: 'closed_at') + end + + def load_monthly_info_on(field:) + counters_stats = issues + .reorder(nil) + .where(Issue.arel_table[field].gteq(start_date)) + .group('month') + .pluck(Arel.sql("date_trunc('month', issues.#{field})::date as month, count(*) as counter")) + .to_h + + observation_months.each do |month| + counters_stats[month] ||= 0 + end + + counters_stats + end + + def accumulated_open(month) + @accumulated_open ||= {} + @accumulated_open[month] ||= begin + base = month == start_date ? initial_accumulated_open : accumulated_open(month - 1.month) + + base + created[month] - closed[month] + end + end + + def initial_accumulated_open + @initial_accumulated_open ||= issues + .opened + .where(Issue.arel_table['created_at'].lt(start_date)) + .reorder(nil) + .count + end +end diff --git a/ee/app/models/issuables_analytics.rb b/ee/app/models/issuables_analytics.rb index fcb6f3c9bda0ce..e762eb2dc9d9c2 100644 --- a/ee/app/models/issuables_analytics.rb +++ b/ee/app/models/issuables_analytics.rb @@ -5,6 +5,9 @@ # # By default it creates the hash only for the last 12 months including the current month, but it accepts # a parameter to get issuables for n months back. +# +# This class should be removed together with feature flag :new_issues_analytics_chart_data when +# :new_issues_analytics_chart_data becomes default behavior class IssuablesAnalytics include Gitlab::Utils::StrongMemoize diff --git a/ee/changelogs/unreleased/247271-add-issues-analytics-accumulated-open-data.yml b/ee/changelogs/unreleased/247271-add-issues-analytics-accumulated-open-data.yml new file mode 100644 index 00000000000000..925914e61df35e --- /dev/null +++ b/ee/changelogs/unreleased/247271-add-issues-analytics-accumulated-open-data.yml @@ -0,0 +1,5 @@ +--- +title: Add issue close date index +merge_request: 46444 +author: +type: performance diff --git a/ee/config/feature_flags/development/new_issues_analytics_chart_data.yml b/ee/config/feature_flags/development/new_issues_analytics_chart_data.yml new file mode 100644 index 00000000000000..d8922b3e985d2b --- /dev/null +++ b/ee/config/feature_flags/development/new_issues_analytics_chart_data.yml @@ -0,0 +1,7 @@ +--- +name: new_issues_analytics_chart_data +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/46444 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/247271 +group: group::analytics +type: development +default_enabled: false diff --git a/ee/spec/controllers/groups/issues_analytics_controller_spec.rb b/ee/spec/controllers/groups/issues_analytics_controller_spec.rb index 178f63c1089fba..cb1c57d31d6951 100644 --- a/ee/spec/controllers/groups/issues_analytics_controller_spec.rb +++ b/ee/spec/controllers/groups/issues_analytics_controller_spec.rb @@ -9,7 +9,7 @@ let_it_be(:project1) { create(:project, :empty_repo, namespace: group) } let_it_be(:project2) { create(:project, :empty_repo, namespace: group) } let_it_be(:issue1) { create(:issue, project: project1, confidential: true) } - let_it_be(:issue2) { create(:issue, project: project2, state: :closed) } + let_it_be(:issue2) { create(:issue, :closed, project: project2) } before do group.add_owner(user) diff --git a/ee/spec/controllers/projects/analytics/issues_analytics_controller_spec.rb b/ee/spec/controllers/projects/analytics/issues_analytics_controller_spec.rb index bf311fca8abcc6..2a3680c1db96fc 100644 --- a/ee/spec/controllers/projects/analytics/issues_analytics_controller_spec.rb +++ b/ee/spec/controllers/projects/analytics/issues_analytics_controller_spec.rb @@ -8,7 +8,7 @@ let_it_be(:group) { create(:group) } let_it_be(:project1) { create(:project, :empty_repo, namespace: group) } let_it_be(:issue1) { create(:issue, project: project1, confidential: true) } - let_it_be(:issue2) { create(:issue, project: project1, state: :closed) } + let_it_be(:issue2) { create(:issue, :closed, project: project1) } before do group.add_owner(user) diff --git a/ee/spec/models/analytics/issues_analytics_spec.rb b/ee/spec/models/analytics/issues_analytics_spec.rb new file mode 100644 index 00000000000000..5af923a22c537e --- /dev/null +++ b/ee/spec/models/analytics/issues_analytics_spec.rb @@ -0,0 +1,75 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Analytics::IssuesAnalytics do + subject { described_class.new(issues: project.issues) } + + let_it_be(:project) { create(:project, skip_disk_validation: true) } + + describe '#monthly_counters' do + let_it_be(:today) { Date.today } + let_it_be(:seed_data) do + (0..13).to_a.each_with_object({}) do |month_offset, result| + month = today - month_offset.months + + result[month.strftime(described_class::DATE_FORMAT)] = { + created: rand(2..4), + closed: rand(2) + } + end + end + + let_it_be(:seeded_issues) do + seed_data.map do |month, seed_counters| + month = Date.parse("#{month}-01") + issues = [] + seed_counters[:closed].times do + issues << create(:issue, :closed, project: project, created_at: month + 1.day, closed_at: month + 2.days) + end + + (seed_counters[:created] - seed_counters[:closed]).times do + issues << create(:issue, :opened, project: project, created_at: month + 1.day) + end + + issues + end.flatten + end + + def accumulated_open_for_seeds(month) + seed_data.map do |seed_month, data| + data[:created] - data[:closed] if seed_month <= month + end.compact.sum + end + + context 'without months_back specified' do + let(:expected_counters) do + (0..12).to_a.each_with_object({}) do |month_offset, result| + month = (today - month_offset.months).strftime(described_class::DATE_FORMAT) + + result[month] = seed_data[month].merge(accumulated_open: accumulated_open_for_seeds(month)) + end + end + + it 'returns data for 12 months' do + expect(subject.monthly_counters).to match(expected_counters) + end + end + + context 'with months_back set to 3' do + subject { described_class.new(issues: project.issues, months_back: 3) } + + let(:expected_counters) do + (0..2).to_a.each_with_object({}) do |month_offset, result| + month = (today - month_offset.months).strftime(described_class::DATE_FORMAT) + + result[month] = seed_data[month].merge(accumulated_open: accumulated_open_for_seeds(month)) + end + end + + it 'returns data for 3 months' do + expect(subject.monthly_counters).to match(expected_counters) + end + end + end +end diff --git a/ee/spec/support/shared_examples/controllers/analytics/issues_analytics/shared_issues_analytics_examples.rb b/ee/spec/support/shared_examples/controllers/analytics/issues_analytics/shared_issues_analytics_examples.rb index 2e1d4f1692291a..8b5c7b2c6db973 100644 --- a/ee/spec/support/shared_examples/controllers/analytics/issues_analytics/shared_issues_analytics_examples.rb +++ b/ee/spec/support/shared_examples/controllers/analytics/issues_analytics/shared_issues_analytics_examples.rb @@ -46,8 +46,25 @@ context 'as JSON' do subject { get :show, params: params, format: :json } - it 'renders chart data as JSON' do - expected_result = { issue1.created_at.strftime(IssuablesAnalytics::DATE_FORMAT) => 2 } + context 'when new issue analytics data format is disabled' do + before do + stub_feature_flags(new_issues_analytics_chart_data: false) + end + + it 'renders chart data as JSON' do + expected_result = { issue1.created_at.strftime(IssuablesAnalytics::DATE_FORMAT) => 2 } + + subject + + expect(json_response).to include(expected_result) + end + end + + it 'renders new chart data as JSON' do + month = issue1.created_at.strftime(Analytics::IssuesAnalytics::DATE_FORMAT) + expected_result = { + month => { 'created' => 2, 'closed' => 1, 'accumulated_open' => 1 } + } subject @@ -63,7 +80,10 @@ end it 'does not count issues which user cannot view' do - expected_result = { issue1.created_at.strftime(IssuablesAnalytics::DATE_FORMAT) => 1 } + month = issue2.created_at.strftime(Analytics::IssuesAnalytics::DATE_FORMAT) + expected_result = { + month => { 'created' => 1, 'closed' => 1, 'accumulated_open' => 0 } + } subject -- GitLab