Commit d417dfd0 authored by Stan Hu's avatar Stan Hu

Merge branch 'mk-front-load-rails-slis' into 'master'

Move Rails SLI initialization to boot path

See merge request gitlab-org/gitlab!80059
parents 589c81ec a461670e
......@@ -7,7 +7,6 @@ class MetricsController < ActionController::Base
def index
response = if Gitlab::Metrics.prometheus_metrics_enabled?
Gitlab::Metrics::RailsSlis.initialize_request_slis_if_needed!
metrics_service.metrics_text
else
help_page = help_page_url('administration/monitoring/prometheus/gitlab_metrics',
......
......@@ -57,31 +57,10 @@ Gitlab::Metrics::Sli.initialize_sli(:received_email, [
])
```
Metrics must be initialized before they get
scraped for the first time. This could be done at the start time of the
process that will emit them, in which case we need to pay attention
not to increase application's boot time too much. This is preferable
if possible.
Alternatively, if initializing would take too long, this can be done
during the first scrape. We need to make sure we don't do it for every
scrape. This can be done as follows:
```ruby
def initialize_request_slis_if_needed!
return if Gitlab::Metrics::Sli.initialized?(:rails_request_apdex)
Gitlab::Metrics::Sli.initialize_sli(:rails_request_apdex, possible_request_labels)
end
```
Also pay attention to do it for the different metrics
endpoints we have. Currently the
[`WebExporter`](https://gitlab.com/gitlab-org/gitlab/blob/master/lib/gitlab/metrics/exporter/web_exporter.rb)
and the
[`HealthController`](https://gitlab.com/gitlab-org/gitlab/blob/master/app/controllers/health_controller.rb)
for Rails and
[`SidekiqExporter`](https://gitlab.com/gitlab-org/gitlab/blob/master/lib/gitlab/metrics/exporter/sidekiq_exporter.rb)
for Sidekiq.
Metrics must be initialized before they get scraped for the first time.
This currently happens during the `on_master_start` [life-cycle event](https://gitlab.com/gitlab-org/gitlab/blob/master/lib/gitlab/cluster/lifecycle_events.rb).
Since this delays application readiness until metrics initialization returns, make sure the overhead
this adds is understood and acceptable.
## Tracking operations for an SLI
......
......@@ -4,29 +4,10 @@ module Gitlab
module Metrics
module Exporter
class WebExporter < BaseExporter
RailsMetricsInitializer = Struct.new(:app) do
def call(env)
Gitlab::Metrics::RailsSlis.initialize_request_slis_if_needed!
app.call(env)
end
end
# This exporter is always run on master process
def initialize(**options)
super(Settings.monitoring.web_exporter, log_enabled: true, log_file: 'web_exporter.log', **options)
end
private
def rack_app
app = super
Rack::Builder.app do
use RailsMetricsInitializer
run app
end
end
end
end
end
......
......@@ -4,7 +4,7 @@ module Gitlab
module Metrics
module RailsSlis
class << self
def initialize_request_slis_if_needed!
def initialize_request_slis!
Gitlab::Metrics::Sli.initialize_sli(:rails_request_apdex, possible_request_labels) unless Gitlab::Metrics::Sli.initialized?(:rails_request_apdex)
Gitlab::Metrics::Sli.initialize_sli(:graphql_query_apdex, possible_graphql_query_labels) unless Gitlab::Metrics::Sli.initialized?(:graphql_query_apdex)
end
......
......@@ -62,6 +62,8 @@ module Gitlab
http_requests_total.get({ method: method, status: status, feature_category: feature_category })
end
end
Gitlab::Metrics::RailsSlis.initialize_request_slis!
end
def call(env)
......
......@@ -67,12 +67,6 @@ RSpec.describe MetricsController, :request_store do
expect(response.body).to match(/^prometheus_counter 1$/)
end
it 'initializes the rails request SLIs' do
expect(Gitlab::Metrics::RailsSlis).to receive(:initialize_request_slis_if_needed!).and_call_original
get :index
end
context 'prometheus metrics are disabled' do
before do
allow(Gitlab::Metrics).to receive(:prometheus_metrics_enabled?).and_return(false)
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe Gitlab::Metrics::Exporter::WebExporter do
let(:exporter) { described_class.new }
before do
stub_config(
monitoring: {
web_exporter: {
enabled: true,
port: 0,
address: '127.0.0.1'
}
}
)
exporter.start
end
after do
exporter.stop
end
context 'when running server', :prometheus do
it 'initializes request metrics' do
expect(Gitlab::Metrics::RailsSlis).to receive(:initialize_request_slis_if_needed!).and_call_original
http = Net::HTTP.new(exporter.server.config[:BindAddress], exporter.server.config[:Port])
response = http.request(Net::HTTP::Get.new('/metrics'))
expect(response.body).to include('gitlab_sli:rails_request_apdex')
end
end
end
......@@ -13,7 +13,7 @@ RSpec.describe Gitlab::Metrics::RailsSlis do
allow(Gitlab::Graphql::KnownOperations).to receive(:default).and_return(Gitlab::Graphql::KnownOperations.new(%w(foo bar)))
end
describe '.initialize_request_slis_if_needed!' do
describe '.initialize_request_slis!' do
it "initializes the SLI for all possible endpoints if they weren't", :aggregate_failures do
possible_labels = [
{
......@@ -41,7 +41,7 @@ RSpec.describe Gitlab::Metrics::RailsSlis do
expect(Gitlab::Metrics::Sli).to receive(:initialize_sli).with(:rails_request_apdex, array_including(*possible_labels)).and_call_original
expect(Gitlab::Metrics::Sli).to receive(:initialize_sli).with(:graphql_query_apdex, array_including(*possible_graphql_labels)).and_call_original
described_class.initialize_request_slis_if_needed!
described_class.initialize_request_slis!
end
it 'does not initialize the SLI if they were initialized already', :aggregate_failures do
......@@ -49,13 +49,13 @@ RSpec.describe Gitlab::Metrics::RailsSlis do
expect(Gitlab::Metrics::Sli).to receive(:initialized?).with(:graphql_query_apdex) { true }
expect(Gitlab::Metrics::Sli).not_to receive(:initialize_sli)
described_class.initialize_request_slis_if_needed!
described_class.initialize_request_slis!
end
end
describe '.request_apdex' do
it 'returns the initialized request apdex SLI object' do
described_class.initialize_request_slis_if_needed!
described_class.initialize_request_slis!
expect(described_class.request_apdex).to be_initialized
end
......@@ -63,7 +63,7 @@ RSpec.describe Gitlab::Metrics::RailsSlis do
describe '.graphql_query_apdex' do
it 'returns the initialized request apdex SLI object' do
described_class.initialize_request_slis_if_needed!
described_class.initialize_request_slis!
expect(described_class.graphql_query_apdex).to be_initialized
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