Commit 42ad95fc authored by Max Woolf's avatar Max Woolf
Browse files

refactor: apply review suggestions to Result and metric labels

- Emit the rule-level outcome (allow/log/limit/skip) as the calls_total
  action label instead of preserving the pre-rename "block" vocabulary.
- Simplify Result#skipped? to a double-bang.
- Memoize Result#most_constraining via Array#min; add_evaluation
  invalidates the memo.

Co-Authored-By: default avatarClaude Fable 5 <noreply@anthropic.com>
parent 0631f853
Loading
Loading
Loading
Loading
Loading
+1 −1
Original line number Diff line number Diff line
@@ -364,7 +364,7 @@ should treat the request as allowed.

| metric                                          | type    | labels                              | meaning                                                              |
|-------------------------------------------------|---------|-------------------------------------|----------------------------------------------------------------------|
| `gitlab_labkit_rate_limiter_calls_total`        | counter | `rate_limiter`, `rule`, `action`    | One increment per counted rule (plus one per matched `:skip` rule). `action` is one of `"allow"` (under limit), `"block"` (blocking `:limit` rule), `"log"` (exceeded `:log` rule), `"skip"`. `rule="unmatched", action="allow"` when no rule matched. |
| `gitlab_labkit_rate_limiter_calls_total`        | counter | `rate_limiter`, `rule`, `action`    | One increment per counted rule (plus one per matched `:skip` rule). `action` is the rule-level outcome: `"allow"` (under limit), `"limit"` (blocking `:limit` rule), `"log"` (exceeded `:log` rule), `"skip"`. `rule="unmatched", action="allow"` when no rule matched. |
| `gitlab_labkit_rate_limiter_errors_total`       | counter | `rate_limiter`                      | Fail-open events (any `StandardError` in the labkit path).            |
| `gitlab_labkit_rate_limiter_limit`              | gauge   | `rate_limiter`, `rule`              | Resolved limit at the last check (useful when `limit:` is callable). |
| `gitlab_labkit_rate_limiter_period_seconds`     | gauge   | `rate_limiter`, `rule`              | Resolved period at the last check.                                   |
+1 −11
Original line number Diff line number Diff line
@@ -341,7 +341,7 @@ module Labkit
        Metrics.calls_total.increment(
          rate_limiter: @name,
          rule: evaluation.rule.name,
          action: calls_action_label(evaluation)
          action: (evaluation.exceeded? ? evaluation.rule.action : :allow).to_s
        )
        Metrics.limit_gauge.set(
          { rate_limiter: @name, rule: evaluation.rule.name },
@@ -353,16 +353,6 @@ module Labkit
        )
      end

      # Pre-rename label vocabulary is preserved so dashboards keep working:
      # "block" for a blocking :limit rule, "log" for an exceeded :log rule,
      # "allow" for anything under its limit (the Result-level action for an
      # exceeded :log rule is :allow, but the metric keeps the distinct label).
      def calls_action_label(evaluation)
        return "allow" unless evaluation.exceeded?

        evaluation.block? ? "block" : "log"
      end

      # calls_total carries action="skip" (the rule action, not the :allow the
      # caller sees) so bypass traffic stays distinguishable from counted
      # allows. No limit/period gauges: a skip rule has no limit to report.
+6 −4
Original line number Diff line number Diff line
@@ -40,9 +40,11 @@ module Labkit
        @evaluations = []
        @skip_rule = nil
        @error = error
        @most_constraining = nil
      end

      def add_evaluation(evaluation)
        @most_constraining = nil
        @evaluations << evaluation
        self
      end
@@ -59,7 +61,7 @@ module Labkit
      end

      def skipped?
        !@skip_rule.nil?
        !!@skip_rule
      end

      def block?
@@ -91,10 +93,10 @@ module Labkit
      end

      # The evaluation with the strongest claim on the outcome; ties keep the
      # earliest-declared rule (min returns the first of tied elements, so the
      # incumbent must come first in the pair).
      # earliest-declared rule (min returns the first of tied elements).
      # Memoized; add_evaluation invalidates.
      def most_constraining
        @evaluations.reduce { |best, evaluation| [best, evaluation].min }
        @most_constraining ||= @evaluations.min
      end

      # Returns RFC-compliant rate limit response headers, or {} when no rule matched or an error occurred.
+3 −3
Original line number Diff line number Diff line
@@ -570,11 +570,11 @@ RSpec.describe Labkit::RateLimit::Evaluator do
      expect(metrics.period_gauge.get(rate_limiter: "rack_request", rule: "api_rule")).to eq(120.0)
    end

    it "increments calls_total with action: block when exceeded" do
    it "increments calls_total with action: limit when exceeded" do
      rule = make_rule(name: "api_rule", limit: 5, action: :limit)
      evaluator(rules: [rule]).check(identifier, cost: 6)

      expect(metrics.calls_total.get(rate_limiter: "rack_request", rule: "api_rule", action: "block")).to eq(1.0)
      expect(metrics.calls_total.get(rate_limiter: "rack_request", rule: "api_rule", action: "limit")).to eq(1.0)
    end

    it "increments calls_total with action: log when exceeded with :log action" do
@@ -614,7 +614,7 @@ RSpec.describe Labkit::RateLimit::Evaluator do

      expect(metrics.errors_total.get(rate_limiter: "rack_request")).to eq(1.0)
      expect(metrics.calls_total.get(rate_limiter: "rack_request", rule: "err_rule", action: "allow")).to eq(0.0)
      expect(metrics.calls_total.get(rate_limiter: "rack_request", rule: "err_rule", action: "block")).to eq(0.0)
      expect(metrics.calls_total.get(rate_limiter: "rack_request", rule: "err_rule", action: "limit")).to eq(0.0)
    end

    it "resolves callable limit and period at evaluation time" do
+12 −0
Original line number Diff line number Diff line
@@ -180,6 +180,18 @@ RSpec.describe Labkit::RateLimit::Result do
      expect(result.info.remaining).to eq(1)
    end

    it "recomputes after an evaluation is added following a read (memo invalidation)" do
      loose = evaluation(rule: rule(name: "loose"), exceeded: false, resolved_limit: 1000, count: 20, remaining: 980)
      tight = evaluation(rule: rule(name: "tight"), exceeded: false, resolved_limit: 100, count: 99, remaining: 1)
      result = described_class.new.add_evaluation(loose)

      expect(result.most_constraining).to eq(loose)

      result.add_evaluation(tight)

      expect(result.most_constraining).to eq(tight)
    end

    it "keeps declaration order on ties" do
      first  = evaluation(rule: rule(name: "first"),  exceeded: false, resolved_limit: 10, count: 5, remaining: 5)
      second = evaluation(rule: rule(name: "second"), exceeded: false, resolved_limit: 20, count: 15, remaining: 5)