Add support for client reports #801

savhappy · 2024-09-25T11:25:37Z

No description provided.

whatyouhide · 2024-09-26T09:43:31Z

@savhappy can you lmk when this will be ready for review?

savhappy · 2024-09-26T09:45:00Z

@whatyouhide yep yep I'll tag you here when ready

savhappy · 2024-10-01T18:26:06Z

@whatyouhide ready for an initial review. I've only added a single instance of recording an event but will add in more after discussion

whatyouhide

Left a first pass of comments, some are nits, some are things to address, and a couple are blocking. Let me know if you wanna go through them together 🙃

lib/sentry/application.ex

lib/sentry/config.ex

lib/sentry/event.ex

lib/sentry/envelope.ex

whatyouhide · 2024-10-05T12:17:28Z

lib/sentry/client_report.ex

+  def add_discarded_event({reason, category}) do
+    if Enum.member?(@client_report_reasons, reason) do
+      GenServer.cast(__MODULE__, {:add_discarded_event, {reason, category}})
+    end
+
+    :ok
+  end


Why did you choose to silently ignore events whose reason is not a valid one?

I decided to do this for two reasons -> From what I can tell, the Ruby SDK ignores invalid reasons

def record_lost_event(reason, data_category, num: 1) return unless @send_client_reports return unless CLIENT_REPORT_REASONS.include?(reason) @discarded_events[[reason, data_category]] += num end

and the documentation says

In case a reason needs to be added, it also has to be added to the allowlist in [snuba](https://github.com/getsentry/snuba/blob/4e7cfdddcf7b93eacb762bc74ca2461cec9464e5/snuba/datasets/outcomes_processor.py#L24-L34).

So we choose to do nothing if it's invalid unless we want to add them to this repo mentioned. Thoughts?

Okay, sounds good. Let's silently ignore it for now, BUT leave a comment with basically what you just said to me, but in the code 🙃

lib/sentry/client_report.ex

savhappy · 2024-10-06T12:52:53Z

Thanks @whatyouhide for this! Updated with these comments except the blocking tests. Will add those shortly. Hold off on more feedback till I get the next round out. I need to add data_category to this somewhere too and maybe a capture_event function to direct how this report is classified.

savhappy · 2024-10-06T21:31:40Z

Question @whatyouhide : I need to add in the appropriate data_category to the payload. We add the type only in the headers but other SDK's have a type field. The mappings for types would look like this

defmodule Sentry.DataCategory do
  @spec data_category_mappings(String.t()) :: String.t()
  def data_category_mappings(type) do
    case type do
      "session" -> "session"
      "sessions" -> "session"
      "attachment" -> "attachment"
      "transaction" -> "transaction"
      "profile" -> "profile"
      "span" -> "span"
      "check_in" -> "monitor"
      "statsd" -> "metric_bucket"
      "metric_meta" -> "metric_bucket"
      "event" -> "error"
      "client_report" -> "internal"
      _ -> "default"
    end
  end
end

But we only have the types Event, Attachment, CheckIn, ClientReport (and soon Transaction). So whats the best way to go about deriving the type that will age well as we add more in the future? Or should I just get it based on the struct that's passed and if its not one of the structs see if it fits a string that's passed as the arg for data_category instead? I hope this makes sense. If not we can go into detail on Tuesday.

whatyouhide · 2024-10-07T07:09:37Z

@savhappy can you elaborate on the issue here? Where do we need to add data_category? And what is the data category?

savhappy · 2024-10-07T12:35:03Z

Yep! @whatyouhide The data categories are used to classify the type of data being ingested (ingest side types for collecting statistics).

It's defined in the Client Report docs as -> category: The data category for which the discard reason applies. These are the same data categories used for [rate limits](https://develop.sentry.dev/sdk/rate-limiting/#definitions).

We need to add the data_category to the payload ->

 "discarded_events": [
    {
      "reason": "queue_overflow",
      "category": "transaction",
      "quantity": 1
    }

So When I send {reason, category} over to the genserver. I need to ensure that I'm sending over the correct data category to Sentry.

In Transport module ->

  def record_discarded_event(reason, category) do
    # add logic for data category here
    # call to client report genserver
    ClientReport.add_discarded_event({reason, category})
    # end

    :ok
  end

whatyouhide

Left one more round of comments, this is shaping up! 🎉

lib/sentry/client.ex

whatyouhide · 2024-10-07T20:24:40Z

lib/sentry/client_report.ex

+  @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__)


Using %__MODULE__{} as the state is confusing IMO because you're conflating two responsibilities in the same place: now %__MODULE__{} is both the data definition of the client report, as well as the GenServer's internal state.

Instead, I'll go against my own advice and say: just use the map of discarded events as the GenServer's state, and only add the timestamp when sending the client report. Makes sense?

whatyouhide · 2024-10-07T20:24:48Z

lib/sentry/client_report.ex

+  @send_interval 5000
+
+  @spec start_link([]) :: GenServer.on_start()
+  def start_link([]) do


If you choose to go with Sentry.ClientReport containing both the struct as well as the GenServer, you'll need to make all these functions private (@doc false).

Updated but double check that this is what you meant ->

@doc false @spec start_link([]) :: GenServer.on_start() def start_link([]) do # check config to see if send_client_report is true GenServer.start_link(__MODULE__, %{}, name: __MODULE__) end

whatyouhide · 2024-10-07T20:24:51Z

lib/sentry/client_report.ex

+
+  @spec start_link([]) :: GenServer.on_start()
+  def start_link([]) do
+    # check config to see if send_client_report is true


Is this a TODO?

Yep. If the user chooses to flip to false then we don't report but I'm going to add this here ->

if Config.dsn() != nil && Config.send_client_reports?() do Client.send_client_report(client_report) end

in the :send_report handle_info function

whatyouhide · 2024-10-07T20:27:18Z

lib/sentry/client_report.ex

+  @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}}}


