fix(callbacks): wrap Oban callbacks

lib/sentry.ex

@@ -214,6 +214,37 @@ defmodule Sentry do
   > SDK's default scrubber means that data only your custom scrubber was dropping is sent to
   > Sentry for as long as that scrubber keeps failing. The error-level log is the only signal.
 
+  ### Oban Callbacks
+
+  The callbacks the Oban integration accepts run inside `:telemetry` handlers, and
+  `:telemetry` permanently detaches a handler that fails. Sentry's two Oban handlers
+  therefore catch every failure in their own body, so a callback of yours that keeps failing
+  never stops later jobs from being reported or checked in.
+
+  No Oban item is dropped because one of these callbacks failed. The two that only return a
+  decision fail open, and the three that customize an item fall back to what the SDK derived
+  on its own:
+
+  | Callback | Behavior after a crash |
+  | --- | --- |
+  | `:should_report_error_callback` | the job error is reported |
+  | `:should_report_error_check_in_callback` | the failed check-in is reported |
+  | `:oban_tags_to_sentry_tags` | the event carries the SDK's own Oban tags (`oban_worker`, `oban_queue`, and `oban_state`) and none of yours |
+  | `:monitor_slug_generator` | the check-in uses the slug derived from the worker name |
+  | `c:Sentry.Integrations.Oban.Cron.sentry_check_in_configuration/1` | the check-in uses the slug and monitor config the integration inferred, with nothing merged in |
+
+  > #### A crashing check-in customization sends the check-in elsewhere {: .warning}
+  >
+  > A check-in whose `:monitor_slug_generator` or `sentry_check_in_configuration/1` failed is
+  > still sent, but under the SDK's default slug rather than the one you configured. For as
+  > long as the callback keeps failing, the monitor you meant to check in to receives nothing
+  > and looks idle, while a monitor under the default slug receives the check-ins instead. The
+  > error-level log is the only signal.
+
+  If a failure happens elsewhere in one of the handlers, the check-in or error event it was
+  about to send is lost. That loss is counted in client reports under `internal_sdk_error`
+  rather than passing silently.
+
   ## Reporting Source Code
 
   Sentry supports reporting the source code of (and around) the line that

lib/sentry/callback.ex

@@ -7,29 +7,28 @@ defmodule Sentry.Callback do
   @type spec() :: (... -> term()) | {module(), atom()} | {module(), atom(), [term()]}
 
   @spec run(atom(), (-> result), result, keyword()) :: result when result: var
-  def run(name, fun, fallback, opts \\ []) when is_list(opts) do
-    case run(name, fun) do
-      {:ok, result} ->
-        result
-
-      :failed ->
-        record_discard(Keyword.get(opts, :discard))
-        fallback
+  def run(name, fun, fallback, opts \\ []) when is_atom(name) and is_list(opts) do
+    case guard(describe_failure(name, Keyword.get(opts, :context)), fun, opts) do
+      {:ok, result} -> result
+      :failed -> fallback
     end
   end
 
   @spec run(atom(), (-> result)) :: {:ok, result} | :failed when result: var
   def run(name, fun) when is_atom(name) do
-    guard("#{inspect(name)} callback failed", fun)
+    guard(describe_failure(name, nil), fun)
   end
 
-  @spec guard(String.t(), (-> result)) :: {:ok, result} | :failed when result: var
-  def guard(description, fun) when is_binary(description) and is_function(fun, 0) do
+  @spec guard(String.t(), (-> result), keyword()) :: {:ok, result} | :failed when result: var
+  def guard(description, fun, opts \\ [])
+      when is_binary(description) and is_function(fun, 0) and is_list(opts) do
     {:ok, fun.()}
   catch
     kind, reason ->
       LoggerUtils.error(description <> ": " <> Exception.format(kind, reason, __STACKTRACE__))
 
+      record_discard(Keyword.get(opts, :discard))
+
       :failed
   end
 
@@ -75,4 +74,7 @@ defmodule Sentry.Callback do
     _ = ClientReport.Sender.record_discarded_events(reason, event_or_data_category)
     :ok
   end
+
+  defp describe_failure(name, nil), do: "#{inspect(name)} callback failed"
+  defp describe_failure(name, context), do: "#{inspect(name)} callback failed #{context}"
 end

lib/sentry/config.ex

@@ -76,6 +76,12 @@ defmodule Sentry.Config do
 
           This example transforms all Oban job tags into Sentry tags prefixed
           with `oban_tags.` and with a value of `true`. *Available since 12.0.0*.
