Merge branch 'per-directory-instrumentation-testing' into 'master'

Move migration statistics to a file per migration

See merge request gitlab-org/gitlab!83895

lib/gitlab/database/migrations/instrumentation.rb

@@ -6,11 +6,8 @@ module Gitlab
       class Instrumentation
         STATS_FILENAME = 'migration-stats.json'
 
-        attr_reader :observations
-
         def initialize(result_dir:, observer_classes: ::Gitlab::Database::Migrations::Observers.all_observers)
           @observer_classes = observer_classes
-          @observations = []
           @result_dir = result_dir
         end
 
@@ -38,15 +35,16 @@ module Gitlab
           on_each_observer(observers) { |observer| observer.after }
           on_each_observer(observers) { |observer| observer.record }
 
-          record_observation(observation)
+          record_observation(observation, destination_dir: per_migration_result_dir)
         end
 
         private
 
         attr_reader :observer_classes
 
-        def record_observation(observation)
-          @observations << observation
+        def record_observation(observation, destination_dir:)
+          stats_file_location = File.join(destination_dir, STATS_FILENAME)
+          File.write(stats_file_location, observation.to_json)
         end
 
         def on_each_observer(observers, &block)

lib/gitlab/database/migrations/runner.rb

@@ -6,7 +6,7 @@ module Gitlab
       class Runner
         BASE_RESULT_DIR = Rails.root.join('tmp', 'migration-testing').freeze
         METADATA_FILENAME = 'metadata.json'
-        SCHEMA_VERSION = 2 # Version of the output format produced by the runner
+        SCHEMA_VERSION = 3 # Version of the output format produced by the runner
 
         class << self
           def up
@@ -80,13 +80,8 @@ module Gitlab
             end
           end
         ensure
-          if instrumentation
-            stats_filename = File.join(result_dir, Gitlab::Database::Migrations::Instrumentation::STATS_FILENAME)
-            File.write(stats_filename, instrumentation.observations.to_json)
-
           metadata_filename = File.join(result_dir, METADATA_FILENAME)
           File.write(metadata_filename, { version: SCHEMA_VERSION }.to_json)
-          end
 
           # We clear the cache here to mirror the cache clearing that happens at the end of `db:migrate` tasks
           # This clearing makes subsequent rake tasks in the same execution pick up database schema changes caused by

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

@@ -11,6 +11,10 @@ RSpec.describe Gitlab::Database::Migrations::Instrumentation do
   describe '#observe' do
     subject { described_class.new(result_dir: result_dir) }
 
+    def load_observation(result_dir, migration_name)
+      Gitlab::Json.parse(File.read(File.join(result_dir, migration_name, described_class::STATS_FILENAME)))
+    end
+
     let(:migration_name) { 'test' }
     let(:migration_version) { '12345' }
 
@@ -87,7 +91,7 @@ RSpec.describe Gitlab::Database::Migrations::Instrumentation do
         end
 
         context 'retrieving observations' do
-          subject { instance.observations.first }
+          subject { load_observation(result_dir, migration_name) }
 
           before do
             observe
@@ -98,10 +102,10 @@ RSpec.describe Gitlab::Database::Migrations::Instrumentation do
           end
 
           it 'records a valid observation', :aggregate_failures do
-            expect(subject.walltime).not_to be_nil
-            expect(subject.success).to be_falsey
-            expect(subject.version).to eq(migration_version)
-            expect(subject.name).to eq(migration_name)
+            expect(subject['walltime']).not_to be_nil
+            expect(subject['success']).to be_falsey
+            expect(subject['version']).to eq(migration_version)
+            expect(subject['name']).to eq(migration_name)
           end
         end
       end
@@ -113,11 +117,18 @@ RSpec.describe Gitlab::Database::Migrations::Instrumentation do
       let(:migration1) { double('migration1', call: nil) }
       let(:migration2) { double('migration2', call: nil) }
 
+      let(:migration_name_2) { 'other_migration' }
+      let(:migration_version_2) { '98765' }
+
       it 'records observations for all migrations' do
         subject.observe(version: migration_version, name: migration_name, connection: connection) {}
-        subject.observe(version: migration_version, name: migration_name, connection: connection) { raise 'something went wrong' } rescue nil
+        subject.observe(version: migration_version_2, name: migration_name_2, connection: connection) { raise 'something went wrong' } rescue nil
+
+        expect { load_observation(result_dir, migration_name) }.not_to raise_error
+        expect { load_observation(result_dir, migration_name_2) }.not_to raise_error
 
-        expect(subject.observations.size).to eq(2)
+        # Each observation is a subdirectory of the result_dir, so here we check that we didn't record an extra one
+        expect(Pathname(result_dir).children.map { |d| d.basename.to_s }).to contain_exactly(migration_name, migration_name_2)
       end
     end
   end