Verified Commit 96e48148 authored by Max Woolf's avatar Max Woolf Committed by GitLab
Browse files

Merge branch 'bvl/remove-check-logging' into 'master' 

refactor: remove per-request logging from `Limiter#check`

See merge request !273

Merged-by: Max Woolf's avatarMax Woolf <mwoolf@gitlab.com>
Approved-by: default avatarSam Wiskow <swiskow@gitlab.com>
Approved-by: Max Woolf's avatarMax Woolf <mwoolf@gitlab.com>
Co-authored-by: Bob Van Landuyt's avatarBob Van Landuyt <bob@gitlab.com>
parents 8eea3315 8b004d37

lib/labkit/rate_limit/limiter.rb

+3 −16
Original line number Diff line number Diff line
@@ -21,11 +21,10 @@ module Labkit 


      def initialize(name:, rules:, redis: nil, logger: nil)

        @logger = logger || RateLimit.config.logger || Labkit::Logging::JsonLogger.new($stdout)

        validated_name = validate_name!(name)

        @name = validated_name

        @name = validate_name!(name)



        @evaluator = Evaluator.new(

          name: validated_name,

          name: @name,

          rules: prepare_rules(rules),

          redis: redis || RateLimit.config.redis,

          logger: @logger
@@ -36,19 +35,7 @@ module Labkit 
      # @return [Result]

      def check(identifier)

        id = identifier.is_a?(Identifier) ? identifier : Identifier.new(identifier)

        result = @evaluator.check(id)



        if result.exceeded? && result.action == :block

          @logger.warn(

            message: "rate_limit_check",

            name: @name,

            rule_name: result.rule.name,

            exceeded: true,

            severity: "WARN"

          )

        end



        result

        @evaluator.check(id)

      end



      private

spec/labkit/rate_limit/limiter_spec.rb

+0 −25
Original line number Diff line number Diff line
@@ -117,31 +117,6 @@ RSpec.describe Labkit::RateLimit::Limiter do 
    end

  end



  describe "Scenario B: exceeded :block rule emits rate_limit_check WARN" do

    it "logs warn with rule_name and exceeded: true" do

      allow(redis).to receive(:incr).and_return(101)

      lim = limiter

      lim.check({ user: 42 })

      expect(logger).to have_received(:warn).with(

        hash_including(

          message: "rate_limit_check",

          rule_name: "default",

          exceeded: true

        )

      )

    end

  end



  describe "Scenario C: exceeded :log rule does not emit rate_limit_check WARN" do

    it "does not call warn with rate_limit_check message" do

      allow(redis).to receive(:incr).and_return(101)

      r = rule(action: :log)

      lim = limiter(rules: [r])

      lim.check({ user: 42 })

      expect(logger).not_to have_received(:warn).with(hash_including(message: "rate_limit_check"))

    end

  end



  describe "prepare_rules: rule name deduplication and sanitization" do

    context "when in dev/test environment" do

      it "raises ArgumentError for duplicate rule names" do