safe(r) Oban telemetry (#3743)

* safe telemetry

* use try/catch instead

* add some tests

* cleanup tests

---------

Co-authored-by: hq1 <hq@mtod.org>

lib/oban_error_reporter.ex

@@ -1,5 +1,20 @@
 defmodule ObanErrorReporter do
-  def handle_event([:oban, :job, :exception], measure, %{job: job} = meta, _) do
+  require Logger
+
+  def handle_event(name, measurements, metadata, _) do
+    # handling telemetry event in a try/catch block
+    # to avoid handler detachment in the case of an error
+    # see https://hexdocs.pm/telemetry/telemetry.html#attach/4
+    try do
+      handle_event(name, measurements, metadata)
+    catch
+      kind, reason ->
+        message = Exception.format(kind, reason, __STACKTRACE__)
+        Logger.error(message)
+    end
+  end
+
+  defp handle_event([:oban, :job, :exception], measure, %{job: job} = meta) do
     extra =
       job
       |> Map.take([:id, :args, :meta, :queue, :worker])
@@ -10,13 +25,13 @@ defmodule ObanErrorReporter do
     Sentry.capture_exception(meta.reason, stacktrace: meta.stacktrace, extra: extra)
   end
 
-  def handle_event([:oban, :notifier, :exception], _timing, meta, _) do
+  defp handle_event([:oban, :notifier, :exception], _timing, meta) do
     extra = Map.take(meta, ~w(channel payload)a)
 
     Sentry.capture_exception(meta.reason, stacktrace: meta.stacktrace, extra: extra)
   end
 
-  def handle_event([:oban, :plugin, :exception], _timing, meta, _) do
+  defp handle_event([:oban, :plugin, :exception], _timing, meta) do
     extra = Map.take(meta, ~w(plugin)a)
 
     Sentry.capture_exception(meta.reason, stacktrace: meta.stacktrace, extra: extra)

test/workers/oban_error_reporter_test.exs

@@ -0,0 +1,43 @@
+defmodule ObanErrorReporterTest do
+  use ExUnit.Case, async: true
+
+  describe "handle_event/4" do
+    setup do
+      :telemetry.attach_many(
+        "oban-errors-test",
+        [[:oban, :job, :exception], [:oban, :notifier, :exception], [:oban, :plugin, :exception]],
+        &ObanErrorReporter.handle_event/4,
+        %{}
+      )
+
+      on_exit(fn -> :ok = :telemetry.detach("oban-errors-test") end)
+    end
+
+    @tag :capture_log
+    test "doesn't detach on failure" do
+      :ok =
+        :telemetry.execute(
+          [:oban, :job, :exception],
+          _bad_measurements = %{},
+          _bad_metadata = %{job: :bad_job}
+        )
+
+      handlers = :telemetry.list_handlers([:oban, :job, :exception])
+      assert Enum.any?(handlers, &(&1.id == "oban-errors-test"))
+    end
+
+    test "logs an error on failure" do
+      log =
+        ExUnit.CaptureLog.capture_log(fn ->
+          :ok =
+            :telemetry.execute(
+              [:oban, :job, :exception],
+              _bad_measurements = %{},
+              _bad_metadata = %{job: :bad_job}
+            )
+        end)
+
+      assert log =~ "[error] ** (BadMapError) expected a map, got: :bad_job"
+    end
+  end
+end