diff --git a/config/initializers/database_query_analyzers.rb b/config/initializers/database_query_analyzers.rb index b2ddb6bccd1bb17a84d5babd92e2636fc776437e..7b570fcfb29da5ad35d918de0b7ccd4cb63b7971 100644 --- a/config/initializers/database_query_analyzers.rb +++ b/config/initializers/database_query_analyzers.rb @@ -3,4 +3,8 @@ # Currently we register validator only for `dev` or `test` environment if Gitlab.dev_or_test_env? || Gitlab::Utils.to_boolean('GITLAB_ENABLE_QUERY_ANALYZERS', default: false) Gitlab::Database::QueryAnalyzer.instance.hook! + + Gitlab::Application.configure do |config| + config.middleware.use(Gitlab::Middleware::QueryAnalyzer) + end end diff --git a/lib/gitlab/database/query_analyzer.rb b/lib/gitlab/database/query_analyzer.rb index 6cbeefa0aae5960b205411f2ea31c8c7da838de3..53de6697ca6103f3d8d9f8cdff9684d9c96ddb4e 100644 --- a/lib/gitlab/database/query_analyzer.rb +++ b/lib/gitlab/database/query_analyzer.rb @@ -4,45 +4,99 @@ module Gitlab module Database # The purpose of this class is to implement a various query analyzers based on `pg_query` # And process them all via `Gitlab::Database::QueryAnalyzers::*` + # + # Sometimes this might cause errors in specs. + # This is best to be disable with `describe '...', query_analyzers: false do` class QueryAnalyzer include ::Singleton - ANALYZERS = [].freeze - Parsed = Struct.new( :sql, :connection, :pg ) + attr_reader :all_analyzers + + def initialize + @all_analyzers = [] + end + def hook! @subscriber = ActiveSupport::Notifications.subscribe('sql.active_record') do |event| - process_sql(event.payload[:sql], event.payload[:connection]) + # In some cases analyzer code might trigger another SQL call + # to avoid stack too deep this detects recursive call of subscriber + with_ignored_recursive_calls do + process_sql(event.payload[:sql], event.payload[:connection]) + end end end - private + def within + # Due to singleton nature of analyzers + # only an outer invocation of the `.within` + # is allowed to initialize them + return yield if already_within? + + begin! + + begin + yield + ensure + end! + end + end + + def already_within? + # If analyzers are set they are already configured + !enabled_analyzers.nil? + end def process_sql(sql, connection) - analyzers = enabled_analyzers(connection) - return unless analyzers.any? + analyzers = enabled_analyzers + return unless analyzers&.any? parsed = parse(sql, connection) return unless parsed analyzers.each do |analyzer| analyzer.analyze(parsed) - rescue => e # rubocop:disable Style/RescueStandardError + rescue StandardError => e # We catch all standard errors to prevent validation errors to introduce fatal errors in production Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e) end end - def enabled_analyzers(connection) - ANALYZERS.select do |analyzer| - analyzer.enabled?(connection) - rescue StandardError => e # rubocop:disable Style/RescueStandardError - # We catch all standard errors to prevent validation errors to introduce fatal errors in production + private + + # Enable query analyzers + def begin! + analyzers = all_analyzers.select do |analyzer| + if analyzer.enabled? + analyzer.begin! + + true + end + rescue StandardError => e + Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e) + + false + end + + Thread.current[:query_analyzer_enabled_analyzers] = analyzers + end + + # Disable enabled query analyzers + def end! + enabled_analyzers.select do |analyzer| + analyzer.end! + rescue StandardError => e Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e) end + + Thread.current[:query_analyzer_enabled_analyzers] = nil + end + + def enabled_analyzers + Thread.current[:query_analyzer_enabled_analyzers] end def parse(sql, connection) @@ -57,6 +111,17 @@ def parse(sql, connection) nil end + + def with_ignored_recursive_calls + return if Thread.current[:query_analyzer_recursive] + + begin + Thread.current[:query_analyzer_recursive] = true + yield + ensure + Thread.current[:query_analyzer_recursive] = nil + end + end end end end diff --git a/lib/gitlab/database/query_analyzers/base.rb b/lib/gitlab/database/query_analyzers/base.rb index e1be889c803ea609a38451bb0cdfa7c989f783ab..d3a20523e741745f2da6b87bbade31da47ab14c1 100644 --- a/lib/gitlab/database/query_analyzers/base.rb +++ b/lib/gitlab/database/query_analyzers/base.rb @@ -4,7 +4,19 @@ module Gitlab module Database module QueryAnalyzers class Base - def self.enabled?(connection) + def self.begin! + Thread.current[self.class.name] = {} + end + + def self.end! + Thread.current[self.class.name] = nil + end + + def self.context + Thread.current[self.class.name] + end + + def self.enabled? raise NotImplementedError end diff --git a/lib/gitlab/middleware/query_analyzer.rb b/lib/gitlab/middleware/query_analyzer.rb new file mode 100644 index 0000000000000000000000000000000000000000..8d63c644a6985c5a675e995ac86f5c2605c0ef67 --- /dev/null +++ b/lib/gitlab/middleware/query_analyzer.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +module Gitlab + module Middleware + class QueryAnalyzer + def initialize(app) + @app = app + end + + def call(env) + ::Gitlab::Database::QueryAnalyzer.instance.within { @app.call(env) } + end + end + end +end diff --git a/lib/gitlab/sidekiq_middleware.rb b/lib/gitlab/sidekiq_middleware.rb index c97b1632bf80739b0fbcbd309707a5280e6e317b..69802fd6217e42683a62894088a524abdfafb291 100644 --- a/lib/gitlab/sidekiq_middleware.rb +++ b/lib/gitlab/sidekiq_middleware.rb @@ -33,6 +33,7 @@ def self.server_configurator(metrics: true, arguments_logger: true, memory_kille chain.add ::Gitlab::SidekiqMiddleware::BatchLoader chain.add ::Gitlab::SidekiqMiddleware::InstrumentationLogger chain.add ::Gitlab::SidekiqMiddleware::AdminMode::Server + chain.add ::Gitlab::SidekiqMiddleware::QueryAnalyzer if Gitlab.dev_or_test_env? || Gitlab::Utils.to_boolean(ENV['GITLAB_ENABLE_QUERY_ANALYZERS'], default: false) chain.add ::Gitlab::SidekiqVersioning::Middleware chain.add ::Gitlab::SidekiqStatus::ServerMiddleware chain.add ::Gitlab::SidekiqMiddleware::WorkerContext::Server diff --git a/lib/gitlab/sidekiq_middleware/query_analyzer.rb b/lib/gitlab/sidekiq_middleware/query_analyzer.rb new file mode 100644 index 0000000000000000000000000000000000000000..4478fcd359456ae756b40cb406b796acfe8399ff --- /dev/null +++ b/lib/gitlab/sidekiq_middleware/query_analyzer.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module Gitlab + module SidekiqMiddleware + class QueryAnalyzer + def call(worker, job, queue) + ::Gitlab::Database::QueryAnalyzer.instance.within { yield } + end + end + end +end diff --git a/spec/lib/gitlab/database/query_analyzer_spec.rb b/spec/lib/gitlab/database/query_analyzer_spec.rb index f0a3021927ba7b75c362a7cfe631a83e22050c54..16b73b8123fe5c23f1144c0265390d33aa55dbd3 100644 --- a/spec/lib/gitlab/database/query_analyzer_spec.rb +++ b/spec/lib/gitlab/database/query_analyzer_spec.rb @@ -2,11 +2,16 @@ require 'spec_helper' -RSpec.describe Gitlab::Database::QueryAnalyzer do +RSpec.describe Gitlab::Database::QueryAnalyzer, query_analyzers: false do let(:analyzer) { double(:query_analyzer) } + let(:disabled_analyzer) { double(:disabled_query_analyzer) } before do - stub_const('Gitlab::Database::QueryAnalyzer::ANALYZERS', [analyzer]) + allow(described_class.instance).to receive(:all_analyzers).and_return([analyzer, disabled_analyzer]) + allow(analyzer).to receive(:enabled?).and_return(true) + allow(analyzer).to receive(:begin!) + allow(analyzer).to receive(:end!) + allow(disabled_analyzer).to receive(:enabled?).and_return(false) end context 'the hook is enabled by default in specs' do @@ -17,7 +22,57 @@ expect(parsed.pg.tables).to eq(%w[projects]) end - Project.connection.execute("SELECT 1 FROM projects") + described_class.instance.within do + Project.connection.execute("SELECT 1 FROM projects") + end + end + + it 'does prevent recursive execution' do + expect(analyzer).to receive(:enabled?).and_return(true) + expect(analyzer).to receive(:analyze) do + Project.connection.execute("SELECT 1 FROM projects") + end + + described_class.instance.within do + Project.connection.execute("SELECT 1 FROM projects") + end + end + end + + describe '#within' do + context 'when it is already initialized' do + around do |example| + described_class.instance.within do + example.run + end + end + + it 'does not evaluate enabled? again do yield block' do + expect(analyzer).not_to receive(:enabled?) + + expect { |b| described_class.instance.within(&b) }.to yield_control + end + end + + context 'when initializer is enabled' do + before do + expect(analyzer).to receive(:enabled?).and_return(true) + end + + it 'calls begin! and end!' do + expect(analyzer).to receive(:begin!) + expect(analyzer).to receive(:end!) + + expect { |b| described_class.instance.within(&b) }.to yield_control + end + + it 'when begin! raises the end! is not called' do + expect(analyzer).to receive(:begin!).and_raise('exception') + expect(analyzer).not_to receive(:end!) + expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception) + + expect { |b| described_class.instance.within(&b) }.to yield_control + end end end @@ -63,9 +118,18 @@ expect { process_sql("SELECT 1 FROM projects") }.not_to raise_error end + it 'does call analyze only on enabled initializers' do + expect(analyzer).to receive(:analyze) + expect(disabled_analyzer).not_to receive(:analyze) + + expect { process_sql("SELECT 1 FROM projects") }.not_to raise_error + end + def process_sql(sql) - ApplicationRecord.load_balancer.read_write do |connection| - described_class.instance.send(:process_sql, sql, connection) + described_class.instance.within do + ApplicationRecord.load_balancer.read_write do |connection| + described_class.instance.process_sql(sql, connection) + end end end end diff --git a/spec/support/database/query_analyzer.rb b/spec/support/database/query_analyzer.rb new file mode 100644 index 0000000000000000000000000000000000000000..85fa55f81efd1ed8d20c7a668e7d41efe863a1b1 --- /dev/null +++ b/spec/support/database/query_analyzer.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +# With the usage of `describe '...', query_analyzers: false` +# can be disabled selectively + +RSpec.configure do |config| + config.around do |example| + if example.metadata.fetch(:query_analyzers, true) + ::Gitlab::Database::QueryAnalyzer.instance.within { example.run } + else + example.run + end + end +end