This code is not needed if you use Map.update/4 the way you do below. If the map is empty, you'll init {reason, category} to the third arg of Map.update/4 (1, correctly). Plz remove the if 🙃

whatyouhide · 2024-10-07T20:27:37Z

lib/sentry/client_report.ex

+
+  @impl true
+  def handle_info(:send_report, state) do
+    if map_size(state.discarded_events) == 0 do


Opposite: if map is empty, then don't report?

Whoops! Yes, thx!

lib/sentry/client_report.ex

whatyouhide · 2024-10-07T20:29:49Z

lib/sentry/client_report.ex

+  end
+
+  @spec add_discarded_event({reason(), String.t()}) :: :ok
+  def add_discarded_event({reason, category}) do


Don't use a tuple here, it's not needed. Just pass two args, (reason, category) and call the GenServer cast {:add_discarded_event, reason, category}.

whatyouhide · 2024-10-07T20:31:09Z

lib/sentry/transport.ex

@@ -40,6 +40,11 @@ defmodule Sentry.Transport do
    result
  end

+  def record_discarded_event(reason, category) do
+    ClientReport.add_discarded_event({reason, DataCategory.data_category_mapping(category)})


Why go through the extra data_category_mapping/1 step here rather than just do the mapping in ClientReport, without introducing a new Sentry.DataCategory module?

I believe we resolved this in our pairing. Updates made so lemme know thoughts

whatyouhide · 2024-10-10T18:20:34Z

lib/sentry.ex

@@ -411,6 +412,7 @@ defmodule Sentry do
      |> CheckIn.new()
      |> Client.send_check_in(options)
    else
+      # add record dropped event here? Dropped due to....


Ask on Discord because I don't think we need to report these here but maybe other SDKs do. Remove this comment in the meantime so that we can start shipping this PR.

whatyouhide · 2024-10-10T18:21:51Z

lib/sentry/client.ex

        :unsampled

      :excluded ->
+        # do we need to add this if an event is dropped due to it being a duplicate??


Ask in Discord

whatyouhide · 2024-10-10T18:23:14Z

lib/sentry/envelope.ex

@@ -34,6 +34,41 @@ defmodule Sentry.Envelope do
    }
  end

