updated test + third round of feedback

Author: savhappy

lib/sentry.ex

@@ -179,7 +179,7 @@ defmodule Sentry do
   > with `:source_code_exclude_patterns`.
   """
 
-  alias Sentry.{CheckIn, Client, ClientError, Config, Event, LoggerUtils, Options, Transport}
+  alias Sentry.{CheckIn, Client, ClientError, ClientReport, Config, Event, LoggerUtils, Options}
 
   require Logger
 
@@ -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)}")
-        Transport.record_discarded_event(:event_processor, event)
+        ClientReport.record_discarded_events(:event_processor, [event])
         :ignored
 
       # If we're in test mode, let's send the event down the pipeline anyway.

lib/sentry/client.ex

@@ -8,15 +8,15 @@ defmodule Sentry.Client do
   alias Sentry.{
     CheckIn,
     ClientError,
+    ClientReport,
     Config,
     Dedupe,
     Envelope,
     Event,
     Interfaces,
     LoggerUtils,
     Transport,
-    Options,
-    ClientReport
+    Options
   }
 
   require Logger
@@ -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)
-        Transport.record_discarded_event(:sample_rate, event)
+        ClientReport.record_discarded_events(:sample_rate, [event])
         :unsampled
 
       :excluded ->

lib/sentry/client_report.ex

@@ -8,7 +8,7 @@ defmodule Sentry.ClientReport do
   @moduledoc since: "10.8.0"
 
   use GenServer
-  alias Sentry.{Client, Config}
+  alias Sentry.{Client, Config, Envelope}
 
   @client_report_reasons [
     :ratelimit_backoff,
@@ -19,7 +19,9 @@ defmodule Sentry.ClientReport do
     :before_send,
     :event_processor,
     :insufficient_data,
-    :backpressure
+    :backpressure,
+    :send_error,
+    :internal_sdk_error
   ]
 
   @typedoc """
@@ -42,16 +44,36 @@ defmodule Sentry.ClientReport do
 
   @doc false
   @spec start_link([]) :: GenServer.on_start()
-  def start_link([]) do
-    GenServer.start_link(__MODULE__, %{}, name: __MODULE__)
+  def start_link(opts \\ []) do
+    GenServer.start_link(__MODULE__, %{}, name: Keyword.get(opts, :name, __MODULE__))
   end
 
-  @spec add_discarded_event(reason(), String.t()) :: :ok
-  def add_discarded_event(reason, category) do
+  @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
-      GenServer.cast(__MODULE__, {:add_discarded_event, reason, category})
+      _ =
+        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
 
@@ -64,7 +86,7 @@ defmodule Sentry.ClientReport do
 
   @doc false
   @impl true
-  def handle_cast({:add_discarded_event, reason, category}, discarded_events) do
+  def handle_cast({:record_discarded_events, reason, category}, discarded_events) do
     {:noreply, Map.update(discarded_events, {reason, category}, 1, &(&1 + 1))}
   end
 

lib/sentry/envelope.ex

@@ -48,7 +48,7 @@ defmodule Sentry.Envelope do
 
   @spec get_data_category(Attachment.t() | CheckIn.t() | ClientReport.t() | Event.t()) ::
           String.t()
-  def get_data_category(type) do
+  def get_data_category(%mod{} = type) when mod in [Attachment, CheckIn, ClientReport, Event] do
     case type do
       %Attachment{} ->
         "attachment"
@@ -61,12 +61,6 @@ defmodule Sentry.Envelope do
 
       %Event{} ->
         "error"
-
-      _ ->
-        raise ArgumentError, """
-        data category only accepts defined structs but was passed the invalid:
-        #{inspect(type)}\
-        """
     end
   end
 

lib/sentry/transport.ex

@@ -39,17 +39,6 @@ defmodule Sentry.Transport do
     result
   end
 
-  def record_discarded_event(reason, event_item) do
-    _ =
-      event_item
-      |> List.wrap()
-      |> Enum.each(&ClientReport.add_discarded_event(reason, Envelope.get_data_category(&1)))
-
-    # 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
-
   defp post_envelope_with_retries(
          client,
          endpoint,
@@ -77,7 +66,7 @@ defmodule Sentry.Transport do
         )
 
       {:retry_after, _delay_ms} ->
