Merge branch '60024-remove-restclient-from-prom-client' into 'master'
Resolve "Use Gitlab::HTTP in PrometheusClient instead of RestClient" Closes #60024 See merge request gitlab-org/gitlab-ce!31053
This commit is contained in:
commit
4bbf942010
17 changed files with 112 additions and 79 deletions
|
@ -83,7 +83,7 @@ module Clusters
|
|||
|
||||
# ensures headers containing auth data are appended to original k8s client options
|
||||
options = kube_client.rest_client.options.merge(headers: kube_client.headers)
|
||||
RestClient::Resource.new(proxy_url, options)
|
||||
Gitlab::PrometheusClient.new(proxy_url, options)
|
||||
rescue Kubeclient::HttpError
|
||||
# If users have mistakenly set parameters or removed the depended clusters,
|
||||
# `proxy_url` could raise an exception because gitlab can not communicate with the cluster.
|
||||
|
|
|
@ -14,10 +14,6 @@ module PrometheusAdapter
|
|||
raise NotImplementedError
|
||||
end
|
||||
|
||||
def prometheus_client_wrapper
|
||||
Gitlab::PrometheusClient.new(prometheus_client)
|
||||
end
|
||||
|
||||
def can_query?
|
||||
prometheus_client.present?
|
||||
end
|
||||
|
@ -35,7 +31,7 @@ module PrometheusAdapter
|
|||
def calculate_reactive_cache(query_class_name, *args)
|
||||
return unless prometheus_client
|
||||
|
||||
data = Object.const_get(query_class_name, false).new(prometheus_client_wrapper).query(*args)
|
||||
data = Object.const_get(query_class_name, false).new(prometheus_client).query(*args)
|
||||
{
|
||||
success: true,
|
||||
data: data,
|
||||
|
|
|
@ -63,15 +63,16 @@ class PrometheusService < MonitoringService
|
|||
|
||||
# Check we can connect to the Prometheus API
|
||||
def test(*args)
|
||||
Gitlab::PrometheusClient.new(prometheus_client).ping
|
||||
|
||||
prometheus_client.ping
|
||||
{ success: true, result: 'Checked API endpoint' }
|
||||
rescue Gitlab::PrometheusClient::Error => err
|
||||
{ success: false, result: err }
|
||||
end
|
||||
|
||||
def prometheus_client
|
||||
RestClient::Resource.new(api_url, max_redirects: 0) if should_return_client?
|
||||
return unless should_return_client?
|
||||
|
||||
Gitlab::PrometheusClient.new(api_url)
|
||||
end
|
||||
|
||||
def prometheus_available?
|
||||
|
@ -84,7 +85,7 @@ class PrometheusService < MonitoringService
|
|||
private
|
||||
|
||||
def should_return_client?
|
||||
api_url && manual_configuration? && active? && valid?
|
||||
api_url.present? && manual_configuration? && active? && valid?
|
||||
end
|
||||
|
||||
def synchronize_service_state
|
||||
|
|
|
@ -98,7 +98,7 @@ module Prometheus
|
|||
end
|
||||
|
||||
def prometheus_client_wrapper
|
||||
prometheus_adapter&.prometheus_client_wrapper
|
||||
prometheus_adapter&.prometheus_client
|
||||
end
|
||||
|
||||
def can_query?
|
||||
|
|
|
@ -3,6 +3,7 @@
|
|||
module Gitlab
|
||||
# Helper methods to interact with Prometheus network services & resources
|
||||
class PrometheusClient
|
||||
include Gitlab::Utils::StrongMemoize
|
||||
Error = Class.new(StandardError)
|
||||
QueryError = Class.new(Gitlab::PrometheusClient::Error)
|
||||
|
||||
|
@ -14,10 +15,17 @@ module Gitlab
|
|||
# Minimal value of the `step` parameter for `query_range` in seconds.
|
||||
QUERY_RANGE_MIN_STEP = 60
|
||||
|
||||
attr_reader :rest_client, :headers
|
||||
# Key translation between RestClient and Gitlab::HTTP (HTTParty)
|
||||
RESTCLIENT_GITLAB_HTTP_KEYMAP = {
|
||||
ssl_cert_store: :cert_store
|
||||
}.freeze
|
||||
|
||||
def initialize(rest_client)
|
||||
@rest_client = rest_client
|
||||
attr_reader :api_url, :options
|
||||
private :api_url, :options
|
||||
|
||||
def initialize(api_url, options = {})
|
||||
@api_url = api_url.chomp('/')
|
||||
@options = options
|
||||
end
|
||||
|
||||
def ping
|
||||
|
@ -27,14 +35,10 @@ module Gitlab
|
|||
def proxy(type, args)
|
||||
path = api_path(type)
|
||||
get(path, args)
|
||||
rescue RestClient::ExceptionWithResponse => ex
|
||||
if ex.response
|
||||
ex.response
|
||||
else
|
||||
raise PrometheusClient::Error, "Network connection error"
|
||||
end
|
||||
rescue RestClient::Exception
|
||||
raise PrometheusClient::Error, "Network connection error"
|
||||
rescue Gitlab::HTTP::ResponseError => ex
|
||||
raise PrometheusClient::Error, "Network connection error" unless ex.response && ex.response.try(:code)
|
||||
|
||||
handle_response(ex.response)
|
||||
end
|
||||
|
||||
def query(query, time: Time.now)
|
||||
|
@ -78,50 +82,58 @@ module Gitlab
|
|||
private
|
||||
|
||||
def api_path(type)
|
||||
['api', 'v1', type].join('/')
|
||||
[api_url, 'api', 'v1', type].join('/')
|
||||
end
|
||||
|
||||
def json_api_get(type, args = {})
|
||||
path = api_path(type)
|
||||
response = get(path, args)
|
||||
handle_response(response)
|
||||
rescue RestClient::ExceptionWithResponse => ex
|
||||
if ex.response
|
||||
handle_exception_response(ex.response)
|
||||
else
|
||||
raise PrometheusClient::Error, "Network connection error"
|
||||
rescue Gitlab::HTTP::ResponseError => ex
|
||||
raise PrometheusClient::Error, "Network connection error" unless ex.response && ex.response.try(:code)
|
||||
|
||||
handle_response(ex.response)
|
||||
end
|
||||
|
||||
def gitlab_http_key(key)
|
||||
RESTCLIENT_GITLAB_HTTP_KEYMAP[key] || key
|
||||
end
|
||||
|
||||
def mapped_options
|
||||
options.keys.map { |k| [gitlab_http_key(k), options[k]] }.to_h
|
||||
end
|
||||
|
||||
def http_options
|
||||
strong_memoize(:http_options) do
|
||||
{ follow_redirects: false }.merge(mapped_options)
|
||||
end
|
||||
rescue RestClient::Exception
|
||||
raise PrometheusClient::Error, "Network connection error"
|
||||
end
|
||||
|
||||
def get(path, args)
|
||||
rest_client[path].get(params: args)
|
||||
Gitlab::HTTP.get(path, { query: args }.merge(http_options) )
|
||||
rescue SocketError
|
||||
raise PrometheusClient::Error, "Can't connect to #{rest_client.url}"
|
||||
raise PrometheusClient::Error, "Can't connect to #{api_url}"
|
||||
rescue OpenSSL::SSL::SSLError
|
||||
raise PrometheusClient::Error, "#{rest_client.url} contains invalid SSL data"
|
||||
raise PrometheusClient::Error, "#{api_url} contains invalid SSL data"
|
||||
rescue Errno::ECONNREFUSED
|
||||
raise PrometheusClient::Error, 'Connection refused'
|
||||
end
|
||||
|
||||
def handle_response(response)
|
||||
json_data = parse_json(response.body)
|
||||
if response.code == 200 && json_data['status'] == 'success'
|
||||
json_data['data'] || {}
|
||||
else
|
||||
raise PrometheusClient::Error, "#{response.code} - #{response.body}"
|
||||
end
|
||||
end
|
||||
response_code = response.try(:code)
|
||||
response_body = response.try(:body)
|
||||
|
||||
def handle_exception_response(response)
|
||||
if response.code == 200 && response['status'] == 'success'
|
||||
response['data'] || {}
|
||||
elsif response.code == 400
|
||||
json_data = parse_json(response.body)
|
||||
raise PrometheusClient::Error, "#{response_code} - #{response_body}" unless response_code
|
||||
|
||||
json_data = parse_json(response_body) if [200, 400].include?(response_code)
|
||||
|
||||
case response_code
|
||||
when 200
|
||||
json_data['data'] if response['status'] == 'success'
|
||||
when 400
|
||||
raise PrometheusClient::QueryError, json_data['error'] || 'Bad data received'
|
||||
else
|
||||
raise PrometheusClient::Error, "#{response.code} - #{response.body}"
|
||||
raise PrometheusClient::Error, "#{response_code} - #{response_body}"
|
||||
end
|
||||
end
|
||||
|
||||
|
|
|
@ -67,7 +67,7 @@ module Sentry
|
|||
|
||||
def handle_request_exceptions
|
||||
yield
|
||||
rescue HTTParty::Error => e
|
||||
rescue Gitlab::HTTP::Error => e
|
||||
Gitlab::Sentry.track_acceptable_exception(e)
|
||||
raise_error 'Error when connecting to Sentry'
|
||||
rescue Net::OpenTimeout
|
||||
|
|
|
@ -61,7 +61,7 @@ describe 'User activates issue tracker', :js do
|
|||
|
||||
context 'when the connection test fails' do
|
||||
it 'activates the service' do
|
||||
stub_request(:head, url).to_raise(HTTParty::Error)
|
||||
stub_request(:head, url).to_raise(Gitlab::HTTP::Error)
|
||||
|
||||
click_link(tracker)
|
||||
|
||||
|
|
|
@ -48,7 +48,7 @@ describe 'User activates issue tracker', :js do
|
|||
|
||||
context 'when the connection test fails' do
|
||||
it 'activates the service' do
|
||||
stub_request(:head, url).to_raise(HTTParty::Error)
|
||||
stub_request(:head, url).to_raise(Gitlab::HTTP::Error)
|
||||
|
||||
click_link(tracker)
|
||||
fill_form
|
||||
|
|
|
@ -173,7 +173,7 @@ describe Gitlab::BitbucketImport::Importer do
|
|||
context 'when importing a pull request throws an exception' do
|
||||
before do
|
||||
allow(pull_request).to receive(:raw).and_return('hello world')
|
||||
allow(subject.client).to receive(:pull_request_comments).and_raise(HTTParty::Error)
|
||||
allow(subject.client).to receive(:pull_request_comments).and_raise(Gitlab::HTTP::Error)
|
||||
end
|
||||
|
||||
it 'logs an error without the backtrace' do
|
||||
|
|
|
@ -3,7 +3,7 @@ require 'spec_helper'
|
|||
describe Gitlab::PrometheusClient do
|
||||
include PrometheusHelpers
|
||||
|
||||
subject { described_class.new(RestClient::Resource.new('https://prometheus.example.com')) }
|
||||
subject { described_class.new('https://prometheus.example.com') }
|
||||
|
||||
describe '#ping' do
|
||||
it 'issues a "query" request to the API endpoint' do
|
||||
|
@ -79,8 +79,16 @@ describe Gitlab::PrometheusClient do
|
|||
expect(req_stub).to have_been_requested
|
||||
end
|
||||
|
||||
it 'raises a Gitlab::PrometheusClient::Error error when a RestClient::Exception is rescued' do
|
||||
req_stub = stub_prometheus_request_with_exception(prometheus_url, RestClient::Exception)
|
||||
it 'raises a Gitlab::PrometheusClient::Error error when a Gitlab::HTTP::ResponseError is rescued' do
|
||||
req_stub = stub_prometheus_request_with_exception(prometheus_url, Gitlab::HTTP::ResponseError)
|
||||
|
||||
expect { subject }
|
||||
.to raise_error(Gitlab::PrometheusClient::Error, "Network connection error")
|
||||
expect(req_stub).to have_been_requested
|
||||
end
|
||||
|
||||
it 'raises a Gitlab::PrometheusClient::Error error when a Gitlab::HTTP::ResponseError with a code is rescued' do
|
||||
req_stub = stub_prometheus_request_with_exception(prometheus_url, Gitlab::HTTP::ResponseError.new(code: 400))
|
||||
|
||||
expect { subject }
|
||||
.to raise_error(Gitlab::PrometheusClient::Error, "Network connection error")
|
||||
|
@ -89,13 +97,13 @@ describe Gitlab::PrometheusClient do
|
|||
end
|
||||
|
||||
context 'ping' do
|
||||
subject { described_class.new(RestClient::Resource.new(prometheus_url)).ping }
|
||||
subject { described_class.new(prometheus_url).ping }
|
||||
|
||||
it_behaves_like 'exceptions are raised'
|
||||
end
|
||||
|
||||
context 'proxy' do
|
||||
subject { described_class.new(RestClient::Resource.new(prometheus_url)).proxy('query', { query: '1' }) }
|
||||
subject { described_class.new(prometheus_url).proxy('query', { query: '1' }) }
|
||||
|
||||
it_behaves_like 'exceptions are raised'
|
||||
end
|
||||
|
@ -310,17 +318,34 @@ describe Gitlab::PrometheusClient do
|
|||
end
|
||||
end
|
||||
|
||||
context 'when RestClient::Exception is raised' do
|
||||
context 'when Gitlab::HTTP::ResponseError is raised' do
|
||||
before do
|
||||
stub_prometheus_request_with_exception(query_url, RestClient::Exception)
|
||||
stub_prometheus_request_with_exception(query_url, response_error)
|
||||
end
|
||||
|
||||
context "without response code" do
|
||||
let(:response_error) { Gitlab::HTTP::ResponseError }
|
||||
it 'raises PrometheusClient::Error' do
|
||||
expect { subject.proxy('query', { query: prometheus_query }) }.to(
|
||||
raise_error(Gitlab::PrometheusClient::Error, 'Network connection error')
|
||||
)
|
||||
end
|
||||
end
|
||||
|
||||
context "with response code" do
|
||||
let(:response_error) do
|
||||
response = Net::HTTPResponse.new(1.1, 400, '{}sumpthin')
|
||||
allow(response).to receive(:body) { '{}' }
|
||||
Gitlab::HTTP::ResponseError.new(response)
|
||||
end
|
||||
|
||||
it 'raises Gitlab::PrometheusClient::QueryError' do
|
||||
expect { subject.proxy('query', { query: prometheus_query }) }.to(
|
||||
raise_error(Gitlab::PrometheusClient::QueryError, 'Bad data received')
|
||||
)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -63,7 +63,7 @@ describe Sentry::Client do
|
|||
|
||||
shared_examples 'maps exceptions' do
|
||||
exceptions = {
|
||||
HTTParty::Error => 'Error when connecting to Sentry',
|
||||
Gitlab::HTTP::Error => 'Error when connecting to Sentry',
|
||||
Net::OpenTimeout => 'Connection to Sentry timed out',
|
||||
SocketError => 'Received SocketError when trying to connect to Sentry',
|
||||
OpenSSL::SSL::SSLError => 'Sentry returned invalid SSL data',
|
||||
|
|
|
@ -86,16 +86,15 @@ describe Clusters::Applications::Prometheus do
|
|||
project: cluster.cluster_project.project)
|
||||
end
|
||||
|
||||
it 'creates proxy prometheus rest client' do
|
||||
expect(subject.prometheus_client).to be_instance_of(RestClient::Resource)
|
||||
it 'creates proxy prometheus_client' do
|
||||
expect(subject.prometheus_client).to be_instance_of(Gitlab::PrometheusClient)
|
||||
end
|
||||
|
||||
it 'creates proper url' do
|
||||
expect(subject.prometheus_client.url).to eq("#{kubernetes_url}/api/v1/namespaces/gitlab-managed-apps/services/prometheus-prometheus-server:80/proxy")
|
||||
end
|
||||
|
||||
it 'copies options and headers from kube client to proxy client' do
|
||||
expect(subject.prometheus_client.options).to eq(kube_client.rest_client.options.merge(headers: kube_client.headers))
|
||||
it 'copies proxy_url, options and headers from kube client to prometheus_client' do
|
||||
expect(Gitlab::PrometheusClient)
|
||||
.to(receive(:new))
|
||||
.with(a_valid_url, kube_client.rest_client.options.merge(headers: kube_client.headers))
|
||||
subject.prometheus_client
|
||||
end
|
||||
|
||||
context 'when cluster is not reachable' do
|
||||
|
|
|
@ -40,13 +40,13 @@ describe PrometheusAdapter, :use_clean_rails_memory_store_caching do
|
|||
|
||||
describe 'matched_metrics' do
|
||||
let(:matched_metrics_query) { Gitlab::Prometheus::Queries::MatchedMetricQuery }
|
||||
let(:prometheus_client_wrapper) { double(:prometheus_client_wrapper, label_values: nil) }
|
||||
let(:prometheus_client) { double(:prometheus_client, label_values: nil) }
|
||||
|
||||
context 'with valid data' do
|
||||
subject { service.query(:matched_metrics) }
|
||||
|
||||
before do
|
||||
allow(service).to receive(:prometheus_client_wrapper).and_return(prometheus_client_wrapper)
|
||||
allow(service).to receive(:prometheus_client).and_return(prometheus_client)
|
||||
synchronous_reactive_cache(service)
|
||||
end
|
||||
|
||||
|
|
|
@ -105,10 +105,6 @@ describe PrometheusService, :use_clean_rails_memory_store_caching do
|
|||
context 'manual configuration is enabled' do
|
||||
let(:manual_configuration) { true }
|
||||
|
||||
it 'returns rest client from api_url' do
|
||||
expect(service.prometheus_client.url).to eq(api_url)
|
||||
end
|
||||
|
||||
it 'calls valid?' do
|
||||
allow(service).to receive(:valid?).and_call_original
|
||||
|
||||
|
|
|
@ -33,7 +33,7 @@ describe Projects::LfsPointers::LfsDownloadLinkListService do
|
|||
|
||||
before do
|
||||
allow(project).to receive(:lfs_enabled?).and_return(true)
|
||||
response = instance_double(HTTParty::Response)
|
||||
response = instance_double(Gitlab::HTTP::Response)
|
||||
allow(response).to receive(:body).and_return(objects_response.to_json)
|
||||
allow(response).to receive(:success?).and_return(true)
|
||||
allow(Gitlab::HTTP).to receive(:post).and_return(response)
|
||||
|
@ -95,7 +95,7 @@ describe Projects::LfsPointers::LfsDownloadLinkListService do
|
|||
|
||||
shared_examples 'JSON parse errors' do |body|
|
||||
it 'raises error' do
|
||||
response = instance_double(HTTParty::Response)
|
||||
response = instance_double(Gitlab::HTTP::Response)
|
||||
allow(response).to receive(:body).and_return(body)
|
||||
allow(response).to receive(:success?).and_return(true)
|
||||
allow(Gitlab::HTTP).to receive(:post).and_return(response)
|
||||
|
|
|
@ -131,7 +131,7 @@ describe Prometheus::ProxyService do
|
|||
allow(environment).to receive(:prometheus_adapter)
|
||||
.and_return(prometheus_adapter)
|
||||
allow(prometheus_adapter).to receive(:can_query?).and_return(true)
|
||||
allow(prometheus_adapter).to receive(:prometheus_client_wrapper)
|
||||
allow(prometheus_adapter).to receive(:prometheus_client)
|
||||
.and_return(prometheus_client)
|
||||
end
|
||||
|
||||
|
|
|
@ -5,3 +5,7 @@ RSpec::Matchers.define :be_url do |_|
|
|||
URI.parse(actual) rescue false
|
||||
end
|
||||
end
|
||||
|
||||
# looks better when used like:
|
||||
# expect(thing).to receive(:method).with(a_valid_url)
|
||||
RSpec::Matchers.alias_matcher :a_valid_url, :be_url
|
||||
|
|
Loading…
Reference in a new issue