Merge pull request #214 from getsentry/mfa-fix

Fix Culprit ambiguity

Author: mitchellhenke

lib/sentry/event.ex

@@ -99,7 +99,7 @@ defmodule Sentry.Event do
       server_name: server_name,
       exception: exception,
       stacktrace: %{
-        frames: stacktrace_to_frames(stacktrace)
+        frames: stacktrace_to_frames(stacktrace),
       },
       release: release,
       extra: extra,
@@ -170,8 +170,9 @@ defmodule Sentry.Event do
     in_app_module_whitelist = Config.in_app_module_whitelist()
     stacktrace
     |> Enum.map(fn(line) ->
-        {mod, function, arity, location} = line
-        arity = arity_to_integer(arity)
+        {mod, function, arity_or_args, location} = line
+        f_args = args_from_stacktrace([line])
+        arity = arity_to_integer(arity_or_args)
         file = Keyword.get(location, :file)
         file = if(file, do: String.Chars.to_string(file), else: file)
         line_number = Keyword.get(location, :line)
@@ -182,6 +183,7 @@ defmodule Sentry.Event do
           module: mod,
           lineno: line_number,
           in_app: is_in_app?(mod, in_app_module_whitelist),
+          vars: f_args,
         }
         |> put_source_context(file, line_number)
       end)
@@ -202,7 +204,24 @@ defmodule Sentry.Event do
 
   @spec culprit_from_stacktrace(Exception.stacktrace) :: String.t | nil
   def culprit_from_stacktrace([]), do: nil
-  def culprit_from_stacktrace([{m, f, a, _} | _]), do: Exception.format_mfa(m, f, a)
+  def culprit_from_stacktrace([{m, f, a, _} | _]) do
+    Exception.format_mfa(m, f, arity_to_integer(a))
+  end
+
+  @doc """
+  Builds a map from argument value list.  For Sentry, typically the
+  key in the map would be the name of the variable, but we don't have that
+  available.
+  """
+  @spec args_from_stacktrace(Exception.stacktrace) :: String.t | nil
+  def args_from_stacktrace([{_m, _f, a, _} | _]) when is_list(a) do
+    Enum.with_index(a)
+    |> Enum.map(fn({arg, index}) ->
+      {"arg#{index}", inspect(arg)}
+    end)
+    |> Enum.into(%{})
+  end
+  def args_from_stacktrace(_), do: %{}
 
   defp arity_to_integer(arity) when is_list(arity), do: Enum.count(arity)
   defp arity_to_integer(arity) when is_integer(arity), do: arity

mix.exs

@@ -32,7 +32,7 @@ defmodule Sentry.Mixfile do
       {:dialyxir, "> 0.0.0", only: :dev},
       {:ex_doc, "~> 0.16.0", only: :dev},
       {:credo, "~> 0.8", only: [:dev, :test], runtime: false},
-      {:bypass, "~> 0.7.0", only: [:test]}
+      {:bypass, "~> 0.8.0", only: [:test]}
     ]
   end
 

mix.lock