-        record_discarded_event(:ratelimit_backoff, envelope_items)
+        ClientReport.record_discarded_events(:ratelimit_backoff, envelope_items)
         {:error, ClientError.new(:too_many_retries)}
 
       {:error, _reason} when retries_left != [] ->
@@ -94,11 +83,11 @@ defmodule Sentry.Transport do
         )
 
       {:error, {:http, {status, headers, body}}} ->
-        record_discarded_event(:send_error, envelope_items)
+        ClientReport.record_discarded_events(:send_error, envelope_items)
         {:error, ClientError.server_error(status, headers, body)}
 
       {:error, reason} ->
-        record_discarded_event(:send_error, envelope_items)
+        ClientReport.record_discarded_events(:send_error, envelope_items)
         {:error, ClientError.new(reason)}
     end
   end

test/envelope_test.exs

@@ -145,8 +145,7 @@ defmodule Sentry.EnvelopeTest do
              timestamp: "2024-10-12T13:21:13"
            }) == "error"
 
-    assert_raise ArgumentError,
-                 ~r/data category only accepts defined structs but was passed the invalid:\n:completely_banana_value/,
+    assert_raise FunctionClauseError,
                  fn -> Envelope.get_data_category(:completely_banana_value) end
   end
 end

test/sentry/client_report_test.exs

@@ -1,23 +1,34 @@
 defmodule Sentry.ClientReportTest do
-  use Sentry.Case, async: true
+  use Sentry.Case, async: false
 
-  alias Sentry.ClientReport
+  alias Sentry.{ClientReport, Event}
 
-  describe "add_discarded_event/1" do
-    test "records discarded event to state" do
-      assert :sys.get_state(ClientReport) == %{}
+  describe "record_discarded_events/2" do
+    test "succefully records the discarded event to the client report" do
+      {:ok, _clientreport} = start_supervised({ClientReport, [name: :test_client_report]})
 
-      ClientReport.add_discarded_event(:event_processor, "error")
+      events = [
+        %Event{
+          event_id: Sentry.UUID.uuid4_hex(),
+          timestamp: "2024-10-12T13:21:13"
+        }
+      ]
 
-      assert :sys.get_state(ClientReport) == %{{:event_processor, "error"} => 1}
+      assert ClientReport.record_discarded_events(:before_send, events, :test_client_report) ==
+               :ok
 
-      ClientReport.add_discarded_event(:event_processor, "error")
-      ClientReport.add_discarded_event(:event_processor, "error")
-      ClientReport.add_discarded_event(:network_error, "error")
+      assert :sys.get_state(:test_client_report) == %{{:before_send, "error"} => 1}
 
-      # updates quantity when duplcate events are sent
-      assert :sys.get_state(ClientReport) == %{
-               {:event_processor, "error"} => 3,
+      ClientReport.record_discarded_events(:before_send, events, :test_client_report)
+
+      assert :sys.get_state(:test_client_report) == %{{:before_send, "error"} => 2}
+
+      ClientReport.record_discarded_events(:event_processor, events, :test_client_report)
+      ClientReport.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
              }
     end

test/sentry/client_test.exs

@@ -418,4 +418,34 @@ defmodule Sentry.ClientTest do
       end
     end
   end
+
+  describe "send_client_report/1" do
+    test "succefully sends discarded events to Sentry" do
+      bypass = Bypass.open()
+      put_test_config(dsn: "http://public:secret@localhost:#{bypass.port}/1")
+
+      client_report =
+        %Sentry.ClientReport{
+          timestamp: "2024-10-15T14:22:49",
+          discarded_events: [
+            %{reason: :event_processor, category: "error", quantity: 1}
+          ]
+        }
+
+      Bypass.expect_once(bypass, fn conn ->
+        {:ok, body, conn} = Plug.Conn.read_body(conn)
+        assert [{_headers, client_report_body}] = decode_envelope!(body)
+
+        assert client_report_body["discarded_events"] == [
+                 %{"category" => "error", "quantity" => 1, "reason" => "event_processor"}
+               ]
+
+        assert client_report_body["timestamp"] == "2024-10-15T14:22:49"
+
+        Plug.Conn.resp(conn, 200, ~s<{"id": "340"}>)
+      end)
+
+      assert {:ok, "340"} = Client.send_client_report(client_report)
+    end
+  end
 end