Merge branch 'ali/db-testing-pass-more-information-to-observers' into 'master'

DB testing: Refactor to have stateful observers per migration

See merge request gitlab-org/gitlab!65777

lib/gitlab/database/migrations/instrumentation.rb

@@ -9,8 +9,8 @@ module Gitlab
 
         attr_reader :observations
 
-        def initialize(observers = ::Gitlab::Database::Migrations::Observers.all_observers)
-          @observers = observers
+        def initialize(observer_classes = ::Gitlab::Database::Migrations::Observers.all_observers)
+          @observer_classes = observer_classes
           @observations = []
         end
 
@@ -18,9 +18,11 @@ module Gitlab
           observation = Observation.new(version, name)
           observation.success = true
 
+          observers = observer_classes.map { |c| c.new(observation) }
+
           exception = nil
 
-          on_each_observer { |observer| observer.before }
+          on_each_observer(observers) { |observer| observer.before }
 
           observation.walltime = Benchmark.realtime do
             yield
@@ -29,8 +31,8 @@ module Gitlab
             observation.success = false
           end
 
-          on_each_observer { |observer| observer.after }
-          on_each_observer { |observer| observer.record(observation) }
+          on_each_observer(observers) { |observer| observer.after }
+          on_each_observer(observers) { |observer| observer.record }
 
           record_observation(observation)
 
@@ -41,13 +43,13 @@ module Gitlab
 
         private
 
-        attr_reader :observers
+        attr_reader :observer_classes
 
         def record_observation(observation)
           @observations << observation
         end
 
-        def on_each_observer(&block)
+        def on_each_observer(observers, &block)
           observers.each do |observer|
             yield observer
           rescue StandardError => e

lib/gitlab/database/migrations/observers.rb

@@ -6,10 +6,10 @@ module Gitlab
       module Observers
         def self.all_observers
           [
-            TotalDatabaseSizeChange.new,
-            QueryStatistics.new,
-            QueryLog.new,
-            QueryDetails.new
+            TotalDatabaseSizeChange,
+            QueryStatistics,
+            QueryLog,
+            QueryDetails
           ]
         end
       end

lib/gitlab/database/migrations/observers/migration_observer.rb

@@ -5,10 +5,11 @@ module Gitlab
     module Migrations
       module Observers
         class MigrationObserver
-          attr_reader :connection
+          attr_reader :connection, :observation
 
-          def initialize
+          def initialize(observation)
             @connection = ActiveRecord::Base.connection
+            @observation = observation
           end
 
           def before
@@ -19,7 +20,7 @@ module Gitlab
             # implement in subclass
           end
 
-          def record(observation)
+          def record
             raise NotImplementedError, 'implement in subclass'
           end
         end

lib/gitlab/database/migrations/observers/query_details.rb

@@ -22,7 +22,7 @@ module Gitlab
             @file.close
           end
 
-          def record(observation)
+          def record
             File.rename(@file_path, File.join(Instrumentation::RESULT_DIR, "#{observation.version}_#{observation.name}-query-details.json"))
           end
 

lib/gitlab/database/migrations/observers/query_log.rb

@@ -17,7 +17,7 @@ module Gitlab
             @logger.close
           end
 
-          def record(observation)
+          def record
             File.rename(@log_file_path, File.join(Instrumentation::RESULT_DIR, "#{observation.version}_#{observation.name}.log"))
           end
         end

lib/gitlab/database/migrations/observers/query_statistics.rb

@@ -16,7 +16,7 @@ module Gitlab
             connection.execute('select pg_stat_statements_reset()')
           end
 
-          def record(observation)
+          def record
             return unless enabled?
 
             observation.query_statistics = connection.execute(<<~SQL)

lib/gitlab/database/migrations/observers/total_database_size_change.rb

@@ -13,7 +13,7 @@ module Gitlab
             @size_after = get_total_database_size
           end
 
-          def record(observation)
+          def record
             return unless @size_after && @size_before
 
             observation.total_database_size_change = @size_after - @size_before

spec/lib/gitlab/database/migrations/instrumentation_spec.rb

@@ -13,17 +13,27 @@ RSpec.describe Gitlab::Database::Migrations::Instrumentation do
     end
 
     context 'behavior with observers' do
-      subject { described_class.new(observers).observe(version: migration_version, name: migration_name) {} }
+      subject { described_class.new([Gitlab::Database::Migrations::Observers::MigrationObserver]).observe(version: migration_version, name: migration_name) {} }
 
-      let(:observers) { [observer] }
       let(:observer) { instance_double('Gitlab::Database::Migrations::Observers::MigrationObserver', before: nil, after: nil, record: nil) }
 
