Commit 4054df87 authored by Dmitry Gruzd's avatar Dmitry Gruzd Committed by Terri Chu

Add Advanced Search timeout

parent 3f9e766e
...@@ -45,6 +45,23 @@ Gitlab.ee do ...@@ -45,6 +45,23 @@ Gitlab.ee do
end end
end end
### Modified from elasticsearch-model/lib/elasticsearch/model/searching.rb
module Elasticsearch
module Model
module Searching
class SearchRequest
def execute!
response = klass.client.search(@definition)
raise Elastic::TimeoutError if response['timed_out']
response
end
end
end
end
end
### Modified from elasticsearch-model/lib/elasticsearch/model.rb ### Modified from elasticsearch-model/lib/elasticsearch/model.rb
[ [
......
...@@ -15,6 +15,8 @@ module EE ...@@ -15,6 +15,8 @@ module EE
# for self-managed we check if the licensed feature available # for self-managed we check if the licensed feature available
track_redis_hll_event :show, name: 'i_search_paid', track_redis_hll_event :show, name: 'i_search_paid',
if: :track_search_paid? if: :track_search_paid?
rescue_from Elastic::TimeoutError, with: :render_timeout
end end
private private
......
...@@ -13,7 +13,7 @@ module Elastic ...@@ -13,7 +13,7 @@ module Elastic
# Counts need to be fast as we load one count per type of document # Counts need to be fast as we load one count per type of document
# on every page load. Fail early if they are slow since they don't # on every page load. Fail early if they are slow since they don't
# need to be accurate. # need to be accurate.
es_options[:timeout] = '1s' if search_options[:count_only] es_options[:timeout] = search_options[:count_only] ? '1s' : '30s'
# Calling elasticsearch-ruby method # Calling elasticsearch-ruby method
super(query, es_options) super(query, es_options)
......
# frozen_string_literal: true
module Elastic
TimeoutError = Class.new(StandardError)
end
...@@ -38,6 +38,26 @@ RSpec.describe 'Global elastic search', :elastic, :sidekiq_inline do ...@@ -38,6 +38,26 @@ RSpec.describe 'Global elastic search', :elastic, :sidekiq_inline do
end end
end end
shared_examples 'advanced search timeouts' do
context 'when search times out' do
before do
allow_next_instance_of(SearchService) do |service|
allow(service).to receive(:search_objects).and_raise(Elastic::TimeoutError)
end
visit path
end
it 'renders timeout information' do
expect(page).to have_content('Your search timed out')
end
it 'sets tab count to 0' do
expect(page.find('.search-filter .active')).to have_text('0')
end
end
end
describe 'I do not overload the database' do describe 'I do not overload the database' do
let(:creation_traits) { [] } let(:creation_traits) { [] }
...@@ -48,6 +68,7 @@ RSpec.describe 'Global elastic search', :elastic, :sidekiq_inline do ...@@ -48,6 +68,7 @@ RSpec.describe 'Global elastic search', :elastic, :sidekiq_inline do
let(:query_count_multiplier) { 0 } let(:query_count_multiplier) { 0 }
it_behaves_like 'an efficient database result' it_behaves_like 'an efficient database result'
it_behaves_like 'advanced search timeouts'
end end
context 'searching projects' do context 'searching projects' do
...@@ -60,6 +81,7 @@ RSpec.describe 'Global elastic search', :elastic, :sidekiq_inline do ...@@ -60,6 +81,7 @@ RSpec.describe 'Global elastic search', :elastic, :sidekiq_inline do
let(:query_count_multiplier) { 4 } let(:query_count_multiplier) { 4 }
it_behaves_like 'an efficient database result' it_behaves_like 'an efficient database result'
it_behaves_like 'advanced search timeouts'
end end
context 'searching merge requests' do context 'searching merge requests' do
...@@ -70,6 +92,7 @@ RSpec.describe 'Global elastic search', :elastic, :sidekiq_inline do ...@@ -70,6 +92,7 @@ RSpec.describe 'Global elastic search', :elastic, :sidekiq_inline do
let(:query_count_multiplier) { 0 } let(:query_count_multiplier) { 0 }
it_behaves_like 'an efficient database result' it_behaves_like 'an efficient database result'
it_behaves_like 'advanced search timeouts'
end end
context 'searching milestones' do context 'searching milestones' do
...@@ -84,6 +107,8 @@ RSpec.describe 'Global elastic search', :elastic, :sidekiq_inline do ...@@ -84,6 +107,8 @@ RSpec.describe 'Global elastic search', :elastic, :sidekiq_inline do
context 'searching code' do context 'searching code' do
let(:path) { search_path(search: '*', scope: 'blobs') } let(:path) { search_path(search: '*', scope: 'blobs') }
it_behaves_like 'advanced search timeouts'
it 'avoids N+1 database queries' do it 'avoids N+1 database queries' do
project.repository.index_commits_and_blobs project.repository.index_commits_and_blobs
...@@ -107,6 +132,10 @@ RSpec.describe 'Global elastic search', :elastic, :sidekiq_inline do ...@@ -107,6 +132,10 @@ RSpec.describe 'Global elastic search', :elastic, :sidekiq_inline do
let(:path_for_one) { search_path(search: '*', scope: 'commits', per_page: 1) } let(:path_for_one) { search_path(search: '*', scope: 'commits', per_page: 1) }
let(:path_for_multiple) { search_path(search: '*', scope: 'commits', per_page: 5) } let(:path_for_multiple) { search_path(search: '*', scope: 'commits', per_page: 5) }
it_behaves_like 'advanced search timeouts' do
let(:path) { path_for_one }
end
it 'avoids N+1 database queries' do it 'avoids N+1 database queries' do
project.repository.index_commits_and_blobs project.repository.index_commits_and_blobs
......
...@@ -14,6 +14,7 @@ RSpec.describe Peek::Views::Elasticsearch, :elastic, :request_store do ...@@ -14,6 +14,7 @@ RSpec.describe Peek::Views::Elasticsearch, :elastic, :request_store do
describe '#results' do describe '#results' do
let(:results) { described_class.new.results } let(:results) { described_class.new.results }
let(:project) { create(:project, :repository) } let(:project) { create(:project, :repository) }
let(:timeout) { '30s' }
it 'includes performance details' do it 'includes performance details' do
::Gitlab::SafeRequestStore.clear! ::Gitlab::SafeRequestStore.clear!
...@@ -24,9 +25,9 @@ RSpec.describe Peek::Views::Elasticsearch, :elastic, :request_store do ...@@ -24,9 +25,9 @@ RSpec.describe Peek::Views::Elasticsearch, :elastic, :request_store do
expect(results[:duration]).to be_kind_of(String) expect(results[:duration]).to be_kind_of(String)
expect(results[:details].last[:method]).to eq('GET') expect(results[:details].last[:method]).to eq('GET')
expect(results[:details].last[:path]).to eq('gitlab-test/doc/_search') expect(results[:details].last[:path]).to eq('gitlab-test/doc/_search')
expect(results[:details].last[:params]).to eq({ routing: "project_#{project.id}" }) expect(results[:details].last[:params]).to eq({ routing: "project_#{project.id}", timeout: timeout })
expect(results[:details].last[:request]).to eq("GET gitlab-test/doc/_search?routing=project_#{project.id}") expect(results[:details].last[:request]).to eq("GET gitlab-test/doc/_search?routing=project_#{project.id}&timeout=#{timeout}")
end end
end end
end end
...@@ -13,7 +13,10 @@ RSpec.shared_examples 'does not hit Elasticsearch twice for objects and counts' ...@@ -13,7 +13,10 @@ RSpec.shared_examples 'does not hit Elasticsearch twice for objects and counts'
results.objects(scope) results.objects(scope)
results.public_send("#{scope}_count") results.public_send("#{scope}_count")
request = ::Gitlab::Instrumentation::ElasticsearchTransport.detail_store.first
expect(::Gitlab::Instrumentation::ElasticsearchTransport.get_request_count).to eq(1) expect(::Gitlab::Instrumentation::ElasticsearchTransport.get_request_count).to eq(1)
expect(request.dig(:params, :timeout)).to eq('30s')
end end
end 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