refactor(oban/cron): consider attempts when reporting failed checkins

lib/sentry/config.ex

@@ -120,6 +120,26 @@ defmodule Sentry.Config do
               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*.
               """
+            ],
+            should_report_error_check_in_callback: [
+              type: {:or, [nil, {:fun, 2}]},
+              default: nil,
+              type_doc: "`(Oban.Worker.t() | nil, Oban.Job.t() -> boolean())` or `nil`",
+              doc: """
+              A function that determines whether to report a failed check-in for an Oban cron
+              job. The function receives the worker module and the `Oban.Job` struct and should
+              return `true` to report the failed check-in or `false` to skip it.
+
+              ```elixir
+              should_report_error_check_in_callback: fn _worker, job ->
+                job.attempt >= job.max_attempts
+              end
+              ```
+
+              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*.
+              """
             ]
           ]
         ]

lib/sentry/integrations/oban/cron.ex

@@ -6,6 +6,7 @@ defmodule Sentry.Integrations.Oban.Cron do
   @moduledoc since: "10.9.0"
 
   alias Sentry.Integrations.CheckInIDMappings
+  alias Sentry.LoggerUtils
 
   @doc """
   The Oban integration calls this callback (if present) to customize
@@ -62,28 +63,74 @@ defmodule Sentry.Integrations.Oban.Cron do
     end
   end
 
+  # A snoozed job is rescheduled rather than finished, so its check-in stays open
+  # until the run that actually completes closes it.
+  defp handle_oban_job_event(:stop, _measurements, %{state: :snoozed}, _config) do
+    :ok
+  end
+
   defp handle_oban_job_event(:stop, measurements, metadata, config) do
-    if opts = job_to_check_in_opts(metadata.job, config) do
-      status =
-        case metadata.state do
-          :success -> :ok
-          :failure -> :error
-          :cancelled -> :ok
-          :discard -> :ok
-          :snoozed -> :ok
-        end
+    status =
+      case metadata.state do
+        :success -> :ok
+        :failure -> :error
+        :cancelled -> :ok
+        :discard -> :ok
+      end
 
-      opts
-      |> Keyword.merge(status: status, duration: duration_in_seconds(measurements))
-      |> Sentry.capture_check_in()
-    end
+    maybe_capture_check_in(status, measurements, metadata, config)
   end
 
   defp handle_oban_job_event(:exception, measurements, metadata, config) do
-    if opts = job_to_check_in_opts(metadata.job, config) do
-      opts
-      |> Keyword.merge(status: :error, duration: duration_in_seconds(measurements))
-      |> Sentry.capture_check_in()
+    maybe_capture_check_in(:error, measurements, metadata, config)
+  end
+
+  defp maybe_capture_check_in(status, measurements, metadata, config) do
+    if status != :error or should_report_error_check_in?(metadata.job, config) do
+      if opts = job_to_check_in_opts(metadata.job, config) do
+        opts
+        |> Keyword.merge(status: status, duration: duration_in_seconds(measurements))
+        |> Sentry.capture_check_in()
+      end
+    end
+  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
   end
 

test/sentry/config_oban_tags_to_sentry_tags_test.exs

@@ -94,4 +94,54 @@ defmodule Sentry.ConfigObanTagsToSentryTagsTest do
       end
     end
   end
+
+  describe "should_report_error_check_in_callback configuration validation" do
+    test "accepts nil" do
+      assert :ok =
+               put_test_config(
+                 integrations: [oban: [cron: [should_report_error_check_in_callback: nil]]]
+               )
+
+      assert Sentry.Config.integrations()[:oban][:cron][:should_report_error_check_in_callback] ==
+               nil
+    end
+
+    test "accepts function with arity 2" do
+      fun = fn _worker, _job -> true end
+
+      assert :ok =
+               put_test_config(
+                 integrations: [oban: [cron: [should_report_error_check_in_callback: fun]]]
+               )
+
+      assert Sentry.Config.integrations()[:oban][:cron][:should_report_error_check_in_callback] ==
+               fun
+    end
+
+    test "rejects function with wrong arity" do
+      fun = fn _job -> true end
+
+      assert_raise ArgumentError,
+                   ~r/invalid value for :should_report_error_check_in_callback/,
+                   fn ->
+                     put_test_config(
+                       integrations: [oban: [cron: [should_report_error_check_in_callback: fun]]]
+                     )
+                   end
+    end
+
+    test "rejects invalid types" do
+      for invalid <- ["invalid", 123, []] do
+        assert_raise ArgumentError,
+                     ~r/invalid value for :should_report_error_check_in_callback/,
+                     fn ->
+                       put_test_config(
+                         integrations: [
+                           oban: [cron: [should_report_error_check_in_callback: invalid]]
+                         ]
+                       )
+                     end
+      end
+    end
+  end
 end

test/sentry/integrations/oban/cron_test.exs

@@ -2,11 +2,19 @@ defmodule Sentry.Integrations.Oban.CronTest do
   alias Sentry.Integrations.CheckInIDMappings
   use Sentry.Case, async: false
 
+  import ExUnit.CaptureLog
   import Sentry.Test.Assertions
   import Sentry.TestHelpers
 
   alias Sentry.Test, as: SentryTest
 