+      before do
+        allow(Gitlab::Database::Migrations::Observers::MigrationObserver).to receive(:new).and_return(observer)
+      end
+
+      it 'instantiates observer with observation' do
+        expect(Gitlab::Database::Migrations::Observers::MigrationObserver)
+          .to receive(:new)
+          .with(instance_of(Gitlab::Database::Migrations::Observation)) { |observation| expect(observation.version).to eq(migration_version) }
+          .and_return(observer)
+
+        subject
+      end
+
       it 'calls #before, #after, #record on given observers' do
         expect(observer).to receive(:before).ordered
         expect(observer).to receive(:after).ordered
-        expect(observer).to receive(:record).ordered do |observation|
-          expect(observation.version).to eq(migration_version)
-        end
+        expect(observer).to receive(:record).ordered
 
         subject
       end

spec/lib/gitlab/database/migrations/observers/query_details_spec.rb

@@ -2,7 +2,7 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Database::Migrations::Observers::QueryDetails do
-  subject { described_class.new }
+  subject { described_class.new(observation) }
 
   let(:observation) { Gitlab::Database::Migrations::Observation.new(migration_version, migration_name) }
   let(:connection) { ActiveRecord::Base.connection }
@@ -50,7 +50,7 @@ RSpec.describe Gitlab::Database::Migrations::Observers::QueryDetails do
     subject.before
     run_query
     subject.after
-    subject.record(observation)
+    subject.record
   end
 
   def run_query

spec/lib/gitlab/database/migrations/observers/query_log_spec.rb

@@ -2,7 +2,7 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Database::Migrations::Observers::QueryLog do
-  subject { described_class.new }
+  subject { described_class.new(observation) }
 
   let(:observation) { Gitlab::Database::Migrations::Observation.new(migration_version, migration_name) }
   let(:connection) { ActiveRecord::Base.connection }
@@ -34,6 +34,6 @@ RSpec.describe Gitlab::Database::Migrations::Observers::QueryLog do
     subject.before
     connection.execute(query)
     subject.after
-    subject.record(observation)
+    subject.record
   end
 end

spec/lib/gitlab/database/migrations/observers/query_statistics_spec.rb

@@ -2,8 +2,9 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Database::Migrations::Observers::QueryStatistics do
-  subject { described_class.new }
+  subject { described_class.new(observation) }
 
+  let(:observation) { Gitlab::Database::Migrations::Observation.new }
   let(:connection) { ActiveRecord::Base.connection }
 
   def mock_pgss(enabled: true)
@@ -37,7 +38,6 @@ RSpec.describe Gitlab::Database::Migrations::Observers::QueryStatistics do
   end
 
   describe '#record' do
-    let(:observation) { Gitlab::Database::Migrations::Observation.new }
     let(:result) { double }
     let(:pgss_query) do
       <<~SQL
@@ -52,7 +52,7 @@ RSpec.describe Gitlab::Database::Migrations::Observers::QueryStatistics do
         mock_pgss(enabled: true)
         expect(connection).to receive(:execute).with(pgss_query).once.and_return(result)
 
-        expect { subject.record(observation) }.to change { observation.query_statistics }.from(nil).to(result)
+        expect { subject.record }.to change { observation.query_statistics }.from(nil).to(result)
       end
     end
 
@@ -61,7 +61,7 @@ RSpec.describe Gitlab::Database::Migrations::Observers::QueryStatistics do
         mock_pgss(enabled: false)
         expect(connection).not_to receive(:execute)
 
-        expect { subject.record(observation) }.not_to change { observation.query_statistics }
+        expect { subject.record }.not_to change { observation.query_statistics }
       end
     end
   end

spec/lib/gitlab/database/migrations/observers/total_database_size_change_spec.rb

@@ -2,7 +2,7 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Database::Migrations::Observers::TotalDatabaseSizeChange do
-  subject { described_class.new }
+  subject { described_class.new(observation) }
 
   let(:observation) { Gitlab::Database::Migrations::Observation.new }
   let(:connection) { ActiveRecord::Base.connection }
@@ -14,7 +14,7 @@ RSpec.describe Gitlab::Database::Migrations::Observers::TotalDatabaseSizeChange
 
     subject.before
     subject.after
-    subject.record(observation)
+    subject.record
 
     expect(observation.total_database_size_change).to eq(256 - 1024)
   end
@@ -27,13 +27,13 @@ RSpec.describe Gitlab::Database::Migrations::Observers::TotalDatabaseSizeChange
     it 'does not record anything if before size is unknown' do
       subject.after
 
-      expect { subject.record(observation) }.not_to change { observation.total_database_size_change }
+      expect { subject.record }.not_to change { observation.total_database_size_change }
     end
 
     it 'does not record anything if after size is unknown' do
       subject.before
 
-      expect { subject.record(observation) }.not_to change { observation.total_database_size_change }
+      expect { subject.record }.not_to change { observation.total_database_size_change }
     end
   end
 end