chore: shave off some test code around the new rate limiter (#962)

Author: whatyouhide

.dialyzer_ignore.exs

@@ -1,4 +1,5 @@
 [
+  {"test/support/case.ex"},
   {"test/support/example_plug_application.ex"},
   {"test/support/test_helpers.ex"},
   {"lib/sentry/opentelemetry/sampler.ex", :pattern_match, 1}

config/config.exs

@@ -9,7 +9,6 @@ if config_env() == :test do
     hackney_opts: [recv_timeout: 50, pool: :sentry_pool],
     send_result: :sync,
     send_max_attempts: 1,
-    start_rate_limiter: false,
     dedup_events: false,
     test_mode: true,
     traces_sample_rate: 1.0

lib/sentry/application.ex

@@ -33,14 +33,6 @@ defmodule Sentry.Application do
         []
       end
 
-    # Don't start RateLimiter in test environment - tests start their own instances
-    maybe_rate_limiter =
-      if Application.get_env(:sentry, :start_rate_limiter, true) == false do
-        []
-      else
-        [Sentry.Transport.RateLimiter]
-      end
-
     children =
       [
         {Registry, keys: :unique, name: Sentry.Transport.SenderRegistry},
@@ -55,7 +47,7 @@ defmodule Sentry.Application do
       ] ++
         maybe_http_client_spec ++
         maybe_span_storage ++
-        maybe_rate_limiter ++
+        maybe_rate_limiter() ++
         [Sentry.Transport.SenderPool]
 
     cache_loaded_applications()
@@ -106,4 +98,12 @@ defmodule Sentry.Application do
       end
     end)
   end
+
+  # In tests, we do not run a global rate limiter; tests start their own when
+  # they need it.
+  if Mix.env() == :test do
+    defp maybe_rate_limiter, do: []
+  else
+    defp maybe_rate_limiter, do: [Sentry.Transport.RateLimiter]
+  end
 end

lib/sentry/transport/rate_limiter.ex

@@ -1,12 +1,25 @@
 defmodule Sentry.Transport.RateLimiter do
   @moduledoc false
+
   # Tracks rate limits per category from Sentry API responses.
   # Uses an ETS table to store expiry timestamps for rate-limited categories.
   # When Sentry returns a 429 response with rate limit headers, this module
   # stores the expiry time per category, allowing other parts of the SDK to
   # check if an event should be dropped before sending.
   #
+  # The ETS table stores tuples with these elements:
+  #
+  #   1. Category (String.t/0 | :global): the category being rate limited.
+  #   2. Expiry timestamp (Unix timestamp in seconds): time at which the rate-limit
+  #      entry expires and can be pruned).
+  #
   # See https://develop.sentry.dev/sdk/expected-features/rate-limiting/
+  #
+  # For testing, we use the trick of determining the name of this GenServer
+  # and consequently the ETS table it uses) based on the Mix environment (at compile
+  # time, so no impact on performance). If we're in the :test environment, we require
+  # that there's a table name for this in the process dictionary. In normal circumstances
+  # we use __MODULE__ instead.
 
   use GenServer
 
@@ -22,20 +35,18 @@ defmodule Sentry.Transport.RateLimiter do
   ## Options
 
     * `:name` - The name to register the GenServer under. Defaults to `__MODULE__`.
-    * `:table_name` - The name for the ETS table. Defaults to `__MODULE__`.
 
   """
   @spec start_link(keyword()) :: GenServer.on_start()
-  def start_link(opts \\ []) do
+  def start_link(opts) when is_list(opts) do
     name = Keyword.get(opts, :name, __MODULE__)
-    GenServer.start_link(__MODULE__, opts, name: name)
+    GenServer.start_link(__MODULE__, _table_name = name, name: name)
   end
 
   ## GenServer Callbacks
 
   @impl true
-  def init(opts) do
-    table_name = Keyword.get(opts, :table_name, __MODULE__)
+  def init(table_name) do
     _table = :ets.new(table_name, [:named_table, :public, :set, read_concurrency: true])
     schedule_sweep()
     {:ok, %__MODULE__{table_name: table_name}}
@@ -45,7 +56,8 @@ defmodule Sentry.Transport.RateLimiter do
   def handle_info(:sweep, %__MODULE__{table_name: table_name} = state) do
     now = System.system_time(:second)
 
-    # Match spec: select entries where expiry (position 2) < now
+    # This match spec elects entries where expiry is in the past.
+    # Remember, tuples are {category, expiry_time}.
     match_spec = [{{:"$1", :"$2"}, [{:<, :"$2", now}], [true]}]
 
     :ets.select_delete(table_name, match_spec)
@@ -62,11 +74,6 @@ defmodule Sentry.Transport.RateLimiter do
   Returns `true` if the category is rate-limited (either specifically or via
   a global rate limit), `false` otherwise.
 
-  ## Options
-
-    * `:table_name` - The ETS table name. Falls back to the `:rate_limiter_table_name`
-      value in the process dictionary, then to `__MODULE__`.
-
   ## Examples
 
       iex> RateLimiter.rate_limited?("error")
@@ -77,136 +84,105 @@ defmodule Sentry.Transport.RateLimiter do
       true
 
   """
-  @spec rate_limited?(String.t(), keyword()) :: boolean()
-  def rate_limited?(category, opts \\ []) do
-    table_name = get_table_name(opts)
+  @spec rate_limited?(String.t()) :: boolean()
+  def rate_limited?(category) when is_binary(category) do
     now = System.system_time(:second)
-    check_rate_limited(table_name, category, now) or check_rate_limited(table_name, :global, now)
+    rate_limited?(category, now) or rate_limited?(:global, now)
   end
 
   @doc """
-  Updates global rate limit from a Retry-After header value.
-
-  This is a fallback for when X-Sentry-Rate-Limits is not present.
-  Stores a global rate limit (:global key) that affects all categories.
-
-  ## Options
+  Updates global rate limit from a `Retry-After` header value.
 
-    * `:table_name` - The ETS table name. Falls back to the `:rate_limiter_table_name`
-      value in the process dictionary, then to `__MODULE__`.
+  This is a fallback for when `X-Sentry-Rate-Limits` is not present.
+  Stores a global rate limit (`:global` key) that affects all categories.
+  The `Retry-After` header is parsed before getting here, so we get a clean
+  integer value here.
 
   ## Examples
 
       iex> RateLimiter.update_global_rate_limit(60)
       :ok
 
   """
-  @spec update_global_rate_limit(pos_integer(), keyword()) :: :ok
-  def update_global_rate_limit(retry_after_seconds, opts \\ [])
-      when is_integer(retry_after_seconds) do
-    table_name = get_table_name(opts)
-    now = System.system_time(:second)
-    expiry = now + retry_after_seconds
-    :ets.insert(table_name, {:global, expiry})
+  @spec update_global_rate_limit(pos_integer()) :: :ok
+  def update_global_rate_limit(retry_after_seconds) when is_integer(retry_after_seconds) do
+    expiry = System.system_time(:second) + retry_after_seconds
+    :ets.insert(name(), {:global, expiry})
     :ok
   end
 
   @doc """
-  Updates rate limits from the X-Sentry-Rate-Limits header.
+  Updates rate limits from the `X-Sentry-Rate-Limits` header value.
 
   Parses the header value and stores expiry timestamps for each category.
   Returns `:ok` regardless of parsing success.
 
-  ## Options
-
-    * `:table_name` - The ETS table name. Falls back to the `:rate_limiter_table_name`
-      value in the process dictionary, then to `__MODULE__`.
-
   ## Examples
 
       iex> RateLimiter.update_rate_limits("60:error;transaction")
       :ok
 
   """
-  @spec update_rate_limits(String.t(), keyword()) :: :ok
-  def update_rate_limits(rate_limits_header, opts \\ []) do
-    table_name = get_table_name(opts)
+  @spec update_rate_limits(String.t()) :: :ok
+  def update_rate_limits(rate_limits_header) when is_binary(rate_limits_header) do
     now = System.system_time(:second)
-    rate_limits = parse_rate_limits_header(rate_limits_header)
 
-    Enum.each(rate_limits, fn {category, retry_after_seconds} ->
-      expiry = now + retry_after_seconds
-      :ets.insert(table_name, {category, expiry})
-    end)
+    rate_limits_header
+    |> parse_rate_limits_header()
+    |> Enum.map(fn {category, retry_after_seconds} -> {category, now + retry_after_seconds} end)
+    |> then(&:ets.insert(name(), &1))
 
     :ok
   end
 
   ## Private Helpers
 
-  # Get the table name with the following hierarchy:
-  # 1. Value passed in opts[:table_name]
-  # 2. Value from process dictionary (:rate_limiter_table_name)
-  # 3. Default module name
-  @spec get_table_name(keyword()) :: atom()
-  defp get_table_name(opts) do
-    case Keyword.fetch(opts, :table_name) do
-      {:ok, table_name} -> table_name
-      :error -> Process.get(:rate_limiter_table_name, __MODULE__)
-    end
-  end
-
-  @spec check_rate_limited(atom(), String.t() | :global, integer()) :: boolean()
-  defp check_rate_limited(table_name, category, time) do
-    case :ets.lookup(table_name, category) do
-      [{^category, expiry}] when expiry > time -> true
-      _ -> false
+  defp rate_limited?(category, now) do
+    case :ets.lookup(name(), category) do
+      [{^category, expiry}] when expiry > now -> true
+      _other -> false
     end
   end
 
   # Parse X-Sentry-Rate-Limits header
   # Format: "60:error;transaction:key, 2700:default:organization"
   # Returns: [{category, retry_after_seconds}, ...]
-  @spec parse_rate_limits_header(String.t()) :: [{String.t() | :global, integer()}]
   defp parse_rate_limits_header(header_value) do
     header_value
     |> String.split(",")
-    |> Enum.flat_map(&parse_quota_limit/1)
+    |> Enum.flat_map(fn quota_limit -> quota_limit |> String.trim() |> parse_quota_limit() end)
   end
 
-  @spec parse_quota_limit(String.t()) :: [{String.t() | :global, integer()}]
+  # Parses a single quota limit, like: "60:error;transaction:key"
   defp parse_quota_limit(quota_limit_str) do
-    {retry_after_str, rest} =
-      quota_limit_str |> String.trim() |> String.split(":") |> List.pop_at(0)
-
-    case parse_retry_after(retry_after_str) do
-      {:ok, retry_after} -> parse_categories(rest, retry_after)
-      :error -> []
+    with [retry_after_str | rest] <- String.split(quota_limit_str, ":"),
+         {retry_after, ""} <- Integer.parse(retry_after_str) do
+      rest
+      |> parse_categories()
+      |> Enum.map(&{&1, retry_after})
+    else
+      _other -> []
     end
   end
 
-  @spec parse_retry_after(String.t() | nil) :: {:ok, integer()} | :error
-  defp parse_retry_after(nil), do: :error
-
-  defp parse_retry_after(retry_after_str) do
-    case Integer.parse(retry_after_str) do
-      {retry_after, ""} -> {:ok, retry_after}
-      _ -> :error
+  defp parse_categories([categories_str | _rest]) do
+    case String.split(categories_str, ";", trim: true) do
+      [] -> [:global]
+      categories -> categories
     end
   end
 
-  @spec parse_categories([String.t()], integer()) :: [{String.t() | :global, integer()}]
-  defp parse_categories([categories_str | _rest], retry_after) do
-    case String.split(categories_str, ";") do
-      [""] -> [{:global, retry_after}]
-      categories -> Enum.map(categories, fn cat -> {cat, retry_after} end)
-    end
+  defp parse_categories([]) do
+    [:global]
   end
 
-  defp parse_categories(_, _), do: []
-
-  @spec schedule_sweep() :: reference()
   defp schedule_sweep do
     Process.send_after(self(), :sweep, @default_sweep_interval_ms)
   end
+
+  if Mix.env() == :test do
+    defp name, do: Process.get(:rate_limiter_table_name, __MODULE__)
+  else
+    defp name, do: __MODULE__
+  end
 end

test/sentry/client_test.exs

@@ -375,7 +375,6 @@ defmodule Sentry.ClientTest do
              end) =~ "the Sentry SDK could not encode the event to JSON: :im_just_bad"
     end
 
