fix(callbacks): wrap event pipeline callbacks

lib/sentry.ex

@@ -114,6 +114,10 @@ defmodule Sentry do
   that is, a module implementing the `Sentry.EventFilter` behaviour. This is still supported,
   but is now deprecated. See `Sentry.EventFilter` for more information.
 
+  If the configured filter cannot be called, or its `c:Sentry.EventFilter.exclude_exception?/2`
+  callback crashes, the exception is excluded and the failure is logged. See the
+  [*Crashing Callbacks* section](#module-crashing-callbacks) below.
+
   ## Event Callbacks
 
   You can configure the `:before_send` and `:after_send_event` options to
@@ -149,6 +153,28 @@ defmodule Sentry do
 
   If the `before_send` callback returns `nil` or `false`, the event is not reported.
 
+  ## Crashing Callbacks
+
+  If a `:before_send`, `:after_send_event`, or `:filter` callback raises, throws, or exits,
+  Sentry catches the failure and logs it at the `:error` level instead of letting it reach
+  the code that was reporting the event. The log carries the `:sentry` logger domain, so the
+  SDK never reports its own callback failure as an event. Other configurable callbacks, such
+  as `:before_send_log` and `:before_send_metric`, handle their own failures and are not
+  covered by this section.
+
+  The item being handled is then dropped:
+
+    * A `:before_send` callback that crashes is treated like one that returned `false`.
+      The event or transaction is not sent, and the capture function returns `:excluded`.
+
+    * A `:filter` module that cannot be called, or whose
+      `c:Sentry.EventFilter.exclude_exception?/2` crashes, is treated like one that
+      excluded the exception. `capture_exception/2` returns `:excluded`.
+
+  An `:after_send_event` callback runs once the event has already been sent and its
+  return value is ignored, so a crash there changes nothing the caller sees: the send
+  result is still the one the transport produced.
+
   ## Reporting Source Code
 
   Sentry supports reporting the source code of (and around) the line that
@@ -184,6 +210,7 @@ defmodule Sentry do
   """
 
   alias Sentry.{
+    Callback,
     CheckIn,
     Client,
     ClientError,
@@ -273,7 +300,14 @@ defmodule Sentry do
     event_source = Keyword.get(options, :event_source)
     {send_opts, create_event_opts} = Options.split_send_event_options(options)
 
-    if filter_module.exclude_exception?(exception, event_source) do
+    exclude? =
+      Callback.run(
+        :filter,
+        fn -> filter_module.exclude_exception?(exception, event_source) end,
+        true
+      )
+
+    if exclude? do
       :excluded
     else
       exception

lib/sentry/callback.ex

@@ -0,0 +1,36 @@
+defmodule Sentry.Callback do
+  @moduledoc false
+
+  alias Sentry.LoggerUtils
+
+  @type spec() :: (... -> term()) | {module(), atom()}
+
+  @spec run(atom(), (-> result), result) :: result when result: var
+  def run(name, fun, fallback) when is_atom(name) and is_function(fun, 0) do
+    fun.()
+  catch
+    kind, reason ->
+      LoggerUtils.error(
+        "#{inspect(name)} callback failed: " <>
+          Exception.format(kind, reason, __STACKTRACE__)
+      )
+
+      fallback
+  end
+
+  @spec to_fun(atom(), spec(), [term()]) :: (-> term())
+  def to_fun(name, spec, args) when is_atom(name) and is_list(args) do
+    case spec do
+      fun when is_function(fun, length(args)) ->
+        fn -> apply(fun, args) end
+
+      {mod, fun} when is_atom(mod) and is_atom(fun) ->
+        fn -> apply(mod, fun, args) end
+
+      other ->
+        raise ArgumentError,
+              "#{inspect(name)} must be an anonymous function or a {module, function} tuple, " <>
+                "got: #{inspect(other)}"
+    end
+  end
+end

lib/sentry/client.ex

@@ -6,6 +6,7 @@ defmodule Sentry.Client do
   # See https://develop.sentry.dev/sdk/unified-api/#client.
 
   alias Sentry.{
+    Callback,
     CheckIn,
     ClientError,
     ClientReport,
@@ -197,30 +198,21 @@ defmodule Sentry.Client do
     end
   end
 
-  defp call_before_send(event, function) when is_function(function, 1) do
-    function.(event) || false
+  defp call_before_send(event, callback) do
+    invocation = Callback.to_fun(:before_send, callback, [event])
+    Callback.run(:before_send, fn -> invocation.() || false end, false)
   end
 
-  defp call_before_send(event, {mod, fun}) do
-    apply(mod, fun, [event]) || false
-  end
-
-  defp call_before_send(_event, other) do
-    raise ArgumentError, """
-    :before_send must be an anonymous function or a {module, function} tuple, got: \
-    #{inspect(other)}\
-    """
+  defp maybe_call_after_send(_event_or_transaction, _result, nil) do
+    nil
   end
 
   defp maybe_call_after_send(event_or_transaction, result, callback) do
-    message = ":after_send_event must be an anonymous function or a {module, function} tuple"
-
-    case callback do
-      function when is_function(function, 2) -> function.(event_or_transaction, result)
-      {module, function} -> apply(module, function, [event_or_transaction, result])
-      nil -> nil
-      _ -> raise ArgumentError, message
-    end
+    Callback.run(
+      :after_send_event,
+      Callback.to_fun(:after_send_event, callback, [event_or_transaction, result]),
+      nil
+    )
   end
 
   defp encode_and_send(_event, _result_type = :async, _client, _request_retries) do

lib/sentry/config.ex

@@ -485,7 +485,9 @@ defmodule Sentry.Config do
       default: Sentry.DefaultEventFilter,
       doc: """
       A module that implements the `Sentry.EventFilter`
-      behaviour. Defaults to `Sentry.DefaultEventFilter`. See the
+      behaviour. Defaults to `Sentry.DefaultEventFilter`. If the module cannot be called,
+      or its callback crashes, the failure is logged at the `:error` level and the
+      exception is excluded. See the
       [*Filtering Exceptions* section](#module-filtering-exceptions) below.
       """
     ],
@@ -934,8 +936,10 @@ defmodule Sentry.Config do
       Allows performing operations on the event *before* it is sent as
       well as filtering out the event altogether.
       If the callback returns `nil` or `false`, the event is not reported. If it returns an
-      updated `Sentry.Event`, then the updated event is used instead. See the [*Event Callbacks*
-      section](#module-event-callbacks) below for more information.
+      updated `Sentry.Event`, then the updated event is used instead. If the callback crashes,
+      the failure is logged at the `:error` level and the event is not reported. See the
+      [*Event Callbacks*](#module-event-callbacks) and [*Crashing
+      Callbacks*](#module-crashing-callbacks) sections below for more information.
 
       `:before_send` is available *since v10.0.0*. Before, it was called `:before_send_event`.
       """
@@ -954,8 +958,11 @@ defmodule Sentry.Config do
       doc: """
       Callback that is called *after*
       attempting to send an event. The result of the HTTP call as well as the event will
-      be passed as arguments. The return value of the callback is not returned. See the
-      [*Event Callbacks* section](#module-event-callbacks) below for more information.
+      be passed as arguments. The return value of the callback is not returned. If the
+      callback crashes, the failure is logged at the `:error` level and the caller still
+      receives the result of the send. See the [*Event Callbacks*](#module-event-callbacks)
+      and [*Crashing Callbacks*](#module-crashing-callbacks) sections below for more
+      information.
       """
     ],
     before_send_log: [

lib/sentry/event_filter.ex

@@ -109,6 +109,11 @@ defmodule Sentry.EventFilter do
   will have `:plug` as a source and events from `Sentry.LoggerBackend`
   will have `:logger` as the source. A custom source can also be specified
   by passing the `:event_source` option to `Sentry.capture_exception/2`.
+
+  If this callback raises, throws, or exits, or if the configured module does not
+  export it, the failure is logged at the `:error` level and never reaches the code
+  that was reporting the exception. The exception is excluded, so
+  `Sentry.capture_exception/2` returns `:excluded`.
   """
   @callback exclude_exception?(exception :: Exception.t(), source :: atom) :: boolean
 end

test/sentry_test.exs

@@ -16,6 +16,12 @@ defmodule SentryTest do
     def exclude_exception?(_, _), do: false
   end
 
+  defmodule RaisingFilter do
+    @behaviour Sentry.EventFilter
+
+    def exclude_exception?(_exception, _source), do: raise("filter is broken")
+  end
+
   setup do
     SentryTest.setup_sentry(dedup_events: false)
   end
@@ -163,6 +169,154 @@ defmodule SentryTest do
     assert :counters.get(request_count, 1) == 2
   end
 
+  describe "a :before_send callback that crashes" do
+    test "drops the event and returns :excluded when the callback raises" do
+      put_test_config(before_send: fn _event -> raise "before_send is broken" end)
+
+      log =
+        capture_log(fn ->
+          assert :excluded = Sentry.capture_message("raising before_send", result: :sync)
+        end)
+
+      assert log =~ ":before_send callback failed"
+      assert log =~ "before_send is broken"
+      assert SentryTest.pop_sentry_reports() == []
+    end
+
+    test "drops the event and returns :excluded when the callback throws" do
+      put_test_config(before_send: fn _event -> throw(:before_send_is_broken) end)
+
+      log =
+        capture_log(fn ->
+          assert :excluded = Sentry.capture_message("throwing before_send", result: :sync)
+        end)
+
+      assert log =~ ":before_send callback failed"
+      assert log =~ "before_send_is_broken"
+      assert SentryTest.pop_sentry_reports() == []
+    end
+
+    test "drops the exception and returns :excluded when the callback exits" do
+      put_test_config(before_send: fn _event -> exit(:before_send_is_broken) end)
+
+      log =
+        capture_log(fn ->
+          assert :excluded =
+                   Sentry.capture_exception(%RuntimeError{message: "oops"}, result: :sync)
+        end)
+
+      assert log =~ ":before_send callback failed"
+      assert log =~ "before_send_is_broken"
+      assert SentryTest.pop_sentry_reports() == []
+    end
+
+    test "drops the transaction and returns :excluded" do
+      transaction = create_transaction(%{transaction: "crashing-before-send-transaction"})
+
+      log =
+        capture_log(fn ->
+          assert :excluded =
+                   Sentry.send_transaction(transaction,
+                     result: :sync,
+                     before_send: fn _transaction -> exit(:before_send_is_broken) end
+                   )
+        end)
+
+      assert log =~ ":before_send callback failed"
+      assert SentryTest.pop_sentry_reports() == []
+    end
+
+    test "does not report its own failure back to Sentry" do
+      test_pid = self()
+      ref = make_ref()
+      handler_name = :"sentry_handler_#{System.unique_integer([:positive])}"
+
+      :ok =
+        :logger.add_handler(handler_name, Sentry.LoggerHandler, %{
+          config: %{capture_log_messages: true, level: :debug}
+        })
+
+      on_exit(fn -> _ = :logger.remove_handler(handler_name) end)
+
+      put_test_config(
+        before_send: fn _event ->
+          send(test_pid, {ref, :called})
+          raise "before_send is broken"
+        end
+      )
+
+      capture_log(fn ->
+        assert :excluded = Sentry.capture_message("self-reporting before_send", result: :sync)
+      end)
+
+      assert_received {^ref, :called}
+      refute_received {^ref, :called}
+    end
+  end
+
+  describe "an :after_send_event callback that crashes" do
+    test "still returns the successful send result for an event" do
+      put_test_config(after_send_event: fn _event, _result -> raise "after_send is broken" end)
+
+      log =
+        capture_log(fn ->
+          assert {:ok, _id} = Sentry.capture_message("raising after_send", result: :sync)
+        end)
+
+      assert log =~ ":after_send_event callback failed"
+      assert log =~ "after_send is broken"
+
+      assert_sentry_report(:event, message: %{formatted: "raising after_send"})
+    end
+
+    test "still returns the successful send result for a transaction" do
+      transaction = create_transaction(%{transaction: "crashing-after-send-transaction"})
+
+      log =
+        capture_log(fn ->
+          assert {:ok, _id} =
+                   Sentry.send_transaction(transaction,
+                     result: :sync,
+                     after_send_event: fn _transaction, _result -> exit(:after_send_is_broken) end
+                   )
+        end)
+
+      assert log =~ ":after_send_event callback failed"
+      assert log =~ "after_send_is_broken"
+
+      assert_sentry_report(:transaction, transaction: "crashing-after-send-transaction")
+    end
+  end
+
+  describe "a :filter callback that crashes" do
+    test "drops the exception and returns :excluded when the filter raises" do
+      put_test_config(filter: RaisingFilter)
+
+      log =
+        capture_log(fn ->
+          assert :excluded =
+                   Sentry.capture_exception(%RuntimeError{message: "oops"}, result: :sync)
+        end)
+
+      assert log =~ ":filter callback failed"
+      assert log =~ "filter is broken"
+      assert SentryTest.pop_sentry_reports() == []
+    end
+
+    test "drops the exception and returns :excluded when the filter cannot be called" do
+      put_test_config(filter: __MODULE__.MissingFilter)
+
+      log =
+        capture_log(fn ->
+          assert :excluded =
+                   Sentry.capture_exception(%RuntimeError{message: "oops"}, result: :sync)
+        end)
+
+      assert log =~ ":filter callback failed"
+      assert SentryTest.pop_sentry_reports() == []
+    end
+  end
+
   describe "send_check_in/1" do
     test "posts a check-in with all the explicit arguments", %{bypass: bypass} do
       put_test_config(environment_name: "test", release: "1.3.2")