@@ -1,5 +1,5 @@
 %{"bunt": {:hex, :bunt, "0.2.0", "951c6e801e8b1d2cbe58ebbd3e616a869061ddadcc4863d0a2182541acae9a38", [:mix], []},
-  "bypass": {:hex, :bypass, "0.7.0", "591d2ac1d87ba1bb84f62656a79de2fe3909993a211021a881825b59c9f7f58f", [:mix], [{:cowboy, "~> 1.0", [hex: :cowboy, optional: false]}, {:plug, "~> 1.0", [hex: :plug, optional: false]}]},
+  "bypass": {:hex, :bypass, "0.8.1", "16d409e05530ece4a72fabcf021a3e5c7e15dcc77f911423196a0c551f2a15ca", [:mix], [{:cowboy, "~> 1.0", [hex: :cowboy, repo: "hexpm", optional: false]}, {:plug, "~> 1.0", [hex: :plug, repo: "hexpm", optional: false]}], "hexpm"},
   "certifi": {:hex, :certifi, "1.1.0", "c9b71a547016c2528a590ccfc28de786c7edb74aafa17446b84f54e04efc00ee", [:rebar3], []},
   "cowboy": {:hex, :cowboy, "1.1.2", "61ac29ea970389a88eca5a65601460162d370a70018afe6f949a29dca91f3bb0", [:rebar3], [{:cowlib, "~> 1.0.2", [hex: :cowlib, optional: false]}, {:ranch, "~> 1.3.2", [hex: :ranch, optional: false]}]},
   "cowlib": {:hex, :cowlib, "1.0.2", "9d769a1d062c9c3ac753096f868ca121e2730b9a377de23dec0f7e08b1df84ee", [:make], []},
@@ -12,7 +12,7 @@
   "metrics": {:hex, :metrics, "1.0.1", "25f094dea2cda98213cecc3aeff09e940299d950904393b2a29d191c346a8486", [:rebar3], []},
   "mime": {:hex, :mime, "1.1.0", "01c1d6f4083d8aa5c7b8c246ade95139620ef8effb009edde934e0ec3b28090a", [:mix], []},
   "mimerl": {:hex, :mimerl, "1.0.2", "993f9b0e084083405ed8252b99460c4f0563e41729ab42d9074fd5e52439be88", [:rebar3], []},
-  "plug": {:hex, :plug, "1.3.5", "7503bfcd7091df2a9761ef8cecea666d1f2cc454cbbaf0afa0b6e259203b7031", [:mix], [{:cowboy, "~> 1.0.1 or ~> 1.1", [hex: :cowboy, optional: true]}, {:mime, "~> 1.0", [hex: :mime, optional: false]}]},
+  "plug": {:hex, :plug, "1.4.3", "236d77ce7bf3e3a2668dc0d32a9b6f1f9b1f05361019946aae49874904be4aed", [:mix], [{:cowboy, "~> 1.0.1 or ~> 1.1", [hex: :cowboy, repo: "hexpm", optional: true]}, {:mime, "~> 1.0", [hex: :mime, repo: "hexpm", optional: false]}], "hexpm"},
   "poison": {:hex, :poison, "3.1.0", "d9eb636610e096f86f25d9a46f35a9facac35609a7591b3be3326e99a0484665", [:mix], []},
   "ranch": {:hex, :ranch, "1.3.2", "e4965a144dc9fbe70e5c077c65e73c57165416a901bd02ea899cfd95aa890986", [:rebar3], []},
   "ssl_verify_fun": {:hex, :ssl_verify_fun, "1.1.1", "28a4d65b7f59893bc2c7de786dec1e1555bd742d336043fe644ae956c3497fbe", [:make, :rebar], []},

test/client_test.exs

@@ -50,7 +50,10 @@ defmodule Sentry.ClientTest do
   test "errors when attempting to report invalid JSON" do
     modify_env(:sentry, dsn: "http://public:secret@localhost:3000/1")
     unencodable_tuple = {:a, :b, :c}
-    assert :error = Sentry.capture_message(unencodable_tuple)
+
+    capture_log(fn ->
+      assert :error = Sentry.capture_message(unencodable_tuple)
+    end)
   end
 
   test "calls anonymous before_send_event" do

test/event_test.exs

@@ -5,7 +5,7 @@ defmodule Sentry.EventTest do
 
   def event_generated_by_exception(extra \\ %{}) do
     try do
-      Event.not_a_function
+      Event.not_a_function(1, 2, 3)
     rescue
       e -> Event.transform_exception(e, [stacktrace: System.stacktrace, extra: extra])
     end
@@ -15,23 +15,24 @@ defmodule Sentry.EventTest do
     event = event_generated_by_exception()
 
     assert event.platform == "elixir"
-    assert event.culprit == "Sentry.Event.not_a_function()"
+    assert event.culprit == "Sentry.Event.not_a_function/3"
     assert event.extra == %{}
     assert event.exception == [
       %{type: UndefinedFunctionError,
-       value: "function Sentry.Event.not_a_function/0 is undefined or private",
+       value: "function Sentry.Event.not_a_function/3 is undefined or private",
        module: nil}
     ]
     assert event.level == "error"
-    assert event.message == "(UndefinedFunctionError) function Sentry.Event.not_a_function/0 is undefined or private"
+    assert event.message == "(UndefinedFunctionError) function Sentry.Event.not_a_function/3 is undefined or private"
     assert is_binary(event.server_name)
-    assert event.stacktrace == %{frames: Enum.reverse([
-      %{filename: nil, function: "Sentry.Event.not_a_function/0", lineno: nil, module: Sentry.Event, context_line: nil, post_context: [], pre_context: [], in_app: false},
-      %{filename: "test/event_test.exs", function: "Sentry.EventTest.event_generated_by_exception/1", lineno: 8, module: Sentry.EventTest, context_line: nil, post_context: [], pre_context: [], in_app: false},
-      %{filename: "test/event_test.exs", function: "Sentry.EventTest.\"test parses error exception\"/1", lineno: 15, module: Sentry.EventTest, context_line: nil, post_context: [], pre_context: [], in_app: false},
-      %{filename: "lib/ex_unit/runner.ex", function: "ExUnit.Runner.exec_test/1", lineno: 302, module: ExUnit.Runner, context_line: nil, post_context: [], pre_context: [], in_app: false},
-      %{filename: "timer.erl", function: ":timer.tc/1", lineno: 166, module: :timer, context_line: nil, post_context: [], pre_context: [], in_app: false},
-      %{filename: "lib/ex_unit/runner.ex", function: "anonymous fn/3 in ExUnit.Runner.spawn_test/3", lineno: 250, module: ExUnit.Runner, context_line: nil, post_context: [], pre_context: [], in_app: false}])
+    assert event.stacktrace == %{
+      frames: Enum.reverse([
+        %{filename: nil, function: "Sentry.Event.not_a_function/3", lineno: nil, module: Sentry.Event, context_line: nil, post_context: [], pre_context: [], in_app: false, vars: %{"arg0" => "1", "arg1" => "2", "arg2" => "3"}},
+        %{filename: "test/event_test.exs", function: "Sentry.EventTest.event_generated_by_exception/1", lineno: 8, module: Sentry.EventTest, context_line: nil, post_context: [], pre_context: [], in_app: false, vars: %{}},
+        %{filename: "test/event_test.exs", function: "Sentry.EventTest.\"test parses error exception\"/1", lineno: 15, module: Sentry.EventTest, context_line: nil, post_context: [], pre_context: [], in_app: false, vars: %{}},
+        %{filename: "lib/ex_unit/runner.ex", function: "ExUnit.Runner.exec_test/1", lineno: 302, module: ExUnit.Runner, context_line: nil, post_context: [], pre_context: [], in_app: false, vars: %{}},
+        %{filename: "timer.erl", function: ":timer.tc/1", lineno: 166, module: :timer, context_line: nil, post_context: [], pre_context: [], in_app: false, vars: %{}},
+        %{filename: "lib/ex_unit/runner.ex", function: "anonymous fn/3 in ExUnit.Runner.spawn_test/3", lineno: 250, module: ExUnit.Runner, context_line: nil, post_context: [], pre_context: [], in_app: false, vars: %{}}])
     }
     assert event.tags == %{}
     assert event.timestamp =~ ~r/\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}/
@@ -83,34 +84,39 @@ defmodule Sentry.EventTest do
     event = Sentry.Event.transform_exception(exception, [stacktrace: [{Elixir.Sentry.Fun, :method, 2, []}, {Elixir.Sentry, :other_method, 4, []},
                                                                       {:other_module, :a_method, 8, []}, {:random, :uniform, 0, []},
                                                                       {Sentry.Submodule.Fun, :this_method, 0, []}]])
-      assert %{frames: [
+    assert %{
+      frames: [
         %{
           module: Sentry.Submodule.Fun,
           function: "Sentry.Submodule.Fun.this_method/0",
           in_app: true,
           filename: nil, lineno: nil,
-          context_line: nil, post_context: [], pre_context: []
+          context_line: nil, post_context: [], pre_context: [],
+          vars: %{},
         },
         %{
           module: :random,
           function: ":random.uniform/0",
           in_app: true,
           filename: nil, lineno: nil,
-          context_line: nil, post_context: [], pre_context: []
+          context_line: nil, post_context: [], pre_context: [],
+          vars: %{},
         },
         %{
           module: :other_module,
           function: ":other_module.a_method/8",
           in_app: false,
           filename: nil, lineno: nil,
-          context_line: nil, post_context: [], pre_context: []
+          context_line: nil, post_context: [], pre_context: [],
+          vars: %{},
         },
         %{
           module: Sentry,
           function: "Sentry.other_method/4",
           in_app: true,
           filename: nil, lineno: nil,
-          context_line: nil, post_context: [], pre_context: []
+          context_line: nil, post_context: [], pre_context: [],
+          vars: %{},
         },
         %{
           filename: nil,
@@ -120,7 +126,8 @@ defmodule Sentry.EventTest do
           in_app: true,
           context_line: nil,
           post_context: [],
-          pre_context: []
+          pre_context: [],
+          vars: %{},
         },
       ]} == event.stacktrace
   end
@@ -129,9 +136,9 @@ defmodule Sentry.EventTest do
     exception = RuntimeError.exception("error")
     event = Sentry.Event.transform_exception(exception, [])
 
-    assert event.modules == %{bunt: "0.2.0", bypass: "0.7.0", certifi: "1.1.0", cowboy: "1.1.2",
+    assert event.modules == %{bunt: "0.2.0", bypass: "0.8.1", certifi: "1.1.0", cowboy: "1.1.2",
       cowlib: "1.0.2", credo: "0.8.6", hackney: "1.8.0", idna: "4.0.0",
-      metrics: "1.0.1", mime: "1.1.0", mimerl: "1.0.2", plug: "1.3.5",
+      metrics: "1.0.1", mime: "1.1.0", mimerl: "1.0.2", plug: "1.4.3",
       poison: "3.1.0", ranch: "1.3.2", ssl_verify_fun: "1.1.1",
       uuid: "1.1.7"}
   end

test/logger_test.exs

@@ -94,6 +94,7 @@ defmodule Sentry.LoggerTest do
                                                           "context_line" => nil,
                                                           "pre_context" => [],
                                                           "post_context" => [],
+                                                          "vars" => %{"arg0" => "{}", "arg1" => "{0, 0}"}
                                                         }
       assert conn.request_path == "/api/1/store/"
       assert conn.method == "POST"

test/plug_test.exs

@@ -75,7 +75,7 @@ defmodule Sentry.PlugTest do
     request_data = Sentry.Plug.build_request_interface_data(conn, options)
     assert request_data[:method] == "POST"
     assert request_data[:data] == %{"hello" => "world"}
-    assert request_data[:headers] == %{"cookie" => "cookie_key=cookie_value", "accept-language" => "en-US", "content-type" => "multipart/mixed; charset: utf-8"}
+    assert request_data[:headers] == %{"cookie" => "cookie_key=cookie_value", "accept-language" => "en-US", "content-type" => "multipart/mixed; boundary=plug_conn_test"}
     assert request_data[:cookies] == %{"cookie_key" => "cookie_value"}
   end
 

test/support/test_client.exs

@@ -1,13 +1,15 @@
 defmodule Sentry.TestClient do
   @behaviour Sentry.HTTPClient
+  require Logger
 
   def send_event(%Sentry.Event{} = event, _opts \\ []) do
     {endpoint, _public_key, _secret_key} = Sentry.Client.get_dsn!
     event = Sentry.Client.maybe_call_before_send_event(event)
     case Poison.encode(event) do
       {:ok, body} ->
         Sentry.Client.request(:post, endpoint, [], body)
-      {:error, _error} ->
+      {:error, error} ->
+        Logger.error("Error sending in Sentry.TestClient: #{inspect(error)}")
         :error
     end
   end