Commit e77a506c authored by Oswaldo Ferreira's avatar Oswaldo Ferreira

Update project_daily_statistics synchronally

The ProjectDailyStatisticsWorker queue accumulates jobs
from time to time, considering how often the git_upload_pack
endpoint is called (taking up to 7 minutes to take the job
from the queue in a few cases).

Turns out that even though this is a low-priority queue (and
it's OK to take a bit more time to process it) it was
seen that the work it does takes around 1.8s (99th percentile),
and 0.8s (99th percentile) DB time, which doesn't explain
involving Sidekiq (and Redis) in the work pipeline.

Here we move the processing out of Sidekiq and back to the
web request behind a feature flag (and deprecate the worker).
parent ee73f5f7
......@@ -23,7 +23,7 @@ module Repositories
# POST /foo/bar.git/git-upload-pack (git pull)
def git_upload_pack
enqueue_fetch_statistics_update
update_fetch_statistics
render_ok
end
......@@ -76,12 +76,16 @@ module Repositories
render plain: exception.message, status: :service_unavailable
end
def enqueue_fetch_statistics_update
def update_fetch_statistics
return unless project
return if Gitlab::Database.read_only?
return unless repo_type.project?
return unless project&.daily_statistics_enabled?
ProjectDailyStatisticsWorker.perform_async(project.id) # rubocop:disable CodeReuse/Worker
if Feature.enabled?(:project_statistics_sync, project, default_enabled: true)
Projects::FetchStatisticsIncrementService.new(project).execute
else
ProjectDailyStatisticsWorker.perform_async(project.id) # rubocop:disable CodeReuse/Worker
end
end
def access
......
......@@ -774,10 +774,6 @@ class Project < ApplicationRecord
{ scope: :project, status: auto_devops&.enabled || Feature.enabled?(:force_autodevops_on_by_default, self) }
end
def daily_statistics_enabled?
Feature.enabled?(:project_daily_statistics, self, default_enabled: true)
end
def unlink_forks_upon_visibility_decrease_enabled?
Feature.enabled?(:unlink_fork_network_upon_visibility_decrease, self, default_enabled: true)
end
......
# frozen_string_literal: true
# Deprecated: https://gitlab.com/gitlab-org/gitlab/-/issues/214585
class ProjectDailyStatisticsWorker # rubocop:disable Scalability/IdempotentWorker
include ApplicationWorker
......
......@@ -4,7 +4,6 @@ module API
class ProjectStatistics < Grape::API
before do
authenticate!
not_found! unless user_project.daily_statistics_enabled?
authorize! :daily_statistics, user_project
end
......
......@@ -95,7 +95,7 @@ describe Repositories::GitHttpController do
allow(controller).to receive(:access_check).and_return(nil)
end
after do
def send_request
post :git_upload_pack, params: params
end
......@@ -106,16 +106,46 @@ describe Repositories::GitHttpController do
it 'does not update project statistics' do
expect(ProjectDailyStatisticsWorker).not_to receive(:perform_async)
send_request
end
end
if expected
it 'updates project statistics' do
expect(ProjectDailyStatisticsWorker).to receive(:perform_async)
context 'when project_statistics_sync feature flag is disabled' do
before do
stub_feature_flags(project_statistics_sync: false)
end
it 'updates project statistics async' do
expect(ProjectDailyStatisticsWorker).to receive(:perform_async)
send_request
end
end
it 'updates project statistics sync' do
expect { send_request }.to change {
Projects::DailyStatisticsFinder.new(project).total_fetch_count
}.from(0).to(1)
end
else
context 'when project_statistics_sync feature flag is disabled' do
before do
stub_feature_flags(project_statistics_sync: false)
end
it 'does not update project statistics' do
expect(ProjectDailyStatisticsWorker).not_to receive(:perform_async)
send_request
end
end
it 'does not update project statistics' do
expect(ProjectDailyStatisticsWorker).not_to receive(:perform_async)
expect { send_request }.not_to change {
Projects::DailyStatisticsFinder.new(project).total_fetch_count
}.from(0)
end
end
end
......
......@@ -2645,18 +2645,6 @@ describe Project do
end
end
describe '#daily_statistics_enabled?' do
it { is_expected.to be_daily_statistics_enabled }
context 'when :project_daily_statistics is disabled for the project' do
before do
stub_feature_flags(project_daily_statistics: { thing: subject, enabled: false })
end
it { is_expected.not_to be_daily_statistics_enabled }
end
end
describe '#change_head' do
let(:project) { create(:project, :repository) }
......
......@@ -50,13 +50,5 @@ describe API::ProjectStatistics do
expect(response).to have_gitlab_http_status(:forbidden)
expect(json_response['message']).to eq('403 Forbidden')
end
it 'responds with 404 when daily_statistics_enabled? is false' do
stub_feature_flags(project_daily_statistics: { thing: public_project, enabled: false })
get api("/projects/#{public_project.id}/statistics", maintainer)
expect(response).to have_gitlab_http_status(:not_found)
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment