Refactor and improve docs for client reports (#810)

Co-authored-by: Savannah Manning <sm05908@gmail.com>

lib/sentry.ex

@@ -341,7 +341,7 @@ defmodule Sentry do
     cond do
       is_nil(event.message) and event.exception == [] ->
         LoggerUtils.log("Cannot report event without message or exception: #{inspect(event)}")
-        ClientReport.record_discarded_events(:event_processor, [event])
+        ClientReport.Sender.record_discarded_events(:event_processor, [event])
         :ignored
 
       # If we're in test mode, let's send the event down the pipeline anyway.

lib/sentry/application.ex

@@ -26,7 +26,7 @@ defmodule Sentry.Application do
         {Registry, keys: :unique, name: Sentry.Transport.SenderRegistry},
         Sentry.Sources,
         Sentry.Dedupe,
-        Sentry.ClientReport,
+        Sentry.ClientReport.Sender,
         {Sentry.Integrations.CheckInIDMappings,
          [
            max_expected_check_in_time:

lib/sentry/client.ex

@@ -82,7 +82,7 @@ defmodule Sentry.Client do
       :unsampled ->
         # See https://github.com/getsentry/develop/pull/551/files
         Sentry.put_last_event_id_and_source(event.event_id, event.source)
-        ClientReport.record_discarded_events(:sample_rate, [event])
+        ClientReport.Sender.record_discarded_events(:sample_rate, [event])
         :unsampled
 
       :excluded ->

lib/sentry/client_report.ex

@@ -1,12 +1,13 @@
 defmodule Sentry.ClientReport do
   @moduledoc """
-  A struct and GenServer implementation to represent and manage **client reports** for Sentry.
+  This module represents the data structure for a **client report**.
 
-  Client reports are used to provide insights into which events are being dropped and for what reason.
+  Client reports are used to provide insights into which events are being
+  dropped (that is, not sent to Sentry) and for what reason.
 
   This module is responsible for recording, storing, and periodically sending these client
   reports to Sentry. You can choose to turn off these reports by configuring the
-  option `send_client_reports?`.
+  `:send_client_reports` option.
 
   Refer to <https://develop.sentry.dev/sdk/client-reports/> for more details.
 
@@ -15,9 +16,6 @@ defmodule Sentry.ClientReport do
 
   @moduledoc since: "10.8.0"
 
-  use GenServer
-  alias Sentry.{Client, Config, Envelope}
-
   @client_report_reasons [
     :ratelimit_backoff,
     :queue_overflow,
@@ -48,100 +46,10 @@ defmodule Sentry.ClientReport do
           discarded_events: [%{reason: reason(), category: String.t(), quantity: pos_integer()}]
         }
 
+  @enforce_keys [:timestamp, :discarded_events]
   defstruct [:timestamp, discarded_events: %{}]
 
-  @send_interval 30_000
-
-  @doc false
-  @spec start_link([]) :: GenServer.on_start()
-  def start_link(opts \\ []) do
-    GenServer.start_link(__MODULE__, %{}, name: Keyword.get(opts, :name, __MODULE__))
-  end
-
-  @doc false
-  @spec record_discarded_events(
-          reason(),
-          [item]
-        ) :: :ok
-        when item:
-               Sentry.Attachment.t()
-               | Sentry.CheckIn.t()
-               | Sentry.ClientReport.t()
-               | Sentry.Event.t()
-  def record_discarded_events(reason, event_items, genserver \\ __MODULE__)
-      when is_list(event_items) do
-    if Enum.member?(@client_report_reasons, reason) do
-      _ =
-        event_items
-        |> Enum.each(
-          &GenServer.cast(
-            genserver,
-            {:record_discarded_events, reason, Envelope.get_data_category(&1)}
-          )
-        )
-    end
-
-    # We silently ignore events whose reasons aren't valid because we have to add it to the allowlist in Snuba
-    # https://develop.sentry.dev/sdk/client-reports/
-
-    :ok
-  end
-
-  @doc false
-  @impl true
-  def init(state) do
-    schedule_report()
-    {:ok, state}
-  end
-
-  @doc false
-  @impl true
-  def handle_cast({:record_discarded_events, reason, category}, discarded_events) do
-    {:noreply, Map.update(discarded_events, {reason, category}, 1, &(&1 + 1))}
-  end
-
   @doc false
-  @impl true
-  def handle_info(:send_report, discarded_events) do
-    if map_size(discarded_events) != 0 do
-      discarded_events =
-        discarded_events
-        |> Enum.map(fn {{reason, category}, quantity} ->
-          %{
-            reason: reason,
-            category: category,
-            quantity: quantity
-          }
-        end)
-
-      client_report =
-        %__MODULE__{
-          timestamp: timestamp(),
-          discarded_events: discarded_events
-        }
-
-      _ =
-        if Config.dsn() != nil && Config.send_client_reports?() do
-          Client.send_client_report(client_report)
-        end
-
-      schedule_report()
-      {:noreply, %{}}
-    else
-      # state is nil so nothing to send but keep looping
-      schedule_report()
-      {:noreply, %{}}
-    end
-  end
-
-  defp schedule_report do
-    Process.send_after(self(), :send_report, @send_interval)
-  end
-
-  defp timestamp do
-    DateTime.utc_now()
-    |> DateTime.truncate(:second)
-    |> DateTime.to_iso8601()
-    |> String.trim_trailing("Z")
-  end
+  @spec reasons() :: [reason(), ...]
+  def reasons, do: @client_report_reasons
 end

lib/sentry/client_report/sender.ex

@@ -0,0 +1,87 @@
+defmodule Sentry.ClientReport.Sender do
+  @moduledoc false
+
+  # This module is responsible for storing client reports and periodically "flushing"
+  # them to Sentry.
+
+  use GenServer
+
+  alias Sentry.{Client, ClientReport, Config, Envelope}
+
+  @send_interval 30_000
+
+  @client_report_reasons ClientReport.reasons()
+
+  @spec start_link([]) :: GenServer.on_start()
+  def start_link(opts \\ []) do
+    GenServer.start_link(__MODULE__, nil, name: Keyword.get(opts, :name, __MODULE__))
+  end
+
+  @spec record_discarded_events(atom(), [item], GenServer.server()) :: :ok
+        when item:
+               Sentry.Attachment.t()
+               | Sentry.CheckIn.t()
+               | ClientReport.t()
+               | Sentry.Event.t()
+  def record_discarded_events(reason, event_items, genserver \\ __MODULE__)
+      when is_list(event_items) do
+    # We silently ignore events whose reasons aren't valid because we have to add it to the allowlist in Snuba
+    # https://develop.sentry.dev/sdk/client-reports/
+    if Enum.member?(@client_report_reasons, reason) do
+      Enum.each(
+        event_items,
+        fn item ->
+          GenServer.cast(
+            genserver,
+            {:record_discarded_events, reason, Envelope.get_data_category(item)}
+          )
+        end
+      )
+    end
+  end
+
+  ## Callbacks
+
+  @impl true
+  def init(nil) do
+    schedule_report()
+    {:ok, _state = %{}}
+  end
+
+  @impl true
+  def handle_cast({:record_discarded_events, reason, category}, discarded_events) do
+    {:noreply, Map.update(discarded_events, {reason, category}, 1, &(&1 + 1))}
+  end
+
+  @impl true
+  def handle_info(:send_report, state) do
+    _ =
+      if map_size(state) != 0 and Config.dsn() != nil and Config.send_client_reports?() do
+        client_report =
+          %ClientReport{
+            timestamp:
+              DateTime.utc_now()
+              |> DateTime.truncate(:second)
+              |> DateTime.to_iso8601()
+              |> String.trim_trailing("Z"),
+            discarded_events:
+              Enum.map(state, fn {{reason, category}, quantity} ->
+                %{
+                  reason: reason,
+                  category: category,
+                  quantity: quantity
+                }
+              end)
+          }
+
+        Client.send_client_report(client_report)
+      end
+
+    schedule_report()
+    {:noreply, %{}}
+  end
+
+  defp schedule_report do
+    Process.send_after(self(), :send_report, @send_interval)
+  end
+end

lib/sentry/envelope.ex

@@ -46,23 +46,16 @@ defmodule Sentry.Envelope do
     }
   end
 
+  @doc """
+  Returns the "data category" of the envelope's contents (to be used in client reports and more).
+  """
+  @doc since: "10.8.0"
   @spec get_data_category(Attachment.t() | CheckIn.t() | ClientReport.t() | Event.t()) ::
           String.t()
-  def get_data_category(%mod{} = type) when mod in [Attachment, CheckIn, ClientReport, Event] do
-    case type do
-      %Attachment{} ->
-        "attachment"
-
-      %CheckIn{} ->
-        "monitor"
-
-      %ClientReport{} ->
-        "internal"
-
-      %Event{} ->
-        "error"
-    end
-  end
+  def get_data_category(%Attachment{}), do: "attachment"
+  def get_data_category(%CheckIn{}), do: "monitor"
+  def get_data_category(%ClientReport{}), do: "internal"
+  def get_data_category(%Event{}), do: "error"
 
   @doc """
   Encodes the envelope into its binary representation.

lib/sentry/transport.ex

@@ -66,7 +66,7 @@ defmodule Sentry.Transport do
         )
 
       {:retry_after, _delay_ms} ->
-        ClientReport.record_discarded_events(:ratelimit_backoff, envelope_items)
+        ClientReport.Sender.record_discarded_events(:ratelimit_backoff, envelope_items)
         {:error, ClientError.new(:too_many_retries)}
 
       {:error, _reason} when retries_left != [] ->
@@ -83,11 +83,11 @@ defmodule Sentry.Transport do
         )
 
       {:error, {:http, {status, headers, body}}} ->
-        ClientReport.record_discarded_events(:send_error, envelope_items)
+        ClientReport.Sender.record_discarded_events(:send_error, envelope_items)
         {:error, ClientError.server_error(status, headers, body)}
 
       {:error, reason} ->
-        ClientReport.record_discarded_events(:send_error, envelope_items)
+        ClientReport.Sender.record_discarded_events(:send_error, envelope_items)
         {:error, ClientError.new(reason)}
     end
   end

mix.exs

@@ -50,7 +50,7 @@ defmodule Sentry.Mixfile do
         groups_for_modules: [
           "Plug and Phoenix": [Sentry.PlugCapture, Sentry.PlugContext, Sentry.LiveViewHook],
           Loggers: [Sentry.LoggerBackend, Sentry.LoggerHandler],
-          "Data Structures": [Sentry.Attachment, Sentry.CheckIn],
+          "Data Structures": [Sentry.Attachment, Sentry.CheckIn, Sentry.ClientReport],
           HTTP: [Sentry.HTTPClient, Sentry.HackneyClient],
           Interfaces: [~r/^Sentry\.Interfaces/],
           Testing: [Sentry.Test]

test/sentry/client_report/sender_test.exs

@@ -0,0 +1,72 @@
+defmodule Sentry.ClientReportTest do
+  use Sentry.Case, async: false
+
+  import Sentry.TestHelpers
+
+  alias Sentry.ClientReport.Sender
+  alias Sentry.Event
+
+  setup do
+    original_retries =
+      Application.get_env(:sentry, :request_retries, Sentry.Transport.default_retries())
+
+    on_exit(fn -> Application.put_env(:sentry, :request_retries, original_retries) end)
+
+    Application.put_env(:sentry, :request_retries, [])
+
+    bypass = Bypass.open()
+    put_test_config(dsn: "http://public:secret@localhost:#{bypass.port}/1")
+    %{bypass: bypass}
+  end
+
+  describe "record_discarded_events/2 + flushing" do
+    test "succefully records the discarded event to the client report", %{bypass: bypass} do
+      start_supervised!({Sender, name: :test_client_report})
+
+      events = [
+        %Event{
+          event_id: Sentry.UUID.uuid4_hex(),
+          timestamp: "2024-10-12T13:21:13"
+        }
+      ]
+
+      assert :ok = Sender.record_discarded_events(:before_send, events, :test_client_report)
+
+      assert :sys.get_state(:test_client_report) == %{{:before_send, "error"} => 1}
+
+      assert :ok = Sender.record_discarded_events(:before_send, events, :test_client_report)
+
+      assert :sys.get_state(:test_client_report) == %{{:before_send, "error"} => 2}
+
+      assert :ok = Sender.record_discarded_events(:event_processor, events, :test_client_report)
+      assert :ok = Sender.record_discarded_events(:network_error, events, :test_client_report)
+
+      assert :sys.get_state(:test_client_report) == %{
+               {:before_send, "error"} => 2,
+               {:event_processor, "error"} => 1,
+               {:network_error, "error"} => 1
+             }
+
+      send(Process.whereis(:test_client_report), :send_report)
+
+      Bypass.expect(bypass, fn conn ->
+        {:ok, body, conn} = Plug.Conn.read_body(conn)
+
+        assert [{%{"type" => "client_report", "length" => _}, client_report}] =
+                 decode_envelope!(body)
+
+        assert client_report["discarded_events"] == [
+                 %{"reason" => "before_send", "category" => "error", "quantity" => 2},
+                 %{"reason" => "event_processor", "category" => "error", "quantity" => 1},
+                 %{"reason" => "network_error", "category" => "error", "quantity" => 1}
+               ]
+
+        assert client_report["timestamp"] =~ ~r/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}$/
+
+        Plug.Conn.resp(conn, 200, ~s<{"id": "340"}>)
+      end)
+
+      assert :sys.get_state(:test_client_report) == %{}
+    end
+  end
+end