## What's wrong `gitlab_labkit_rate_limiter_calls_total{rate_limiter, rule, action}` tries to answer two questions with one counter and does neither well. The clearest symptom is `rule="unmatched"`. That isn't a rule. It's a placeholder the evaluator emits when the loop finishes without a terminating decision (`evaluator.rb:125`), and it covers three unrelated situations: - No rule matched the request. - Rules matched, but only `:log` rules, which don't terminate. `metrics.rb:8-10` documents this. - A rule matched but got skipped over by the `count_distinct` fail-open path (`evaluator.rb:114-118`). Because it sits in the `rule` label, any query that aggregates over `rule` mixes "a throttle decided something" with "nothing decided anything". ## Where this bit us Building a shadow divergence query against Rack::Attack, of the shape `(new blocks - old blocks) / total checks`, getting a usable denominator took three corrections: 1. `allow`, `limit` and `log` are mutually exclusive per rule, so they sum safely. But `skip` for the same request lands on a different rule name (`<rule>_claim`), so summing all actions by rule double counts. 2. All three `rack_request*` limiters evaluate every request. Summing `allow|limit|log` across them counts each request about 3.6 times: 123.5k/s against a real rate of ~53k/s. 3. The dedup is to exclude `rule="unmatched"`, because a request matching a named rule in one limiter is `unmatched` in the other two. It works, 34,466/s across all three with near enough no overlap, but that's a property of how the rules happen to be laid out rather than something the metric promises. None of that is discoverable from the metric. The next person writing this query gets it wrong. There's a fourth problem. A check that fails open increments `errors_total` and nothing else (`evaluator.rb:70-78`), so `calls_total` is not a count of checks. You can't work out what fraction of checks failed open without joining two metrics that don't share a denominator. ## Proposal Two counters, replacing the current one. ### Per rule evaluation ``` gitlab_labkit_rate_limiter_rule_evaluations_total{rate_limiter, rule, action, result} ``` One increment for every rule evaluated, so non-terminating chains are fully visible. `rule` carries real rule names only, no `unmatched`. `action` is the configured rule action: `limit`, `log` or `skip`. `result` is what this one evaluation decided: | Rule action | Exceeded | `result` | |---|---|---| | `limit` | no | `allow` | | `limit` | yes | `block` | | `log` | no | `allow` | | `log` | yes | `log` | | `skip` | n/a | `skip` | This drops the separate `exceeded` label from the proposal in https://gitlab.com/gitlab-com/gl-infra/production-engineering/-/work_items/29052. An exceeded `log` rule is `result="log"`, and `action` still says which kind of rule produced an `allow`, so nothing is lost. ### Per limiter ``` gitlab_labkit_rate_limiter_calls_total{rate_limiter, action, matched, error} ``` One increment per `check` call, including calls that fail open. `action` is `allow` or `block`, what the caller should do. `matched` and `error` are `true` or `false`. `sum by (rate_limiter) (rate(calls_total[5m]))` then gives the request rate through the limiter, with no exclusions and no dedup trick. "Nothing matched" stops being a fake rule and becomes a property of the check, which is what it always was. ## Runbooks changes `gitlab-com/runbooks` needs an MR in the same cycle. Hand-maintained: - `metrics-catalog/services/rate-limiting.jsonnet:45-57`. `significantLabels` is currently `['rate_limiter', 'rule', 'action']`, which is what puts those labels on the `sli_aggregations` recording rules. After the split, `rule` and `action` live on `rule_evaluations_total`, not on `calls_total`, so this has to move or the per-rule breakdown disappears. The SLI framework has no notion of a second counter today, so this needs a decision. The description at `:27-43` is copied verbatim into every generated alert annotation. - `dashboards/rate-limiting/detail.dashboard.jsonnet`. The metric contract prose at `:388-391` documents `action ∈ {allow, block, log}` and the `unmatched`/`allow` pairing. The calls panel at `:412` aggregates by `rate_limiter, rule, action`. The gauge table at `:430-431` is unaffected. The parity panel at `:451-471` needs rewriting. - `mimir-rules/gitlab-gprd/rack-attack-rate-limits.yml`, whole file, once Rack::Attack retires. - `docs/rate-limiting/README.md` has no labkit section at all, and no `#alerts` anchor even though the generated alerts link to `rate-limiting/#alerts`. Regenerated by `make generate`, but the output shape changes: - `mimir-rules/gitlab-{gprd,gstg,pre}/rate-limiting/autogenerated-*-sli-aggregations.yml`. `sli_aggregations:gitlab_labkit_rate_limiter_calls_total:rate_{5m,30m,1h}` loses `rule` and gains `matched`/`error`. Anything consuming it breaks. - `...-component-aggregation.yml` and `...-feature_category-aggregation.yml` collapse to `env,environment,stage`, so they're unaffected by the label change. - `...-service-level-alerts.yml`. `RateLimitingServiceRateLimiterChecksTrafficCessation` and `...TrafficAbsent` key off `requestRate`. Today `calls_total` counts terminating decisions plus log rules; afterwards it counts checks. For the rack limiters that's roughly 124k/s dropping to 53k/s, so the thresholds need revisiting. With `error="true"` on `calls_total`, `errorRate` would become `calls_total{error="true"}` and `errors_total` would go away. That would give the error ratio a denominator it actually shares. Worth deciding as part of this. ## Breaking change Consumers to update in the same cycle: the runbooks MR above, the metrics catalog registration (https://gitlab.com/gitlab-com/gl-infra/production-engineering/-/work_items/28832), the Rate Limiting Overview dashboard (https://gitlab.com/gitlab-com/gl-infra/production-engineering/-/work_items/28831), and the Rack::Attack shadow comparison queries. The design document needs an update as well. Section 1.10 specifies `calls_total{rate_limiter, result}` and `rule_evaluations_total{rate_limiter, rule, action, result, exceeded}`. This proposal renames `result` to `action` on the per-check counter, adds `matched` and `error`, and drops `exceeded` from the per-rule counter. ## Related - https://gitlab.com/gitlab-com/gl-infra/production-engineering/-/work_items/29052, which specified this split and closed with only the action rename shipped - Design document, [1.10 Observability](https://handbook.gitlab.com/handbook/engineering/architecture/design-documents/unified_rate_limiting/#110-observability) 🤖 beep boop