feedback round 2 - update genserver and clean up

Author: savhappy

lib/sentry/client.ex

@@ -99,19 +99,15 @@ defmodule Sentry.Client do
   @spec send_client_report(ClientReport.t()) ::
           {:ok, client_report_id :: String.t()} | {:error, ClientError.t()}
   def send_client_report(%ClientReport{} = client_report) do
-    # We don't pass options because this is internal. But should we use default client or users?
     client = Config.client()
 
     # This is a "private" option, only really used in testing.
     request_retries =
       Application.get_env(:sentry, :request_retries, Transport.default_retries())
 
-    send_result =
-      client_report
-      |> Envelope.from_client_report()
-      |> Transport.encode_and_post_envelope(client, request_retries)
-
-    send_result
+    client_report
+    |> Envelope.from_client_report()
+    |> Transport.encode_and_post_envelope(client, request_retries)
   end
 
   defp sample_event(sample_rate) do

lib/sentry/client_report.ex

@@ -29,7 +29,7 @@ defmodule Sentry.ClientReport do
           unquote(Enum.reduce(@client_report_reasons, &quote(do: unquote(&1) | unquote(&2))))
 
   @typedoc """
-  The struct for a **client report** interface.
+  The struct for a **client report**.
   """
   @type t() :: %__MODULE__{
           timestamp: String.t() | number(),
@@ -43,7 +43,7 @@ defmodule Sentry.ClientReport do
   @spec start_link([]) :: GenServer.on_start()
   def start_link([]) do
     # check config to see if send_client_report is true
-    GenServer.start_link(__MODULE__, %__MODULE__{}, name: __MODULE__)
+    GenServer.start_link(__MODULE__, %{}, name: __MODULE__)
   end
 
   @spec add_discarded_event({reason(), String.t()}) :: :ok
@@ -62,37 +62,36 @@ defmodule Sentry.ClientReport do
   end
 
   @impl true
-  def handle_cast({:add_discarded_event, {reason, category}}, client_report) do
-    if map_size(client_report.discarded_events) == 0 do
-      {:noreply, %{client_report | discarded_events: %{{reason, category} => 1}}}
+  def handle_cast({:add_discarded_event, {reason, category}}, discarded_events) do
+    if map_size(discarded_events) == 0 do
+      {:noreply, %{{reason, category} => 1}}
     else
       discarded_events =
-        Map.update(client_report.discarded_events, {reason, category}, 1, &(&1 + 1))
+        Map.update(discarded_events, {reason, category}, 1, &(&1 + 1))
 
-      {:noreply, %__MODULE__{client_report | discarded_events: discarded_events}}
+      {:noreply, discarded_events}
     end
   end
 
   @impl true
-  def handle_info(:send_report, state) do
-    if map_size(state.discarded_events) == 0 do
-      updated_state = %{
-        state
-        | timestamp: timestamp(),
-          discarded_events: transform_map(state.discarded_events)
+  def handle_info(:send_report, discarded_events) do
+    if map_size(discarded_events) != 0 do
+      client_report = %__MODULE__{
+        timestamp: timestamp(),
+        discarded_events: transform_map(discarded_events)
       }
 
       _ =
         if !Config.dsn() do
-          Client.send_client_report(updated_state)
+          Client.send_client_report(client_report)
         end
 
       schedule_report()
-      {:noreply, %__MODULE__{}}
+      {:noreply, %{}}
     else
       # state is nil so nothing to send but keep looping
       schedule_report()
-      {:noreply, %__MODULE__{}}
+      {:noreply, %{}}
     end
   end
 

lib/sentry/envelope.ex

@@ -46,22 +46,26 @@ defmodule Sentry.Envelope do
     }
   end
 
-  def get_data_category(envelope_item) do
-    case envelope_item do
-      %Attachment{} ->
-        "attachment"
+  def get_data_category(type) do
+    if is_binary(type) do
+      type
+    else
+      case type do
+        %Attachment{} ->
+          "attachment"
 
-      %CheckIn{} ->
-        "monitor"
+        %CheckIn{} ->
+          "monitor"
 
-      %ClientReport{} ->
-        "internal"
+        %ClientReport{} ->
+          "internal"
 
-      %Event{} ->
-        "error"
+        %Event{} ->
+          "error"
 
-      _ ->
-        "default"
+        _ ->
+          "default"
+      end
     end
   end
 

lib/sentry/transport.ex

@@ -3,7 +3,7 @@ defmodule Sentry.Transport do
 
   # This module is exclusively responsible for encoding and POSTing envelopes to Sentry.
 
-  alias Sentry.{ClientError, ClientReport, Config, DataCategory, Envelope, LoggerUtils}
+  alias Sentry.{ClientError, ClientReport, Config, Envelope, LoggerUtils}
 
   @default_retries [1000, 2000, 4000, 8000]
   @sentry_version 5
@@ -41,14 +41,17 @@ defmodule Sentry.Transport do
   end
 
   def record_discarded_event(reason, event_item) do
-    if is_list(event_item) do
-      Enum.map(event_item, fn event ->
-        ClientReport.add_discarded_event({reason, Envelope.get_data_category(event)})
-      end)
-    else
-      ClientReport.add_discarded_event({reason, Envelope.get_data_category(event_item)})
-    end
+    _ =
+      if is_list(event_item) do
+        Enum.map(event_item, fn event ->
+          ClientReport.add_discarded_event({reason, Envelope.get_data_category(event)})
+        end)
+      else
+        ClientReport.add_discarded_event({reason, Envelope.get_data_category(event_item)})
+      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
 

test/sentry/client_report_test.exs

@@ -5,26 +5,20 @@ defmodule Sentry.ClientReportTest do
 
   describe "add_discarded_event/1" do
     test "records discarded event to state" do
-      assert :sys.get_state(ClientReport) == %Sentry.ClientReport{
-               discarded_events: %{}
-             }
+      assert :sys.get_state(ClientReport) == %{}
 
       ClientReport.add_discarded_event({:event_processor, "error"})
 
-      assert :sys.get_state(ClientReport) == %Sentry.ClientReport{
-               discarded_events: %{{:event_processor, "error"} => 1}
-             }
+      assert :sys.get_state(ClientReport) == %{{:event_processor, "error"} => 1}
 
       ClientReport.add_discarded_event({:event_processor, "error"})
       ClientReport.add_discarded_event({:event_processor, "error"})
       ClientReport.add_discarded_event({:network_error, "error"})
 
       # updates quantity when duplcate events are sent
-      assert :sys.get_state(ClientReport) == %Sentry.ClientReport{
-               discarded_events: %{
-                 {:event_processor, "error"} => 3,
-                 {:network_error, "error"} => 1
-               }
+      assert :sys.get_state(ClientReport) == %{
+               {:event_processor, "error"} => 3,
+               {:network_error, "error"} => 1
              }
     end
   end