Commit 8823f24b authored by Peter Leitzen's avatar Peter Leitzen
Browse files

Merge branch 'perf/lazy-string-key-conversion' into 'master' 

perf: implement lazy string key conversion for field validation

See merge request !232

Merged-by: default avatarPeter Leitzen <pleitzen@gitlab.com>
Approved-by: Elliot Forbes's avatarElliot Forbes <eforbes@gitlab.com>
Approved-by: default avatarPeter Leitzen <pleitzen@gitlab.com>
Co-authored-by: default avatardbarrett <dbarrett@gitlab.com>
parents 920fcdff 1dcb8ec5

lib/labkit/logging/field_validator/log_interceptor.rb

+8 −11
Original line number Diff line number Diff line
@@ -38,17 +38,20 @@ module Labkit 
          callsite_path = normalize_path(location.path)

          return data unless callsite_path



          all_fields = extract_string_keys(data)

          logger_class = self.class.name || 'AnonymousLogger'



          all_fields.each do |field|

            standard_field = Labkit::Fields::Deprecated.standard_field_for(field)

          deprecated_lookup = Labkit::Fields::Deprecated.all

          if data.is_a?(Hash)

            data.each_key do |key|

              key_str = key.to_s

              standard_field = deprecated_lookup[key_str]

              next unless standard_field



            Registry.instance.record_offense(callsite_path, location.lineno, field, standard_field, logger_class)

              Registry.instance.record_offense(callsite_path, location.lineno, key_str, standard_field, logger_class)

            end

          end



          Registry.instance.check_for_removed_offenses(callsite_path, all_fields, logger_class)

          Registry.instance.check_for_removed_offenses(callsite_path, data, logger_class)



          data

        end
@@ -83,12 +86,6 @@ module Labkit 
          start_idx += 1 if absolute_path[start_idx] == '/'

          absolute_path[start_idx..]

        end



        def extract_string_keys(data)

          return Set.new unless data.is_a?(Hash)



          data.keys.to_set(&:to_s)

        end

      end

    end

  end

lib/labkit/logging/field_validator/registry.rb

+6 −0
Original line number Diff line number Diff line
@@ -109,6 +109,12 @@ module Labkit 
          config = Config.load

          config.fetch('offenses', [])

        end



        def extract_string_keys(data)

          return Set.new unless data.is_a?(Hash)



          data.keys.to_set(&:to_s)

        end

      end

    end

  end

spec/labkit/logging/field_validator/log_interceptor_spec.rb

+3 −4
Original line number Diff line number Diff line
@@ -20,8 +20,7 @@ RSpec.describe Labkit::Logging::FieldValidator::LogInterceptor do 


  before do

    registry.clear!

    allow(Labkit::Fields::Deprecated).to receive(:standard_field_for).and_return(nil)

    allow(Labkit::Fields::Deprecated).to receive(:standard_field_for).with('meta.user_id').and_return('gl_user_id')

    allow(Labkit::Fields::Deprecated).to receive(:all).and_return({ 'meta.user_id' => 'gl_user_id' })

  end



  around do |example|
@@ -104,9 +103,9 @@ RSpec.describe Labkit::Logging::FieldValidator::LogInterceptor do 
        allow(test_logger).to receive(:determine_callsite).and_return(mock_location)

      end



      it 'calls check_for_removed_offenses with all fields' do

      it 'calls check_for_removed_offenses with data hash' do

        expect(registry).to receive(:check_for_removed_offenses)

          .with('app/test.rb', array_including('gl_user_id', 'message'), 'AnonymousLogger')

          .with('app/test.rb', hash_including('gl_user_id' => 123, 'message' => 'hello'), 'AnonymousLogger')



        test_logger.format_message('INFO', Time.now.utc, 'test', { 'gl_user_id' => 123, 'message' => 'hello' })

      end

spec/labkit/logging/field_validator/registry_spec.rb

+9 −9
Original line number Diff line number Diff line
@@ -119,7 +119,7 @@ RSpec.describe Labkit::Logging::FieldValidator::Registry do 


      it 'marks offense as removed when standard field present and deprecated field absent' do

        # Simulate a log call with the standard field but without the deprecated field

        registry.check_for_removed_offenses('app/models/user.rb', %w[gl_user_id message], 'AppLogger')

        registry.check_for_removed_offenses('app/models/user.rb', { 'gl_user_id' => 123, 'message' => 'test' }, 'AppLogger')



        _detected, _new, removed_offenses = registry.finalize
@@ -130,7 +130,7 @@ RSpec.describe Labkit::Logging::FieldValidator::Registry do 


      it 'does not mark as removed when deprecated field is still present' do

        # Simulate a log call that still has the deprecated field

        registry.check_for_removed_offenses('app/models/user.rb', %w[user_id gl_user_id], 'AppLogger')

        registry.check_for_removed_offenses('app/models/user.rb', { 'user_id' => 123, 'gl_user_id' => 123 }, 'AppLogger')



        _detected, _new, removed_offenses = registry.finalize
@@ -139,7 +139,7 @@ RSpec.describe Labkit::Logging::FieldValidator::Registry do 


      it 'does not mark as removed when standard field is absent' do

        # Simulate a log call without the standard field

        registry.check_for_removed_offenses('app/models/user.rb', %w[message other_field], 'AppLogger')

        registry.check_for_removed_offenses('app/models/user.rb', { 'message' => 'test', 'other_field' => 1 }, 'AppLogger')



        _detected, _new, removed_offenses = registry.finalize