+
+          If the function crashes, the failure is logged at the `:error` level and the event is
+          still reported, carrying only the tags the integration adds itself (`oban_worker`,
+          `oban_queue`, and `oban_state`). A return value that is not a map is logged at the
+          `:warning` level and falls back to those same tags. See the
+          [*Crashing Callbacks*](#module-crashing-callbacks) section below for more information.
           """
         ],
         should_report_error_callback: [
@@ -95,6 +101,10 @@ defmodule Sentry.Config do
 
           This example only reports errors on final retry attempts.
           *Available since 12.0.0*.
+
+          If the function crashes, the failure is logged at the `:error` level and the error is
+          reported, as if the function had returned `true`. See the
+          [*Crashing Callbacks*](#module-crashing-callbacks) section below for more information.
           """
         ],
         cron: [
@@ -119,6 +129,12 @@ defmodule Sentry.Config do
               A `{module, function}` tuple that generates a monitor name based on the `Oban.Job` struct.
               The function is called with the `Oban.Job` as its arguments and must return a string.
               This can be used to customize monitor slugs. *Available since v10.8.0*.
+
+              If the function crashes, the failure is logged at the `:error` level and the
+              check-in is still sent, under the slug the integration derives from the worker
+              name. Until the function is fixed, the monitor you configured it for receives no
+              check-ins at all. See the [*Crashing Callbacks*](#module-crashing-callbacks)
+              section below for more information.
               """
             ],
             should_report_error_check_in_callback: [
@@ -139,6 +155,11 @@ defmodule Sentry.Config do
               This example only reports a failed check-in once all retries are exhausted. While
               retries remain the check-in is left open, so the retry that eventually succeeds
               closes the same check-in. *Available since v13.5.0*.
+
+              If the function crashes, the failure is logged at the `:error` level and the failed
+              check-in is reported, as if the function had returned `true`. See the
+              [*Crashing Callbacks*](#module-crashing-callbacks) section below for more
+              information.
               """
             ]
           ]

lib/sentry/integrations/oban/callbacks.ex

@@ -0,0 +1,43 @@
+defmodule Sentry.Integrations.Oban.Callbacks do
+  @moduledoc false
+
+  alias Sentry.Callback
+  alias Sentry.LoggerUtils
+
+  @spec should_report?(keyword(), atom(), struct()) :: boolean()
+  def should_report?(config, option, job) when is_list(config) and is_atom(option) do
+    case Keyword.get(config, option) do
+      callback when is_function(callback, 2) ->
+        worker = resolve_worker(job)
+
+        Callback.run(
+          option,
+          fn -> callback.(worker, job) == true end,
+          true,
+          context: describe_target(worker, job)
+        )
+
+      _ ->
+        true
+    end
+  end
+
+  @spec describe_target(term(), struct()) :: String.t()
+  def describe_target(worker, job) do
+    "for worker #{inspect(worker)} (job ID #{inspect(job.id)})"
+  end
+
+  defp resolve_worker(job) do
+    case apply(Oban.Worker, :from_string, [job.worker]) do
+      {:ok, mod} ->
+        mod
+
+      {:error, _} ->
+        LoggerUtils.warning(
+          "Could not resolve Oban worker module from string: #{inspect(job.worker)}"
+        )
+
+        nil
+    end
+  end
+end

lib/sentry/integrations/oban/cron.ex

@@ -5,8 +5,9 @@ defmodule Sentry.Integrations.Oban.Cron do
 
   @moduledoc since: "10.9.0"
 
+  alias Sentry.Callback
   alias Sentry.Integrations.CheckInIDMappings
-  alias Sentry.LoggerUtils
+  alias Sentry.Integrations.Oban.Callbacks
 
   @doc """
   The Oban integration calls this callback (if present) to customize
@@ -16,6 +17,13 @@ defmodule Sentry.Integrations.Oban.Cron do
 
   Options returned by this function overwrite any option inferred by the specific
   integration for the check in. We perform *deep merging* of nested keyword options.
+
+  If this callback raises, throws, or exits, the failure is logged at the `:error` level with
+  the `:sentry` logger domain, and the check-in is still sent with the options the integration
+  inferred and nothing merged into them. Since those options include the monitor slug, a
+  check-in that this callback was meant to redirect goes to the monitor named after the worker
+  instead. See the [*Crashing Callbacks*](`m:Sentry#module-crashing-callbacks`) section of the
+  `Sentry` documentation for more information.
   """
   @doc since: "10.9.0"
   @callback sentry_check_in_configuration(oban_job :: struct()) :: options_to_merge :: keyword()
