fix(tests): accept nil as the DSN value again (#1044)

* fix(tests): accept `nil` as the DSN value again

* fixup: ensure dedupe works fine when ignoring

* feat(tests): ensure user callbacks are skipped when dsn is nil in prod/dev

* tests: add basic tests for prod mode

Author: solnic

lib/sentry.ex

@@ -380,12 +380,6 @@ defmodule Sentry do
         ClientReport.Sender.record_discarded_events(:event_processor, [event])
         :ignored
 
-      !Config.dsn() ->
-        # We still validate options even if we're not sending the event. This aims at catching
-        # configuration issues during development instead of only when deploying to production.
-        _options = NimbleOptions.validate!(options, Options.send_event_schema())
-        :ignored
-
       included_envs == :all or to_string(Config.environment_name()) in included_envs ->
         Client.send_event(event, options)
 
@@ -399,12 +393,6 @@ defmodule Sentry do
     included_envs = Config.included_environments()
 
     cond do
-      !Config.dsn() ->
-        # We still validate options even if we're not sending the event. This aims at catching
-        # configuration issues during development instead of only when deploying to production.
-        _options = NimbleOptions.validate!(options, Options.send_event_schema())
-        :ignored
-
       included_envs == :all or to_string(Config.environment_name()) in included_envs ->
         Client.send_transaction(transaction, options)
 
@@ -458,13 +446,9 @@ defmodule Sentry do
   @spec capture_check_in(keyword()) ::
           {:ok, check_in_id :: String.t()} | :ignored | {:error, ClientError.t()}
   def capture_check_in(options) when is_list(options) do
-    if Config.dsn() do
-      options
-      |> CheckIn.new()
-      |> Client.send_check_in(options)
-    else
-      :ignored
-    end
+    options
+    |> CheckIn.new()
+    |> Client.send_check_in(options)
   end
 
   @doc ~S"""

lib/sentry/client.ex

@@ -27,30 +27,35 @@ defmodule Sentry.Client do
   @max_message_length 8_192
 
   @spec send_check_in(CheckIn.t(), keyword()) ::
-          {:ok, check_in_id :: String.t()} | {:error, ClientError.t()}
+          {:ok, check_in_id :: String.t()} | :ignored | {:error, ClientError.t()}
   def send_check_in(%CheckIn{} = check_in, opts) when is_list(opts) do
-    if Config.telemetry_processor_category?(:check_in) do
-      case TelemetryProcessor.add(check_in) do
-        {:ok, {:rate_limited, data_category}} ->
-          ClientReport.Sender.record_discarded_events(:ratelimit_backoff, data_category)
+    cond do
+      is_nil(Config.dsn()) ->
+        :ignored
 
-        :ok ->
-          :ok
-      end
+      Config.telemetry_processor_category?(:check_in) ->
+        case TelemetryProcessor.add(check_in) do
+          {:ok, {:rate_limited, data_category}} ->
+            ClientReport.Sender.record_discarded_events(:ratelimit_backoff, data_category)
 
-      {:ok, check_in.check_in_id}
-    else
-      client = Keyword.get_lazy(opts, :client, &Config.client/0)
+          :ok ->
+            :ok
+        end
 
-      # This is a "private" option, only really used in testing.
-      request_retries =
-        Keyword.get_lazy(opts, :request_retries, fn ->
-          Application.get_env(:sentry, :request_retries, Transport.default_retries())
-        end)
+        {:ok, check_in.check_in_id}
+
+      true ->
+        client = Keyword.get_lazy(opts, :client, &Config.client/0)
 
-      check_in
-      |> Envelope.from_check_in()
-      |> Transport.encode_and_post_envelope(client, request_retries)
+        # This is a "private" option, only really used in testing.
+        request_retries =
+          Keyword.get_lazy(opts, :request_retries, fn ->
+            Application.get_env(:sentry, :request_retries, Transport.default_retries())
+          end)
+
+        check_in
+        |> Envelope.from_check_in()
+        |> Transport.encode_and_post_envelope(client, request_retries)
     end
   end
 
@@ -59,6 +64,7 @@ defmodule Sentry.Client do
   @spec send_event(Event.t(), keyword()) ::
           {:ok, event_id :: String.t()}
           | {:error, ClientError.t()}
+          | :ignored
           | :unsampled
           | :excluded
   def send_event(%Event{} = event, opts) when is_list(opts) do
@@ -78,6 +84,7 @@ defmodule Sentry.Client do
 
     result =
       with {:ok, %Event{} = event} <- maybe_call_before_send(event, before_send),
+           :ok <- ensure_dsn_configured(),
            :ok <- sample_event(sample_rate),
            :ok <- maybe_dedupe(event) do
         send_result = encode_and_send(event, result_type, client, request_retries)
@@ -99,6 +106,9 @@ defmodule Sentry.Client do
       :excluded ->
         :excluded
 
+      :ignored ->
+        :ignored
+
       {:error, %ClientError{} = error} ->
         {:error, error}
     end
@@ -120,6 +130,7 @@ defmodule Sentry.Client do
   @spec send_transaction(Transaction.t(), keyword()) ::
           {:ok, transaction_id :: String.t()}
           | {:error, ClientError.t()}
+          | :ignored
           | :excluded
   def send_transaction(%Transaction{} = transaction, opts \\ []) do
     opts = NimbleOptions.validate!(opts, Options.send_transaction_schema())
@@ -134,16 +145,24 @@ defmodule Sentry.Client do
         Application.get_env(:sentry, :request_retries, Transport.default_retries())
       end)
 
-    with {:ok, %Transaction{} = transaction} <- maybe_call_before_send(transaction, before_send) do
+    with {:ok, %Transaction{} = transaction} <- maybe_call_before_send(transaction, before_send),
+         :ok <- ensure_dsn_configured() do
       send_result = encode_and_send(transaction, result_type, client, request_retries)
       _ignored = maybe_call_after_send(transaction, send_result, after_send_event)
       send_result
     else
       :excluded ->
         :excluded
+
+      :ignored ->
+        :ignored
     end
   end
 
+  defp ensure_dsn_configured do
+    if Config.dsn(), do: :ok, else: :ignored
+  end
+
   defp sample_event(sample_rate) do
     cond do
       sample_rate == 1 -> :ok

lib/sentry/config.ex

@@ -929,7 +929,7 @@ defmodule Sentry.Config do
   def hackney_opts, do: fetch!(:hackney_opts)
 
   @spec before_send() :: (Sentry.Event.t() -> Sentry.Event.t()) | {module(), atom()} | nil
-  def before_send, do: get(:before_send)
+  def before_send, do: compose_send_callback(:before_send)
 
   @spec after_send_event() ::
           (Sentry.Event.t(), term() -> Sentry.Event.t()) | {module(), atom()} | nil
@@ -1028,11 +1028,49 @@ defmodule Sentry.Config do
 
   @spec before_send_log() ::
           (Sentry.LogEvent.t() -> Sentry.LogEvent.t() | nil | false) | {module(), atom()} | nil
-  def before_send_log, do: get(:before_send_log)
+  def before_send_log, do: compose_send_callback(:before_send_log)
 
   @spec before_send_metric() ::
           (Sentry.Metric.t() -> Sentry.Metric.t() | nil | false) | {module(), atom()} | nil
-  def before_send_metric, do: get(:before_send_metric)
+  def before_send_metric, do: compose_send_callback(:before_send_metric)
+
+  # Composes the user-provided callback (under `key`) with an internal callback
+  # (under `internal_<key>`). In production/development the user-provided
+  # callback is dropped when there is no DSN, so callbacks never run for events
+  # that won't be sent. In test mode the user-provided callback is always
+  # honored — tests routinely use `dsn: nil` to assert callback behavior in
+  # isolation, and dropping there would break those contracts.
+  defp compose_send_callback(key) do
+    user_callback = if user_callbacks_enabled?(), do: get(key), else: nil
+    internal_callback = get(internal_callback_key(key))
+
+    case {user_callback, internal_callback} do
+      {nil, nil} -> nil
+      {user, nil} -> user
+      {nil, internal} -> internal
+      {user, internal} -> chain_send_callbacks(user, internal)
+    end
+  end
+
+  defp user_callbacks_enabled? do
+    not is_nil(dsn()) or test_mode?()
+  end
+
+  defp internal_callback_key(:before_send), do: :_internal_before_send
+  defp internal_callback_key(:before_send_log), do: :_internal_before_send_log
+  defp internal_callback_key(:before_send_metric), do: :_internal_before_send_metric
+
+  defp chain_send_callbacks(first, second) do
+    fn struct ->
+      case apply_send_callback(first, struct) do
+        result when result == nil or result == false -> result
+        result -> apply_send_callback(second, result)
+      end
+    end
+  end
+
+  defp apply_send_callback(fun, struct) when is_function(fun, 1), do: fun.(struct)
+  defp apply_send_callback({mod, fun}, struct), do: apply(mod, fun, [struct])
 
   @spec put_config(atom(), term()) :: :ok
   def put_config(key, value) when is_atom(key) do

lib/sentry/test.ex

@@ -650,30 +650,6 @@ defmodule Sentry.Test do
     # Store in process dict for pop_* lookups
     Process.put(:sentry_test_collector, collector_table)
 
-    # Extract user-provided callbacks from extra_config (if any), falling back to current config
-    {user_before_send, extra_config} = Keyword.pop(extra_config, :before_send)
-    {user_before_send_event, extra_config} = Keyword.pop(extra_config, :before_send_event)
-    {user_before_send_log, extra_config} = Keyword.pop(extra_config, :before_send_log)
-    {user_before_send_metric, extra_config} = Keyword.pop(extra_config, :before_send_metric)
-
-    # Use the caller-only registry lookup instead of `Sentry.Config.before_send/0`
-    # so the captured "original" callback is only this test's override (or the
-    # global default), never another concurrent test's wrapping callback.
-    original_before_send =
-      user_before_send || user_before_send_event ||
-        original_config_value(:before_send)
-
-    original_before_send_log =
-      user_before_send_log || original_config_value(:before_send_log)
-
-    original_before_send_metric =
-      user_before_send_metric || original_config_value(:before_send_metric)
-
-    # Build collecting callbacks that wrap the originals
-    new_before_send = build_collecting_callback(original_before_send)
-    new_before_send_log = build_collecting_callback(original_before_send_log)
-    new_before_send_metric = build_collecting_callback(original_before_send_metric)
-
     # Always set a per-test DSN override. When no DSN is provided, use the
     # default Bypass DSN.
     extra_config =
@@ -686,17 +662,20 @@ defmodule Sentry.Test do
         end
       end
 
-    config =
-      [finch_request_opts: [receive_timeout: 2000]]
-      |> Keyword.merge(extra_config)
-      |> Keyword.merge(
-        before_send: new_before_send,
-        before_send_log: new_before_send_log,
-        before_send_metric: new_before_send_metric
-      )
+    config = Keyword.merge([finch_request_opts: [receive_timeout: 2000]], extra_config)
 
     put_test_config(config)
 
+    # Install standalone collecting callbacks under internal slots. They're
+    # composed with the user-provided :before_send / :before_send_log /
+    # :before_send_metric in `Sentry.Config` based on DSN value: when DSN is
+    # `nil`, only these collecting callbacks run; when DSN is set, the user's
+    # callback runs first and its result is collected.
+    collector = build_collecting_callback()
+    Sentry.Test.Config.put_override(:_internal_before_send, collector)
+    Sentry.Test.Config.put_override(:_internal_before_send_log, collector)
+    Sentry.Test.Config.put_override(:_internal_before_send_metric, collector)
+
     scheduler_pid = get_scheduler_pid()
 
     if scheduler_pid do
@@ -752,18 +731,11 @@ defmodule Sentry.Test do
     end)
   end
 
-  # Reads `key` from this test's per-process scope (or any caller's scope on
-  # `[self() | $callers]`), falling back to the global config value. Skips the
-  # full namespace resolver so the captured "original" callback is never
-  # another concurrent test's wrapping callback.
-  defp original_config_value(key) do
-    case Sentry.Test.Scope.Registry.lookup_caller_override(key) do
-      {:ok, value} -> value
-      :default -> :persistent_term.get({:sentry_config, key}, nil)
-    end
-  end
-
-  defp build_collecting_callback(nil) do
+  # Standalone collecting callback. Records the struct in this test's
+  # collector ETS table when one is registered for the calling process (or any
+  # of its callers), then returns the struct unchanged so it flows through any
+  # remaining pipeline stages.
+  defp build_collecting_callback do
     fn struct ->
       case find_collector() do
         nil -> :ok
@@ -774,28 +746,6 @@ defmodule Sentry.Test do
     end
   end
 
-  defp build_collecting_callback({mod, fun}) do
-    build_collecting_callback(Function.capture(mod, fun, 1))
-  end
-
-  defp build_collecting_callback(original) when is_function(original, 1) do
-    fn struct ->
-      # Guard on find_collector/0 so that a test-specific callback stored in
-      # :persistent_term is never invoked from an unrelated async test's process.
-      # When a collector IS found, call the original first so user-defined
-      # filtering/modification is applied before we collect the result.
-      case find_collector() do
-        nil ->
-          struct
-
-        table ->
-          result = original.(struct)
-          unless is_nil(result), do: collect_struct(table, result)
-          result
-      end
-    end
-  end
-
   defp collect_struct(table, struct) do
     :ets.insert(table, {System.unique_integer([:monotonic]), struct})
   end

lib/sentry/test/config.ex

@@ -146,6 +146,19 @@ defmodule Sentry.Test.Config do
     Registry.strict_allow!(owner_pid, allowed_pid)
   end
 
+  @doc false
+  @spec put_override(atom(), term()) :: :ok
+  def put_override(key, value) when is_atom(key) do
+    _ =
+      Registry.update(self(), fn scope ->
+        Scope.put_override(scope, key, value)
+      end)
+
+    auto_allow_globals()
+
+    :ok
+  end
+
   ## Private helpers
 
   defp validate_and_rename({key, value}) do