+  defmodule MyCronWorker do
+    use Oban.Worker
+
+    @impl Oban.Worker
+    def perform(_job), do: :ok
+  end
+
   setup context do
     opts = context[:attach_opts] || []
 
@@ -101,8 +109,7 @@ defmodule Sentry.Integrations.Oban.CronTest do
         success: "ok",
         failure: "error",
         cancelled: "ok",
-        discard: "ok",
-        snoozed: "ok"
+        discard: "ok"
       ],
       {frequency, expected_unit} <- [
         {"@hourly", "hour"},
@@ -146,6 +153,19 @@ defmodule Sentry.Integrations.Oban.CronTest do
     end
   end
 
+  test "should not report a check-in when a job is snoozed", %{ref: ref} do
+    :telemetry.execute([:oban, :job, :stop], %{duration: 0}, %{
+      state: :snoozed,
+      job: %Oban.Job{
+        worker: "Sentry.MyWorker",
+        id: 942,
+        meta: %{"cron" => true, "cron_expr" => "@daily"}
+      }
+    })
+
+    refute_sentry_check_in(ref)
+  end
+
   test "captures exception events with monitor config", %{ref: ref} do
     duration = System.convert_time_unit(12_099, :millisecond, :native)
 
@@ -284,6 +304,114 @@ defmodule Sentry.Integrations.Oban.CronTest do
     )
   end
 
+  describe "should_report_error_check_in_callback" do
+    test "should not report a failed check-in when the callback returns false", %{ref: ref} do
+      attach_with_callback(fn _worker, _job -> false end)
+
+      execute_exception_event()
+
+      refute_sentry_check_in(ref)
+    end
+
+    test "should report a failed check-in when the callback returns true", %{ref: ref} do
+      attach_with_callback(fn _worker, _job -> true end)
+
+      execute_exception_event()
+
+      [check_in_body] = SentryTest.collect_sentry_check_ins(ref, 1)
+      assert_sentry_report(check_in_body, status: "error")
+    end
+
+    test "should pass the worker module and the job to the callback", %{ref: ref} do
+      test_pid = self()
+
+      attach_with_callback(fn worker, job ->
+        send(test_pid, {:callback_args, worker, job})
+        false
+      end)
+
+      execute_exception_event()
+
+      assert_receive {:callback_args, MyCronWorker, %Oban.Job{id: 942}}
+      refute_sentry_check_in(ref)
+    end
+
+    test "should report the failed check-in when the callback raises", %{ref: ref} do
+      log =
+        capture_log([metadata: [:domain]], fn ->
+          attach_with_callback(fn _worker, _job -> raise "callback error" end)
+
+          execute_exception_event()
+
+          [check_in_body] = SentryTest.collect_sentry_check_ins(ref, 1)
+          assert_sentry_report(check_in_body, status: "error")
+        end)
+
+      assert log =~ ":should_report_error_check_in_callback failed"
+      assert log =~ "callback error"
+      assert log =~ ~r/domain=(\w+\.)*sentry/
+    end
+
+    test "should pass a nil worker to the callback when the worker cannot be resolved", %{
+      ref: ref
+    } do
+      test_pid = self()
+
+      log =
+        capture_log([metadata: [:domain]], fn ->
+          attach_with_callback(fn worker, _job ->
+            send(test_pid, {:callback_worker, worker})
+            false
+          end)
+
+          execute_exception_event(worker: "NotA.Real.Worker")
+
+          refute_sentry_check_in(ref)
+        end)
+
+      assert_receive {:callback_worker, nil}
+      assert log =~ ~s(Could not resolve Oban worker module from string: "NotA.Real.Worker")
+      assert log =~ ~r/domain=(\w+\.)*sentry/
+    end
+
+    test "should still report the in-progress check-in when the callback returns false", %{
+      ref: ref
+    } do
+      attach_with_callback(fn _worker, _job -> false end)
+
+      :telemetry.execute([:oban, :job, :start], %{}, %{job: cron_job()})
+
+      [check_in_body] = SentryTest.collect_sentry_check_ins(ref, 1)
+      assert_sentry_report(check_in_body, status: "in_progress")
+    end
+  end
+
+  defp attach_with_callback(callback) do
+    :telemetry.detach(Sentry.Integrations.Oban.Cron)
+
+    Sentry.Integrations.Oban.Cron.attach_telemetry_handler(
+      should_report_error_check_in_callback: callback
+    )
+  end
+
+  defp execute_exception_event(job_overrides \\ []) do
+    :telemetry.execute([:oban, :job, :exception], %{duration: 0}, %{
+      state: :failure,
+      job: cron_job(job_overrides)
+    })
+  end
+
+  defp cron_job(overrides \\ []) do
+    struct!(
+      %Oban.Job{
+        worker: inspect(MyCronWorker),
+        id: 942,
+        meta: %{"cron" => true, "cron_expr" => "@daily"}
+      },
+      overrides
+    )
+  end
+
   def custom_name_generator(%Oban.Job{worker: "Sentry.ClientWorker", args: %{"client" => client}}) do
     "Sentry.ClientWorker.#{client}"
   end