@@ -44,7 +52,13 @@ defmodule Sentry.Integrations.Oban.Cron do
         config
       )
       when event in [:start, :stop, :exception] and mod == Oban.Job and is_binary(cron_expr) do
-    _ = handle_oban_job_event(event, measurements, metadata, config)
+    _ =
+      Callback.guard(
+        describe_failure(metadata.job),
+        fn -> handle_oban_job_event(event, measurements, metadata, config) end,
+        discard: {:internal_sdk_error, "monitor"}
+      )
+
     :ok
   end
 
@@ -55,6 +69,11 @@ defmodule Sentry.Integrations.Oban.Cron do
 
   ## Helpers
 
+  defp describe_failure(job) do
+    "Sentry failed to report an Oban check-in for job #{inspect(job.id)} " <>
+      "(#{inspect(job.worker)})"
+  end
+
   defp handle_oban_job_event(:start, _measurements, metadata, config) do
     if opts = job_to_check_in_opts(metadata.job, config) do
       opts
@@ -96,42 +115,7 @@ defmodule Sentry.Integrations.Oban.Cron do
   end
 
   defp should_report_error_check_in?(job, config) do
-    case Keyword.get(config, :should_report_error_check_in_callback) do
-      callback when is_function(callback, 2) ->
-        call_should_report_error_check_in_callback(callback, job)
-
-      _ ->
-        true
-    end
-  end
-
-  defp call_should_report_error_check_in_callback(callback, job) do
-    worker =
-      case apply(Oban.Worker, :from_string, [job.worker]) do
-        {:ok, mod} ->
-          mod
-
-        {:error, _} ->
-          LoggerUtils.warning(
-            "Could not resolve Oban worker module from string: #{inspect(job.worker)}"
-          )
-
-          nil
-      end
-
-    try do
-      callback.(worker, job) == true
-    rescue
-      error ->
-        LoggerUtils.warning("""
-        :should_report_error_check_in_callback failed for worker #{inspect(worker)} \
-        (job ID #{job.id}):
-
-        #{Exception.format(:error, error, __STACKTRACE__)}\
-        """)
-
-        true
-    end
+    Callbacks.should_report?(config, :should_report_error_check_in_callback, job)
   end
 
   defp job_to_check_in_opts(job, config) when is_struct(job, Oban.Job) do
@@ -146,14 +130,7 @@ defmodule Sentry.Integrations.Oban.Cron do
         monitor_config_opts = maybe_put_timezone_option(monitor_config_opts, job)
         monitor_config_opts = Keyword.merge(monitor_config_opts, schedule_opts)
 
-        monitor_slug =
-          case config[:monitor_slug_generator] do
-            nil ->
-              slugify(job.worker)
-
-            {mod, fun} when is_atom(mod) and is_atom(fun) ->
-              mod |> apply(fun, [job]) |> slugify()
-          end
+        monitor_slug = monitor_slug(job, config[:monitor_slug_generator])
 
         id = CheckInIDMappings.lookup_or_insert_new(job.id)
 
@@ -182,21 +159,35 @@ defmodule Sentry.Integrations.Oban.Cron do
       end
   end
 
-  defp resolve_custom_opts(opts, _job) do
-    opts
-  end
-
   defp resolve_custom_opts(options, mod, per_integration_term) do
     custom_opts =
       if function_exported?(mod, :sentry_check_in_configuration, 1) do
-        mod.sentry_check_in_configuration(per_integration_term)
+        Callback.run(
+          :sentry_check_in_configuration,
+          fn -> mod.sentry_check_in_configuration(per_integration_term) end,
+          [],
+          context: Callbacks.describe_target(mod, per_integration_term)
+        )
       else
         []
       end
 
     deep_merge_keyword(options, custom_opts)
   end
 
+  defp monitor_slug(job, nil) do
+    slugify(job.worker)
+  end
+
+  defp monitor_slug(job, {mod, fun}) when is_atom(mod) and is_atom(fun) do
+    Callback.run(
+      :monitor_slug_generator,
+      fn -> mod |> apply(fun, [job]) |> slugify() end,
+      slugify(job.worker),
+      context: Callbacks.describe_target(job.worker, job)
+    )
+  end
+
   defp deep_merge_keyword(left, right) do
     Keyword.merge(left, right, fn _key, left_val, right_val ->
       if Keyword.keyword?(left_val) and Keyword.keyword?(right_val) do