Commit 4ae05e3b authored by Dylan Griffith's avatar Dylan Griffith

Fix problem with Elasticsearch timed_out check for index_exists?

We previously implemented the `timed_out` count in
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/53955/diffs
but it had to be reverted due to it not being able to handle an empty
response body
https://gitlab.com/gitlab-com/gl-infra/production/-/issues/3623
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/54053 .

It was very easy to reproduce the problem observed in that by simply:

1. Visit `Admin > Settings > Preferences`
2. Update the email additional attributes
3. Save

This problem will actually happen when updating anything in the admin
UI.

This was quite surprising as we have tonnes of tests that go down this
code path throughout GitLab. As such it was very strange to not see a
single test failure outside of these QA tests that ran against staging.
It turns out the reason is that this code only runs when
`SafeRequestStore` is active otherwise the interceptor logic is skipped.
It turns out that none of our tests that execute these code paths had
the `SafeRequestStore` active. In an effort to reduce the risk of this
happening I've actived the `request_store` in the `helper_spec.rb` which
covers many of the Elasticsearch queries we execute which would have
actually caught this problem

Additionally I've updated the unit test with this new specific case and
another slightly similar case of error responses just to be safe.
parent 03f53df3
---
title: Track elasticsearch_timed_out_count for all ES requests in log
merge_request: 53955
merge_request: 54180
author:
type: changed
......@@ -2,7 +2,7 @@
require 'spec_helper'
RSpec.describe Gitlab::Elastic::Helper do
RSpec.describe Gitlab::Elastic::Helper, :request_store do
subject(:helper) { described_class.default }
shared_context 'with a legacy index' do
......
......@@ -105,4 +105,21 @@ RSpec.describe ::Gitlab::Instrumentation::ElasticsearchTransportInterceptor, :el
expect(::Gitlab::Instrumentation::ElasticsearchTransport.get_timed_out_count).to eq(0)
end
end
context 'when the server returns a blank response body' do
it 'does not error' do
stub_request(:any, /#{elasticsearch_url}/).to_return(body: +'', status: 200)
Project.__elasticsearch__.client.perform_request(:get, '/')
end
end
context 'when the request raises some error' do
it 'does not raise a different error in ensure' do
stub_request(:any, /#{elasticsearch_url}/).to_return(body: +'', status: 500)
expect { Project.__elasticsearch__.client.perform_request(:get, '/') }
.to raise_error(::Elasticsearch::Transport::Transport::Errors::InternalServerError)
end
end
end
......@@ -16,7 +16,9 @@ module Gitlab
::Gitlab::Instrumentation::ElasticsearchTransport.increment_request_count
::Gitlab::Instrumentation::ElasticsearchTransport.increment_timed_out_count if response&.body&.dig('timed_out')
if response&.body && response.body.is_a?(Hash) && response.body['timed_out']
::Gitlab::Instrumentation::ElasticsearchTransport.increment_timed_out_count
end
::Gitlab::Instrumentation::ElasticsearchTransport.add_duration(duration)
::Gitlab::Instrumentation::ElasticsearchTransport.add_call_details(duration, method, path, params, body)
......
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