From 61f4d08b7bf734c9af579e54fa8c74ac1bde4b39 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Thu, 12 Oct 2017 10:39:11 +0200 Subject: Allow testing on Gitaly call count Previous efforts were aimed at detecting N + 1 queries, general regressions are hard to find and mitigate. --- doc/development/gitaly.md | 34 ++++++++++++++++++++-- .../projects/pipelines_controller_spec.rb | 32 +++++++++++++------- 2 files changed, 52 insertions(+), 14 deletions(-) diff --git a/doc/development/gitaly.md b/doc/development/gitaly.md index e41d258bec6..0f73217ada3 100644 --- a/doc/development/gitaly.md +++ b/doc/development/gitaly.md @@ -52,8 +52,8 @@ rm -rf tmp/tests/gitaly ## `TooManyInvocationsError` errors -During development and testing, you may experience `Gitlab::GitalyClient::TooManyInvocationsError` failures. -The `GitalyClient` will attempt to block against potential n+1 issues by raising this error +During development and testing, you may experience `Gitlab::GitalyClient::TooManyInvocationsError` failures. +The `GitalyClient` will attempt to block against potential n+1 issues by raising this error when Gitaly is called more than 30 times in a single Rails request or Sidekiq execution. As a temporary measure, export `GITALY_DISABLE_REQUEST_LIMITS=1` to suppress the error. This will disable the n+1 detection @@ -64,7 +64,7 @@ Please raise an issue in the GitLab CE or EE repositories to report the issue. I `TooManyInvocationsError`. Also include any known failing tests if possible. Isolate the source of the n+1 problem. This will normally be a loop that results in Gitaly being called for each -element in an array. If you are unable to isolate the problem, please contact a member +element in an array. If you are unable to isolate the problem, please contact a member of the [Gitaly Team](https://gitlab.com/groups/gl-gitaly/group_members) for assistance. Once the source has been found, wrap it in an `allow_n_plus_1_calls` block, as follows: @@ -79,6 +79,34 @@ end Once the code is wrapped in this block, this code-path will be excluded from n+1 detection. +## Request counts + +Commits and other git data, is now fetched through Gitaly. These fetches can, +much like with a database, be batched. This improves performance for the client +and for Gitaly itself and therefore for the users too. To keep performance stable +and guard performance regressions, Gitaly calls can be counted and the call count +can be tested against: + +```ruby +describe 'Gitaly Request count tests' do + context 'when the request store is activated', :request_store do + it 'correctly counts the gitaly requests made' do + count = gitaly_request_count { subject } + + expect { subject }.to change { Gitlab::GitalyClient.get_request_count }.by(10) + end + end + + context 'when the request store is disabled' do + it 'is always zero' do + subject + + expect(Gitlab::GitalyClient.get_request_count).to be_zero + end + end +end +``` + --- [Return to Development documentation](README.md) diff --git a/spec/controllers/projects/pipelines_controller_spec.rb b/spec/controllers/projects/pipelines_controller_spec.rb index 167e80ed9cd..67b53d2acce 100644 --- a/spec/controllers/projects/pipelines_controller_spec.rb +++ b/spec/controllers/projects/pipelines_controller_spec.rb @@ -3,32 +3,36 @@ require 'spec_helper' describe Projects::PipelinesController do include ApiHelpers - let(:user) { create(:user) } - let(:project) { create(:project, :public) } + set(:user) { create(:user) } + set(:project) { create(:project, :public, :repository) } let(:feature) { ProjectFeature::DISABLED } before do stub_not_protect_default_branch project.add_developer(user) - project.project_feature.update( - builds_access_level: feature) + project.project_feature.update(builds_access_level: feature) sign_in(user) end describe 'GET index.json' do before do - create(:ci_empty_pipeline, status: 'pending', project: project) - create(:ci_empty_pipeline, status: 'running', project: project) - create(:ci_empty_pipeline, status: 'created', project: project) - create(:ci_empty_pipeline, status: 'success', project: project) + branch_head = project.commit + parent = branch_head.parent - get :index, namespace_id: project.namespace, - project_id: project, - format: :json + create(:ci_empty_pipeline, status: 'pending', project: project, sha: branch_head.id) + create(:ci_empty_pipeline, status: 'running', project: project, sha: branch_head.id) + create(:ci_empty_pipeline, status: 'created', project: project, sha: parent.id) + create(:ci_empty_pipeline, status: 'success', project: project, sha: parent.id) + end + + subject do + get :index, namespace_id: project.namespace, project_id: project, format: :json end it 'returns JSON with serialized pipelines' do + subject + expect(response).to have_http_status(:ok) expect(response).to match_response_schema('pipeline') @@ -39,6 +43,12 @@ describe Projects::PipelinesController do expect(json_response['count']['pending']).to eq 1 expect(json_response['count']['finished']).to eq 1 end + + context 'when performing gitaly calls', :request_store do + it 'limits the Gitaly requests' do + expect { subject }.to change { Gitlab::GitalyClient.get_request_count }.by(10) + end + end end describe 'GET show JSON' do -- cgit v1.2.1 From 211e78d59410afe25032614db17c5d842d033fb9 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 17 Oct 2017 08:46:05 +0000 Subject: Update documentation --- doc/development/gitaly.md | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/doc/development/gitaly.md b/doc/development/gitaly.md index 0f73217ada3..2824bbdcfca 100644 --- a/doc/development/gitaly.md +++ b/doc/development/gitaly.md @@ -85,24 +85,12 @@ Commits and other git data, is now fetched through Gitaly. These fetches can, much like with a database, be batched. This improves performance for the client and for Gitaly itself and therefore for the users too. To keep performance stable and guard performance regressions, Gitaly calls can be counted and the call count -can be tested against: +can be tested against. This requires the `:request_store` flag to be set. ```ruby describe 'Gitaly Request count tests' do - context 'when the request store is activated', :request_store do - it 'correctly counts the gitaly requests made' do - count = gitaly_request_count { subject } - - expect { subject }.to change { Gitlab::GitalyClient.get_request_count }.by(10) - end - end - - context 'when the request store is disabled' do - it 'is always zero' do - subject - - expect(Gitlab::GitalyClient.get_request_count).to be_zero - end + it 'correctly counts the gitaly requests made' do + expect { subject }.to change { Gitlab::GitalyClient.get_request_count }.by(10) end end ``` -- cgit v1.2.1 From e5450017d21c7c2b0aa63ca3ac9e07397160e5de Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 17 Oct 2017 09:03:11 +0000 Subject: Update gitaly.md --- doc/development/gitaly.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/doc/development/gitaly.md b/doc/development/gitaly.md index 2824bbdcfca..ca2048c7019 100644 --- a/doc/development/gitaly.md +++ b/doc/development/gitaly.md @@ -89,8 +89,10 @@ can be tested against. This requires the `:request_store` flag to be set. ```ruby describe 'Gitaly Request count tests' do - it 'correctly counts the gitaly requests made' do - expect { subject }.to change { Gitlab::GitalyClient.get_request_count }.by(10) + context 'when the request store is activated', :request_store do + it 'correctly counts the gitaly requests made' do + expect { subject }.to change { Gitlab::GitalyClient.get_request_count }.by(10) + end end end ``` -- cgit v1.2.1