@@ -147,7 +147,7 @@ RSpec.describe Labkit::Logging::FieldValidator::Registry do 
      end



      it 'does not mark as removed for different logger class' do

        registry.check_for_removed_offenses('app/models/user.rb', ['gl_user_id'], 'DifferentLogger')

        registry.check_for_removed_offenses('app/models/user.rb', { 'gl_user_id' => 123 }, 'DifferentLogger')



        _detected, _new, removed_offenses = registry.finalize
@@ -155,7 +155,7 @@ RSpec.describe Labkit::Logging::FieldValidator::Registry do 
      end



      it 'does not mark as removed for different callsite' do

        registry.check_for_removed_offenses('app/models/project.rb', ['gl_user_id'], 'AppLogger')

        registry.check_for_removed_offenses('app/models/project.rb', { 'gl_user_id' => 123 }, 'AppLogger')



        _detected, _new, removed_offenses = registry.finalize
@@ -164,7 +164,7 @@ RSpec.describe Labkit::Logging::FieldValidator::Registry do 


      it 'un-marks removed offense if deprecated field is detected later' do

        # First, the offense appears fixed

        registry.check_for_removed_offenses('app/models/user.rb', ['gl_user_id'], 'AppLogger')

        registry.check_for_removed_offenses('app/models/user.rb', { 'gl_user_id' => 123 }, 'AppLogger')



        # Then, the deprecated field is detected (e.g., from a different code path)

        registry.record_offense('app/models/user.rb', 10, 'user_id', 'gl_user_id', 'AppLogger')
@@ -196,7 +196,7 @@ RSpec.describe Labkit::Logging::FieldValidator::Registry do 
      end



      it 'marks both as removed when standard field is present' do

        registry.check_for_removed_offenses('app/models/user.rb', ['gl_user_id'], 'AppLogger')

        registry.check_for_removed_offenses('app/models/user.rb', { 'gl_user_id' => 123 }, 'AppLogger')



        _detected, _new, removed_offenses = registry.finalize
@@ -206,7 +206,7 @@ RSpec.describe Labkit::Logging::FieldValidator::Registry do 
      it 'only marks matching offenses as removed' do

        # One deprecated field is still present, one is fixed

        registry.record_offense('app/models/user.rb', 10, 'user_id', 'gl_user_id', 'AppLogger')

        registry.check_for_removed_offenses('app/models/user.rb', %w[gl_user_id user_id], 'AppLogger')

        registry.check_for_removed_offenses('app/models/user.rb', { 'gl_user_id' => 123, 'user_id' => 456 }, 'AppLogger')



        _detected, _new, removed_offenses = registry.finalize
@@ -221,7 +221,7 @@ RSpec.describe Labkit::Logging::FieldValidator::Registry do 
      end



      it 'does not mark any offenses as removed' do

        registry.check_for_removed_offenses('app/models/user.rb', ['gl_user_id'], 'AppLogger')

        registry.check_for_removed_offenses('app/models/user.rb', { 'gl_user_id' => 123 }, 'AppLogger')



        _detected, _new, removed_offenses = registry.finalize

spec/labkit/logging/field_validator_spec.rb

+6 −6
Original line number Diff line number Diff line
@@ -271,7 +271,7 @@ RSpec.describe Labkit::Logging::FieldValidator do 
        end



        it 'auto-updates config file to remove fixed offenses' do

          registry.check_for_removed_offenses('app/models/user.rb', ['gl_user_id'], 'AppLogger')

          registry.check_for_removed_offenses('app/models/user.rb', { 'gl_user_id' => 123 }, 'AppLogger')



          suppress_output { described_class.process_violations }
@@ -280,21 +280,21 @@ RSpec.describe Labkit::Logging::FieldValidator do 
        end



        it 'outputs thank you message' do

          registry.check_for_removed_offenses('app/models/user.rb', ['gl_user_id'], 'AppLogger')

          registry.check_for_removed_offenses('app/models/user.rb', { 'gl_user_id' => 123 }, 'AppLogger')



          expect { described_class.process_violations }

            .to output(/Thank you for improving our logging standards/).to_stderr

        end



        it 'lists the fixed offenses in the thank you message' do

          registry.check_for_removed_offenses('app/models/user.rb', ['gl_user_id'], 'AppLogger')

          registry.check_for_removed_offenses('app/models/user.rb', { 'gl_user_id' => 123 }, 'AppLogger')



          expect { described_class.process_violations }

            .to output(%r{app/models/user\.rb.*user_id.*GL_USER_ID}m).to_stderr

        end



        it 'mentions the config file was updated' do

          registry.check_for_removed_offenses('app/models/user.rb', ['gl_user_id'], 'AppLogger')

          registry.check_for_removed_offenses('app/models/user.rb', { 'gl_user_id' => 123 }, 'AppLogger')



          expect { described_class.process_violations }

            .to output(/has been automatically updated/).to_stderr
@@ -308,7 +308,7 @@ RSpec.describe Labkit::Logging::FieldValidator do 


        it 'does not auto-update config file' do

          original_content = File.read(config_path)

          registry.check_for_removed_offenses('app/models/user.rb', ['gl_user_id'], 'AppLogger')

          registry.check_for_removed_offenses('app/models/user.rb', { 'gl_user_id' => 123 }, 'AppLogger')



          suppress_output { described_class.process_violations }
@@ -316,7 +316,7 @@ RSpec.describe Labkit::Logging::FieldValidator do 
        end



        it 'does not output thank you message' do

          registry.check_for_removed_offenses('app/models/user.rb', ['gl_user_id'], 'AppLogger')

          registry.check_for_removed_offenses('app/models/user.rb', { 'gl_user_id' => 123 }, 'AppLogger')



          expect { described_class.process_violations }

            .not_to output(/Thank you/).to_stderr