Improve Oban error reporting (#872)

Author: whatyouhide

lib/sentry/integrations/oban/error_reporter.ex

@@ -26,39 +26,61 @@ defmodule Sentry.Integrations.Oban.ErrorReporter do
   def handle_event([:oban, :job, :exception], _measurements, %{job: job} = _metadata, :no_config) do
     %{reason: reason, stacktrace: stacktrace} = job.unsaved_error
 
+    if report?(reason) do
+      report(job, reason, stacktrace)
+    else
+      :ok
+    end
+  end
+
+  defp report(job, reason, stacktrace) do
     stacktrace =
       case {apply(Oban.Worker, :from_string, [job.worker]), stacktrace} do
         {{:ok, atom_worker}, []} -> [{atom_worker, :process, 1, []}]
         _ -> stacktrace
       end
 
-    fingerprint_opts =
-      if is_exception(reason) do
-        [inspect(reason.__struct__), Exception.message(reason)]
-      else
-        [inspect(reason)]
-      end
-
     opts =
       [
         stacktrace: stacktrace,
         tags: %{oban_worker: job.worker, oban_queue: job.queue, oban_state: job.state},
-        fingerprint: [job.worker] ++ fingerprint_opts,
+        fingerprint: [job.worker, "{{ default }}"],
         extra:
           Map.take(job, [:args, :attempt, :id, :max_attempts, :meta, :queue, :tags, :worker]),
         integration_meta: %{oban: %{job: job}}
       ]
 
     _ =
-      if is_exception(reason) do
-        Sentry.capture_exception(reason, opts)
-      else
-        Sentry.capture_message(
-          "Oban job #{job.worker} errored out: %s",
-          opts ++ [interpolation_parameters: [inspect(reason)]]
-        )
+      case maybe_unwrap_exception(reason) do
+        exception when is_exception(exception) ->
+          Sentry.capture_exception(exception, opts)
+
+        _other ->
+          Sentry.capture_message(
+            "Oban job #{job.worker} errored out: %s",
+            opts ++ [interpolation_parameters: [inspect(reason)]]
+          )
       end
 
     :ok
   end
+
+  # Oban.PerformError also wraps {:discard, _} and {:cancel, _} tuples, but those are
+  # not *errors* and should not be reported to Sentry automatically.
+  defp report?(%{reason: {type, _reason}} = error) when is_exception(error, Oban.PerformError) do
+    type == :error
+  end
+
+  defp report?(_error) do
+    true
+  end
+
+  defp maybe_unwrap_exception(%{reason: {:error, error}} = perform_error)
+       when is_exception(perform_error, Oban.PerformError) and is_exception(error) do
+    error
+  end
+
+  defp maybe_unwrap_exception(reason) do
+    reason
+  end
 end

test/sentry/integrations/oban/error_reporter_test.exs

@@ -10,28 +10,44 @@ defmodule Sentry.Integrations.Oban.ErrorReporterTest do
     def perform(%Oban.Job{}), do: :ok
   end
 
+  @worker_as_string "Sentry.Integrations.Oban.ErrorReporterTest.MyWorker"
+
   describe "handle_event/4" do
     test "reports the correct error to Sentry" do
-      # Any worker is okay here, this is just an easier way to get a job struct.
-      job =
-        %{"id" => "123", "entity" => "user", "type" => "delete"}
-        |> MyWorker.new()
-        |> Ecto.Changeset.apply_action!(:validate)
-        |> Map.replace!(:unsaved_error, %{
-          reason: %RuntimeError{message: "oops"},
-          kind: :error,
-          stacktrace: []
-        })
+      Sentry.Test.start_collecting()
+
+      emit_telemetry_for_failed_job(:error, %RuntimeError{message: "oops"}, [])
+
+      assert [event] = Sentry.Test.pop_sentry_reports()
+      assert event.original_exception == %RuntimeError{message: "oops"}
+      assert [%{stacktrace: %{frames: [stacktrace]}} = exception] = event.exception
+
+      assert exception.type == "RuntimeError"
+      assert exception.value == "oops"
+      assert exception.mechanism.handled == true
+      assert stacktrace.module == MyWorker
+
+      assert stacktrace.function ==
+               "Sentry.Integrations.Oban.ErrorReporterTest.MyWorker.process/1"
+
+      assert event.tags.oban_queue == "default"
+      assert event.tags.oban_state == "available"
+      assert event.tags.oban_worker == "Sentry.Integrations.Oban.ErrorReporterTest.MyWorker"
+      assert %{job: %Oban.Job{}} = event.integration_meta.oban
 
+      assert event.fingerprint == [@worker_as_string, "{{ default }}"]
+    end
+
+    test "unwraps Oban.PerformErrors and reports the wrapped error" do
       Sentry.Test.start_collecting()
 
-      assert :ok =
-               ErrorReporter.handle_event(
-                 [:oban, :job, :exception],
-                 %{},
-                 %{job: job},
-                 :no_config
-               )
+      emit_telemetry_for_failed_job(
+        :error,
+        %Oban.PerformError{
+          reason: {:error, %RuntimeError{message: "oops"}}
+        },
+        []
+      )
 
       assert [event] = Sentry.Test.pop_sentry_reports()
       assert event.original_exception == %RuntimeError{message: "oops"}
@@ -49,50 +65,67 @@ defmodule Sentry.Integrations.Oban.ErrorReporterTest do
       assert event.tags.oban_state == "available"
       assert event.tags.oban_worker == "Sentry.Integrations.Oban.ErrorReporterTest.MyWorker"
       assert %{job: %Oban.Job{}} = event.integration_meta.oban
+
+      assert event.fingerprint == [@worker_as_string, "{{ default }}"]
     end
 
     test "reports non-exception errors to Sentry" do
-      job =
-        %{"id" => "123", "entity" => "user", "type" => "delete"}
-        |> MyWorker.new()
-        |> Ecto.Changeset.apply_action!(:validate)
-        |> Map.replace!(:unsaved_error, %{
-          reason: :undef,
-          kind: :error,
-          stacktrace: []
-        })
-
       Sentry.Test.start_collecting()
 
-      assert :ok =
-               ErrorReporter.handle_event(
-                 [:oban, :job, :exception],
-                 %{},
-                 %{job: job},
-                 :no_config
-               )
+      emit_telemetry_for_failed_job(:error, :undef, [])
 
       assert [event] = Sentry.Test.pop_sentry_reports()
       assert %{job: %Oban.Job{}} = event.integration_meta.oban
 
       assert event.message == %Sentry.Interfaces.Message{
-               formatted:
-                 "Oban job Sentry.Integrations.Oban.ErrorReporterTest.MyWorker errored out: :undef",
-               message:
-                 "Oban job Sentry.Integrations.Oban.ErrorReporterTest.MyWorker errored out: %s",
+               formatted: "Oban job #{@worker_as_string} errored out: :undef",
+               message: "Oban job #{@worker_as_string} errored out: %s",
                params: [":undef"]
              }
 
       assert [%Sentry.Interfaces.Thread{stacktrace: %{frames: [stacktrace]}}] = event.threads
-
       assert stacktrace.module == MyWorker
-
-      assert stacktrace.function ==
-               "Sentry.Integrations.Oban.ErrorReporterTest.MyWorker.process/1"
+      assert stacktrace.function == "#{@worker_as_string}.process/1"
 
       assert event.tags.oban_queue == "default"
       assert event.tags.oban_state == "available"
-      assert event.tags.oban_worker == "Sentry.Integrations.Oban.ErrorReporterTest.MyWorker"
+      assert event.tags.oban_worker == @worker_as_string
+
+      assert event.fingerprint == [@worker_as_string, "{{ default }}"]
     end
+
+    for reason <- [:cancel, :discard] do
+      test "doesn't report Oban.PerformError with reason #{inspect(reason)}" do
+        Sentry.Test.start_collecting()
+
+        emit_telemetry_for_failed_job(
+          :error,
+          %Oban.PerformError{reason: {unquote(reason), "nah"}},
+          []
+        )
+
+        assert Sentry.Test.pop_sentry_reports() == []
+      end
+    end
+  end
+
+  ## Helpers
+
+  defp emit_telemetry_for_failed_job(kind, reason, stacktrace) do
+    job =
+      %{"id" => "123", "entity" => "user", "type" => "delete"}
+      |> MyWorker.new()
+      |> Ecto.Changeset.apply_action!(:validate)
+      |> Map.replace!(:unsaved_error, %{kind: kind, reason: reason, stacktrace: stacktrace})
+
+    assert :ok =
+             ErrorReporter.handle_event(
+               [:oban, :job, :exception],
+               %{},
+               %{job: job},
+               :no_config
+             )
+
+    job
   end
 end