+  @doc """
+  Creates a new envelope containing the client report.
+  since: "10.8.0"


@doc since is an ExDoc feature. I literally meant:

@doc since: "10.8.0"

after the @doc string. There are tons of examples in this repo 🙃

lib/sentry/envelope.ex

whatyouhide · 2024-10-10T18:25:23Z

lib/sentry/envelope.ex

+    if is_binary(type) do
+      type


When will this ever be a string? We talked about functions having a small "type" surface but this function is still accepting binary | Attachment.t | .... Can we make it so this only accepts a struct OR a binary?

Also, can you add typespecs and tests for this?

whatyouhide · 2024-10-10T18:25:59Z

lib/sentry/envelope.ex

+        _ ->
+          "default"


This is dangerous because I can call get_data_category(:completely_banana_value). Make sure the input is either a string or a struct (per comm
ent above)

whatyouhide · 2024-10-10T18:27:19Z

lib/sentry/transport.ex

+    _ =
+      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


List.wrap/1

Enum.map/2 wastes memory here, use Enum.each/2 since we ignore the result.

Suggested change

_ =

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

_ =

event_item

|> List.wrap()

|> Enum.each(&ClientReport.add_discarded_event({reason, Envelope.get_data_category(&1)}))

whatyouhide

There are no Sentry.Transport tests for the new functionality

whatyouhide · 2024-10-13T11:41:38Z

lib/sentry/client_report.ex

+  @spec add_discarded_event(reason(), String.t()) :: :ok
+  def add_discarded_event(reason, category) do


This is internal for now so @doc false

whatyouhide · 2024-10-13T11:42:39Z

lib/sentry/envelope.ex

+
+  @spec get_data_category(Attachment.t() | CheckIn.t() | ClientReport.t() | Event.t()) ::
+          String.t()
+  def get_data_category(type) do


If you wanna raise, do it like this so that invalid input doesn't even get in the function:

Suggested change

def get_data_category(type) do

def get_data_category(%mod{} = type) when mod in [Attachment, CheckIn, ClientReport, Event] do

whatyouhide

We're ready for docs for this and to

whatyouhide · 2024-10-16T06:43:25Z

test/envelope_test.exs

+
+    client_report = %ClientReport{
+      timestamp: "2024-10-12T13:21:13",
+      discarded_events: [%{reason: :event_processor, category: "error"}]


Doesn't this need a quantity as well?

lib/sentry/client_report.ex

savhappy · 2024-10-21T08:24:08Z

@whatyouhide should we create a ticket that reminds us to add in spans to the client report when supported?

savhappy force-pushed the sav/client-error-reports branch 2 times, most recently from f7abce9 to eaf55a1 Compare September 30, 2024 16:07

savhappy marked this pull request as ready for review October 1, 2024 18:25

whatyouhide requested changes Oct 5, 2024

View reviewed changes

whatyouhide reviewed Oct 7, 2024

View reviewed changes

savhappy force-pushed the sav/client-error-reports branch from e98dc43 to 2503547 Compare October 8, 2024 18:33

savhappy requested a review from whatyouhide October 8, 2024 18:35

whatyouhide reviewed Oct 10, 2024

View reviewed changes

savhappy requested a review from whatyouhide October 12, 2024 21:03

whatyouhide reviewed Oct 13, 2024

View reviewed changes

savhappy requested a review from whatyouhide October 15, 2024 20:08

savhappy added 12 commits October 15, 2024 22:11

add initial structs for client report

9d6bdd4

add genserver

91528a0

refactor and add to envelope

6bf15ac

conditionally add client_reports

2381f1c

update test

e67bd09

fix dialyzer

a97df7f

async true

a852e86

initial feddback #1

0240096

add data category

5564099

refactor data category and add in more recordings

c84b3b9

feedback round 2 - update genserver and clean up

02975c0

third round of feedback

d639c0b

updated test + third round of feedback

fa43765

savhappy force-pushed the sav/client-error-reports branch from 88c78a6 to fa43765 Compare October 15, 2024 20:11

whatyouhide reviewed Oct 16, 2024

View reviewed changes

docs added

5c44239

savhappy requested a review from whatyouhide October 17, 2024 20:20

whatyouhide approved these changes Oct 21, 2024

View reviewed changes

lib/sentry/client_report.ex Outdated Show resolved Hide resolved

Update lib/sentry/client_report.ex

507745a

whatyouhide mentioned this pull request Oct 21, 2024

Support client reports #783

Closed

whatyouhide changed the title ~~[WIP] Add client error reports~~ Add support for client reports Oct 21, 2024

whatyouhide merged commit 549d5be into master Oct 21, 2024
3 of 4 checks passed

whatyouhide deleted the sav/client-error-reports branch October 21, 2024 08:27

		@spec add_discarded_event(reason(), String.t()) :: :ok
		def add_discarded_event(reason, category) do

	def get_data_category(type) do
	def get_data_category(%mod{} = type) when mod in [Attachment, CheckIn, ClientReport, Event] do

Add support for client reports #801

Add support for client reports #801

Conversation

savhappy commented Sep 25, 2024

whatyouhide commented Sep 26, 2024

savhappy commented Sep 26, 2024

savhappy commented Oct 1, 2024

whatyouhide left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savhappy commented Oct 6, 2024

savhappy commented Oct 6, 2024 • edited Loading

whatyouhide commented Oct 7, 2024

savhappy commented Oct 7, 2024 • edited Loading

whatyouhide left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savhappy Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whatyouhide left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whatyouhide left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savhappy commented Oct 21, 2024

savhappy commented Oct 6, 2024 •

edited

Loading

savhappy commented Oct 7, 2024 •

edited

Loading

savhappy Oct 8, 2024 •

edited

Loading