From 60c931e6dbedeef3931eb8dc89bd4f5095f97f65 Mon Sep 17 00:00:00 2001
From: Mehmet Emin INAC <minac@gitlab.com>
Date: Fri, 13 Aug 2021 23:24:26 +0300
Subject: [PATCH] Disable updating the historical vulnerability statistics by
default
Historical vulnerability statistics supposed to be updated once a day
by a cronjob because updating them always for each update/insert of a
vulnerability can reduce the throughput of our worker classes and API
endpoints.
With this change, to be able to observe the side effects of disabling
the update logic of historical vulnerability statistics, we are moving
it behind a feature flag. We will remove the logic and the feature flag
eventually.
Changelog: other
EE: true
---
doc/api/graphql/reference/index.md | 6 +-
ee/app/graphql/ee/types/group_type.rb | 2 +-
ee/app/graphql/ee/types/project_type.rb | 2 +-
ee/app/graphql/ee/types/query_type.rb | 2 +-
.../historical_statistics/update_service.rb | 10 ++-
...erability_statistics_always_consistent.yml | 8 +++
.../update_service_spec.rb | 69 ++++++++++++++-----
7 files changed, 75 insertions(+), 24 deletions(-)
create mode 100644 ee/config/feature_flags/development/keep_historical_vulnerability_statistics_always_consistent.yml
diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md
index d349e13450a80..3f9fd5c1d3424 100644
--- a/doc/api/graphql/reference/index.md
+++ b/doc/api/graphql/reference/index.md
@@ -479,7 +479,7 @@ four standard [pagination arguments](#connection-pagination-arguments):
### `Query.vulnerabilitiesCountByDay`
-Number of vulnerabilities per day for the projects on the current user's instance security dashboard.
+The historical number of vulnerabilities per day for the projects on the current user's instance security dashboard.
Returns [`VulnerabilitiesCountByDayConnection`](#vulnerabilitiescountbydayconnection).
@@ -10004,7 +10004,7 @@ four standard [pagination arguments](#connection-pagination-arguments):
##### `Group.vulnerabilitiesCountByDay`
-Number of vulnerabilities per day for the projects in the group and its subgroups.
+The historical number of vulnerabilities per day for the projects in the group and its subgroups.
Returns [`VulnerabilitiesCountByDayConnection`](#vulnerabilitiescountbydayconnection).
@@ -12608,7 +12608,7 @@ four standard [pagination arguments](#connection-pagination-arguments):
##### `Project.vulnerabilitiesCountByDay`
-Number of vulnerabilities per day for the project.
+The historical number of vulnerabilities per day for the project.
Returns [`VulnerabilitiesCountByDayConnection`](#vulnerabilitiescountbydayconnection).
diff --git a/ee/app/graphql/ee/types/group_type.rb b/ee/app/graphql/ee/types/group_type.rb
index a27ae88aa1f38..78f0012b76939 100644
--- a/ee/app/graphql/ee/types/group_type.rb
+++ b/ee/app/graphql/ee/types/group_type.rb
@@ -62,7 +62,7 @@ module GroupType
field :vulnerabilities_count_by_day,
::Types::VulnerabilitiesCountByDayType.connection_type,
null: true,
- description: 'Number of vulnerabilities per day for the projects in the group and its subgroups.',
+ description: 'The historical number of vulnerabilities per day for the projects in the group and its subgroups.',
resolver: ::Resolvers::VulnerabilitiesCountPerDayResolver
field :vulnerability_grades,
diff --git a/ee/app/graphql/ee/types/project_type.rb b/ee/app/graphql/ee/types/project_type.rb
index e5014f649f6e2..0b5a6f373ee45 100644
--- a/ee/app/graphql/ee/types/project_type.rb
+++ b/ee/app/graphql/ee/types/project_type.rb
@@ -25,7 +25,7 @@ module ProjectType
field :vulnerabilities_count_by_day,
::Types::VulnerabilitiesCountByDayType.connection_type,
null: true,
- description: 'Number of vulnerabilities per day for the project.',
+ description: 'The historical number of vulnerabilities per day for the project.',
resolver: ::Resolvers::VulnerabilitiesCountPerDayResolver
field :vulnerability_severities_count, ::Types::VulnerabilitySeveritiesCountType, null: true,
diff --git a/ee/app/graphql/ee/types/query_type.rb b/ee/app/graphql/ee/types/query_type.rb
index c67315000078b..d627bf9126d4d 100644
--- a/ee/app/graphql/ee/types/query_type.rb
+++ b/ee/app/graphql/ee/types/query_type.rb
@@ -34,7 +34,7 @@ module QueryType
null: true,
resolver: ::Resolvers::VulnerabilitiesCountPerDayResolver,
description: <<~DESC
- Number of vulnerabilities per day for the projects on the current user's instance security dashboard.
+ The historical number of vulnerabilities per day for the projects on the current user's instance security dashboard.
DESC
field :geo_node, ::Types::Geo::GeoNodeType,
diff --git a/ee/app/services/vulnerabilities/historical_statistics/update_service.rb b/ee/app/services/vulnerabilities/historical_statistics/update_service.rb
index b5f88d8da0499..a716c8c898db3 100644
--- a/ee/app/services/vulnerabilities/historical_statistics/update_service.rb
+++ b/ee/app/services/vulnerabilities/historical_statistics/update_service.rb
@@ -15,7 +15,7 @@ def initialize(project)
# rubocop: disable CodeReuse/ActiveRecord
def execute
- return if vulnerability_statistic.blank?
+ return unless update_statistic?
::Vulnerabilities::HistoricalStatistic.safe_ensure_unique(retries: 1) do
historical_statistic = vulnerability_historical_statistics.find_or_initialize_by(date: vulnerability_statistic.updated_at)
@@ -29,6 +29,14 @@ def execute
attr_reader :project
delegate :vulnerability_statistic, :vulnerability_historical_statistics, to: :project
+
+ def update_statistic?
+ keep_statistics_always_consistent? && vulnerability_statistic.present?
+ end
+
+ def keep_statistics_always_consistent?
+ Feature.enabled?(:keep_historical_vulnerability_statistics_always_consistent, project)
+ end
end
end
end
diff --git a/ee/config/feature_flags/development/keep_historical_vulnerability_statistics_always_consistent.yml b/ee/config/feature_flags/development/keep_historical_vulnerability_statistics_always_consistent.yml
new file mode 100644
index 0000000000000..672b73cb8a2b0
--- /dev/null
+++ b/ee/config/feature_flags/development/keep_historical_vulnerability_statistics_always_consistent.yml
@@ -0,0 +1,8 @@
+---
+name: keep_historical_vulnerability_statistics_always_consistent
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/68189
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/338508
+milestone: '14.2'
+type: development
+group: group::threat insights
+default_enabled: false
diff --git a/ee/spec/services/vulnerabilities/historical_statistics/update_service_spec.rb b/ee/spec/services/vulnerabilities/historical_statistics/update_service_spec.rb
index 13cab41437a6c..ece101263fe61 100644
--- a/ee/spec/services/vulnerabilities/historical_statistics/update_service_spec.rb
+++ b/ee/spec/services/vulnerabilities/historical_statistics/update_service_spec.rb
@@ -24,34 +24,69 @@
describe '#execute' do
subject(:update_stats) { described_class.new(project).execute }
- context 'when the statistic is not empty' do
- before do
- create(:vulnerability_statistic, project: project, low: 2)
- end
+ around do |example|
+ travel_to(Date.current) { example.run }
+ end
- context 'when there is already a record in the database' do
- around do |example|
- travel_to(Date.current) { example.run }
+ context 'when the `keep_historical_vulnerability_statistics_always_consistent` feature is enabled' do
+ context 'when the statistic is not empty' do
+ before do
+ create(:vulnerability_statistic, project: project, low: 2)
end
- it 'changes the existing historical statistic entity' do
- historical_statistic = create(:vulnerability_historical_statistic, project: project, letter_grade: 'c')
+ context 'when there exists a record in the database' do
+ it 'changes the existing historical statistic entity' do
+ historical_statistic = create(:vulnerability_historical_statistic, project: project, letter_grade: 'c')
+
+ expect { update_stats }.to change { historical_statistic.reload.letter_grade }.from('c').to('b')
+ .and change { historical_statistic.reload.low }.to(2)
+ end
+ end
- expect { update_stats }.to change { historical_statistic.reload.letter_grade }.from('c').to('b')
- .and change { historical_statistic.reload.low }.to(2)
+ context 'when there exists no record in the database' do
+ it 'creates a new record in the database' do
+ expect { update_stats }.to change { Vulnerabilities::HistoricalStatistic.count }.by(1)
+ end
end
end
- context 'when there is no existing record in the database' do
- it 'creates a new record in the database' do
- expect { update_stats }.to change { Vulnerabilities::HistoricalStatistic.count }.by(1)
+ context 'when the statistic is empty' do
+ it 'does not create any historical statistic entity' do
+ expect { update_stats }.not_to change { Vulnerabilities::Statistic.count }
end
end
end
- context 'when the statistic is empty' do
- it 'does not create any historical statistic entity' do
- expect { update_stats }.not_to change { Vulnerabilities::Statistic.count }
+ context 'when the `keep_historical_vulnerability_statistics_always_consistent` feature is disabled' do
+ before do
+ stub_feature_flags(keep_historical_vulnerability_statistics_always_consistent: false)
+ end
+
+ context 'when the statistic is not empty' do
+ before do
+ create(:vulnerability_statistic, project: project, low: 2)
+ end
+
+ context 'when there exists a record in the database' do
+ it 'does not change the existing historical statistic entity' do
+ historical_statistic = create(:vulnerability_historical_statistic, project: project, letter_grade: 'c')
+
+ expect { update_stats }.to not_change { historical_statistic.reload.letter_grade }.from('c')
+ .and not_change { historical_statistic.reload.low }.from(0)
+ end
+ end
+
+ context 'when there exists no record in the database' do
+ it 'does not create a new record in the database' do
+ expect { update_stats }.not_to change { Vulnerabilities::HistoricalStatistic.count }
+ end
+ end
+ end
+
+ context 'when the statistic is empty' do
+ it 'does not create any historical statistic entity' do
+ expect { update_stats }.not_to change { Vulnerabilities::Statistic.count }
+ end
end
end
end
--
GitLab