-    @tag :manual_rate_limiting
     test "uses the async sender pool when :result is :none", %{bypass: bypass} do
       start_supervised!(Sentry.Transport.RateLimiter)
       test_pid = self()

test/sentry/logger_handler_test.exs

@@ -646,8 +646,7 @@ defmodule Sentry.LoggerHandlerTest do
            sync_threshold: nil,
            capture_log_messages: true
          },
-         send_request: true,
-         manual_rate_limiting: true
+         send_request: true
     test "discards logged messages", %{sender_ref: ref} do
       # manually starting the rate limiter with the default name since we don't
       # have the ability to inject it into the logger backend

test/sentry/transport/rate_limiter_test.exs

@@ -3,8 +3,6 @@ defmodule Sentry.Transport.RateLimiterTest do
 
   alias Sentry.Transport.RateLimiter
 
-  defp table_name, do: Process.get(:rate_limiter_table_name)
-
   describe "parse_rate_limits_header/1" do
     test "parses single category limit" do
       # X-Sentry-Rate-Limits: 60:error
@@ -135,4 +133,6 @@ defmodule Sentry.Transport.RateLimiterTest do
       assert RateLimiter.rate_limited?("error") == true
     end
   end
+
+  defp table_name, do: Process.get(:rate_limiter_table_name)
 end

test/support/case.ex

@@ -14,8 +14,8 @@ defmodule Sentry.Case do
       assert config_before == all_config()
     end)
 
-    # Start a fresh RateLimiter for each test with unique names for isolation
-    setup_rate_limiter(context)
+    # Start a fresh RateLimiter for each test with unique names for isolation.
+    setup_rate_limiter()
 
     case context[:span_storage] do
       nil -> :ok
@@ -24,17 +24,10 @@ defmodule Sentry.Case do
     end
   end
 
-  defp setup_rate_limiter(context) do
-    if context[:manual_rate_limiting] != true do
-      uid = System.unique_integer([:positive])
-      server_name = :"test_rate_limiter_#{uid}"
-      table_name = :"test_rate_limiter_table_#{uid}"
-
-      opts = [name: server_name, table_name: table_name]
-      start_supervised!({Sentry.Transport.RateLimiter, opts})
-
+  defp setup_rate_limiter do
+      table_name = :"test_rate_limiter_#{System.unique_integer([:positive])}"
       Process.put(:rate_limiter_table_name, table_name)
-    end
+      start_supervised!({Sentry.Transport.RateLimiter, name: table_name}, id: table_name)
   end
 
   defp setup_span_storage(opts) do