Always return Sentry.ClientError when possible (#795)

Author: whatyouhide

lib/sentry.ex

@@ -404,7 +404,7 @@ defmodule Sentry do
   """
   @doc since: "10.2.0"
   @spec capture_check_in(keyword()) ::
-          {:ok, check_in_id :: String.t()} | :ignored | {:error, term()}
+          {:ok, check_in_id :: String.t()} | :ignored | {:error, ClientError.t()}
   def capture_check_in(options) when is_list(options) do
     if Config.dsn() do
       options

lib/sentry/client.ex

@@ -24,7 +24,7 @@ defmodule Sentry.Client do
   @max_message_length 8_192
 
   @spec send_check_in(CheckIn.t(), keyword()) ::
-          {:ok, check_in_id :: String.t()} | {:error, term()}
+          {:ok, check_in_id :: String.t()} | {:error, ClientError.t()}
   def send_check_in(%CheckIn{} = check_in, opts) when is_list(opts) do
     client = Keyword.get_lazy(opts, :client, &Config.client/0)
 
@@ -37,7 +37,7 @@ defmodule Sentry.Client do
     send_result =
       check_in
       |> Envelope.from_check_in()
-      |> Transport.post_envelope(client, request_retries)
+      |> Transport.encode_and_post_envelope(client, request_retries)
 
     _ = maybe_log_send_result(send_result, check_in)
     send_result
@@ -87,11 +87,8 @@ defmodule Sentry.Client do
       :excluded ->
         :excluded
 
-      {:error, {status, headers, body}} ->
-        {:error, ClientError.server_error(status, headers, body)}
-
-      {:error, reason} ->
-        {:error, ClientError.new(reason)}
+      {:error, %ClientError{} = error} ->
+        {:error, error}
     end
   end
 
@@ -176,7 +173,7 @@ defmodule Sentry.Client do
         send_result =
           event
           |> Envelope.from_event()
-          |> Transport.post_envelope(client, request_retries)
+          |> Transport.encode_and_post_envelope(client, request_retries)
 
         _ = maybe_log_send_result(send_result, event)
         send_result
@@ -315,24 +312,8 @@ defmodule Sentry.Client do
     else
       message =
         case send_result do
-          {:error, {:invalid_json, error}} ->
-            "Failed to send Sentry event. Unable to encode JSON Sentry error - #{inspect(error)}"
-
-          {:error, {:request_failure, last_error}} ->
-            case last_error do
-              {kind, data, stacktrace}
-              when kind in [:exit, :throw, :error] and is_list(stacktrace) ->
-                Exception.format(kind, data, stacktrace)
-
-              _other ->
-                "Failed to send Sentry event. Error in HTTP Request to Sentry - #{inspect(last_error)}"
-            end
-
-          {:error, {status, headers, body}} ->
-            ClientError.server_error(status, headers, body) |> ClientError.message()
-
-          {:error, reason} ->
-            ClientError.new(reason) |> ClientError.message()
+          {:error, %ClientError{} = error} ->
+            "Failed to send Sentry event. #{Exception.message(error)}"
 
           {:ok, _} ->
             nil

lib/sentry/client_error.ex

@@ -70,11 +70,18 @@ defmodule Sentry.ClientError do
   end
 
   defp format(:too_many_retries) do
-    "Sentry responded with status 429 - Too Many Requests"
+    "Sentry responded with status 429 - Too Many Requests and the SDK exhausted the configured retries"
   end
 
   defp format({:invalid_json, reason}) do
-    "the Sentry SDK could not encode the event to JSON: #{Exception.message(reason)}"
+    formatted =
+      if is_exception(reason) do
+        Exception.message(reason)
+      else
+        inspect(reason)
+      end
+
+    "the Sentry SDK could not encode the event to JSON: #{formatted}"
   end
 
   defp format({:request_failure, reason}) do

lib/sentry/transport.ex

@@ -1,8 +1,9 @@
 defmodule Sentry.Transport do
   @moduledoc false
 
-  # This module is exclusively responsible for POSTing envelopes to Sentry.
+  # This module is exclusively responsible for encoding and POSTing envelopes to Sentry.
 
+  alias Sentry.ClientError
   alias Sentry.Config
   alias Sentry.Envelope
 
@@ -15,18 +16,24 @@ defmodule Sentry.Transport do
     @default_retries
   end
 
-  # The "retries" parameter is there for better testing.
-  @spec post_envelope(Envelope.t(), module(), [non_neg_integer()]) ::
-          {:ok, envelope_id :: String.t()} | {:error, term()}
-  def post_envelope(%Envelope{} = envelope, client, retries \\ @default_retries)
+  @doc """
+  Encodes the given envelope and POSTs it to Sentry.
+
+
+  The `retries` parameter is there for better testing. This function also logs
+  a warning if there is an error encoding or posting the envelope.
+  """
+  @spec encode_and_post_envelope(Envelope.t(), module(), [non_neg_integer()]) ::
+          {:ok, envelope_id :: String.t()} | {:error, ClientError.t()}
+  def encode_and_post_envelope(%Envelope{} = envelope, client, retries \\ @default_retries)
       when is_atom(client) and is_list(retries) do
     case Envelope.to_binary(envelope) do
       {:ok, body} ->
         {endpoint, headers} = get_endpoint_and_headers()
         post_envelope_with_retries(client, endpoint, headers, body, retries)
 
       {:error, reason} ->
-        {:error, {:invalid_json, reason}}
+        {:error, ClientError.new({:invalid_json, reason})}
     end
   end
 
@@ -42,18 +49,18 @@ defmodule Sentry.Transport do
         post_envelope_with_retries(client, endpoint, headers, payload, tl(retries_left))
 
       {:retry_after, _delay_ms} ->
-        {:error, :too_many_retries}
+        {:error, ClientError.new(:too_many_retries)}
 
       {:error, _reason} when retries_left != [] ->
         [sleep_interval | retries_left] = retries_left
         Process.sleep(sleep_interval)
         post_envelope_with_retries(client, endpoint, headers, payload, retries_left)
 
-      {:error, {status, headers, body}} ->
-        {:error, {status, headers, body}}
+      {:error, {:http, {status, headers, body}}} ->
+        {:error, ClientError.server_error(status, headers, body)}
 
       {:error, reason} ->
-        {:error, {:request_failure, reason}}
+        {:error, ClientError.new(reason)}
     end
   end
 
@@ -78,10 +85,10 @@ defmodule Sentry.Transport do
         {:retry_after, delay_ms}
 
       {:ok, status, headers, body} ->
-        {:error, {status, headers, body}}
+        {:error, {:http, {status, headers, body}}}
 
       {:error, reason} ->
-        {:error, reason}
+        {:error, {:request_failure, reason}}
     end
   catch
     kind, data -> {:error, {kind, data, __STACKTRACE__}}

lib/sentry/transport/sender.ex

@@ -43,7 +43,7 @@ defmodule Sentry.Transport.Sender do
   def handle_cast({:send, client, %Event{} = event}, %__MODULE__{} = state) do
     event
     |> Envelope.from_event()
-    |> Transport.post_envelope(client)
+    |> Transport.encode_and_post_envelope(client)
     |> Client.maybe_log_send_result([event])
 
     # We sent an event, so we can decrease the number of queued events.

test/sentry/client_error_test.exs

@@ -5,7 +5,7 @@ defmodule Sentry.ClientErrorTest do
   describe "c:Exception.message/1" do
     test "with an atom reason" do
       assert message_for_reason(:too_many_retries) ==
-               "Sentry responded with status 429 - Too Many Requests"
+               "Sentry responded with status 429 - Too Many Requests and the SDK exhausted the configured retries"
     end
 
     test "with {:invalid_json, _} reason" do

test/sentry/client_test.exs

@@ -342,7 +342,7 @@ defmodule Sentry.ClientTest do
 
       assert capture_log(fn ->
                Client.send_event(event, result: :sync)
-             end) =~ "Failed to send Sentry event. Unable to encode JSON"
+             end) =~ "the Sentry SDK could not encode the event to JSON: :im_just_bad"
     end
 
     test "uses the async sender pool when :result is :none", %{bypass: bypass} do

test/sentry/transport_test.exs

@@ -3,9 +3,9 @@ defmodule Sentry.TransportTest do
 
   import Sentry.TestHelpers
 
-  alias Sentry.{Envelope, Event, HackneyClient, Transport}
+  alias Sentry.{ClientError, Envelope, Event, HackneyClient, Transport}
 
-  describe "post_envelope/2" do
+  describe "encode_and_post_envelope/2" do
     setup do
       bypass = Bypass.open()
       put_test_config(dsn: "http://public:secret@localhost:#{bypass.port}/1")
@@ -33,31 +33,46 @@ defmodule Sentry.TransportTest do
         Plug.Conn.resp(conn, 200, ~s<{"id":"123"}>)
       end)
 
-      assert {:ok, "123"} = Transport.post_envelope(envelope, HackneyClient)
+      assert {:ok, "123"} = Transport.encode_and_post_envelope(envelope, HackneyClient)
     end
 
-    defmodule InvalidHTTPClient do
-      def post(_endpoint, _headers, _body) do
-        {:ok, "not an integer", :badarg, %{}}
+    test "returns an error if the HTTP client returns a badly-typed response" do
+      defmodule InvalidHTTPClient do
+        def post(_endpoint, _headers, _body) do
+          Process.get(:invalid_http_return_value) ||
+            raise "missing :invalid_http_return_value from pdict"
+        end
       end
-    end
 
-    test "returns an error with an invalid response from a POST request" do
       envelope = Envelope.from_event(Event.create_event(message: "Hello 1"))
 
-      assert {:error,
-              {:request_failure,
-               {:malformed_http_client_response, "not an integer", :badarg, %{}}}} =
-               Transport.post_envelope(envelope, InvalidHTTPClient, _retries = [])
+      for {:ok, status, headers, body} = invalid_return_value <- [
+            {:ok, 10000, [], ""},
+            {:ok, 200, %{}, ""},
+            {:ok, 200, [], :not_a_binary}
+          ] do
+        Process.put(:invalid_http_return_value, invalid_return_value)
+
+        assert {:request_failure, {:malformed_http_client_response, ^status, ^headers, ^body}} =
+                 error(fn ->
+                   Transport.encode_and_post_envelope(envelope, InvalidHTTPClient, _retries = [])
+                 end)
+      end
+    after
+      Process.delete(:invalid_http_return_value)
+      :code.delete(InvalidHTTPClient)
+      :code.purge(InvalidHTTPClient)
     end
 
     test "returns the HTTP client's error if the HTTP client returns one", %{bypass: bypass} do
       envelope = Envelope.from_event(Event.create_event(message: "Hello"))
 
       Bypass.down(bypass)
 
-      assert {:error, {:request_failure, :econnrefused}} =
-               Transport.post_envelope(envelope, HackneyClient, _retries = [])
+      assert {:request_failure, :econnrefused} =
+               error(fn ->
+                 Transport.encode_and_post_envelope(envelope, HackneyClient, _retries = [])
+               end)
     end
 
     test "returns an error if the response from Sentry is not 200", %{bypass: bypass} do
@@ -69,10 +84,15 @@ defmodule Sentry.TransportTest do
         |> Plug.Conn.resp(400, ~s<{}>)
       end)
 
-      {:error, {_status, headers, _body}} =
-        Transport.post_envelope(envelope, HackneyClient, _retries = [])
+      {:error, %ClientError{} = error} =
+        Transport.encode_and_post_envelope(envelope, HackneyClient, _retries = [])
 
+      assert error.reason == :server_error
+      assert {400, headers, "{}"} = error.http_response
       assert :proplists.get_value("x-sentry-error", headers, nil) == "some error"
+
+      assert Exception.message(error) =~
+               "the Sentry server responded with an error, the details are below."
     end
 
     test "returns an error if the HTTP client raises an error when making the request",
@@ -85,10 +105,10 @@ defmodule Sentry.TransportTest do
         end
       end
 
-      assert {:error, {:error, %RuntimeError{} = exception, _stacktrace}} =
-               Transport.post_envelope(envelope, RaisingHTTPClient, _retries = [])
-
-      assert exception.message == "I'm a really bad HTTP client"
+      assert {:error, %RuntimeError{message: "I'm a really bad HTTP client"}, _stacktrace} =
+               error(fn ->
+                 Transport.encode_and_post_envelope(envelope, RaisingHTTPClient, _retries = [])
+               end)
     after
       :code.delete(RaisingHTTPClient)
       :code.purge(RaisingHTTPClient)
@@ -104,8 +124,10 @@ defmodule Sentry.TransportTest do
         end
       end
 
-      assert {:error, {:exit, :through_the_window, _stacktrace}} =
-               Transport.post_envelope(envelope, ExitingHTTPClient, _retries = [])
+      assert {:exit, :through_the_window, _stacktrace} =
+               error(fn ->
+                 Transport.encode_and_post_envelope(envelope, ExitingHTTPClient, _retries = [])
+               end)
     after
       :code.delete(ExitingHTTPClient)
       :code.purge(ExitingHTTPClient)
@@ -121,8 +143,10 @@ defmodule Sentry.TransportTest do
         end
       end
 
-      assert {:error, {:throw, :catch_me_if_you_can, _stacktrace}} =
-               Transport.post_envelope(envelope, ThrowingHTTPClient, _retries = [])
+      assert {:throw, :catch_me_if_you_can, _stacktrace} =
+               error(fn ->
+                 Transport.encode_and_post_envelope(envelope, ThrowingHTTPClient, _retries = [])
+               end)
     after
       :code.delete(ThrowingHTTPClient)
       :code.purge(ThrowingHTTPClient)
@@ -145,10 +169,10 @@ defmodule Sentry.TransportTest do
 
       put_test_config(json_library: CrashingJSONLibrary)
 
-      assert {:error, {:error, %RuntimeError{} = exception, _stacktrace}} =
-               Transport.post_envelope(envelope, HackneyClient, _retries = [])
-
-      assert exception.message == "I'm a really bad JSON library"
+      assert {:error, %RuntimeError{message: "I'm a really bad JSON library"}, _stacktrace} =
+               error(fn ->
+                 Transport.encode_and_post_envelope(envelope, HackneyClient, _retries = [])
+               end)
     after
       :code.delete(CrashingJSONLibrary)
       :code.purge(CrashingJSONLibrary)
@@ -165,8 +189,10 @@ defmodule Sentry.TransportTest do
         Plug.Conn.resp(conn, 200, ~s<invalid JSON>)
       end)
 
-      assert {:error, {:request_failure, %Jason.DecodeError{}}} =
-               Transport.post_envelope(envelope, HackneyClient, _retries = [0])
+      assert {:request_failure, %Jason.DecodeError{}} =
+               error(fn ->
+                 Transport.encode_and_post_envelope(envelope, HackneyClient, _retries = [0])
+               end)
 
       assert_received {:request, ^ref}
       assert_received {:request, ^ref}
@@ -191,7 +217,8 @@ defmodule Sentry.TransportTest do
         end
       end)
 
-      assert {:ok, "123"} = Transport.post_envelope(envelope, HackneyClient, _retries = [10, 25])
+      assert {:ok, "123"} =
+               Transport.encode_and_post_envelope(envelope, HackneyClient, _retries = [10, 25])
 
       assert System.system_time(:millisecond) - start_time >= 35
 
@@ -213,10 +240,18 @@ defmodule Sentry.TransportTest do
         |> Plug.Conn.resp(429, ~s<{}>)
       end)
 
-      assert {:error, :too_many_retries} =
-               Transport.post_envelope(envelope, HackneyClient, _retries = [])
+      assert :too_many_retries =
+               error(fn ->
+                 Transport.encode_and_post_envelope(envelope, HackneyClient, _retries = [])
+               end)
 
       assert_received {:request, ^ref}
     end
   end
+
+  defp error(fun) do
+    assert {:error, %ClientError{} = error} = fun.()
+    assert is_binary(Exception.message(error))
+    error.reason
+  end
 end