From 17985cfc7796ec3fffc1aeef31a50eabd1224983 Mon Sep 17 00:00:00 2001 From: Andrea Leopardi Date: Sat, 9 Dec 2023 16:00:57 +0100 Subject: [PATCH] Add threads interface and fix messages with stacktraces (#668) --- lib/sentry/client.ex | 9 ++++++ lib/sentry/event.ex | 43 +++++++++++++++++++---------- lib/sentry/interfaces.ex | 34 +++++++++++++++++++++++ test/event_test.exs | 35 +++++++++++++++++++++++ test/logger_backend_test.exs | 38 ++++++++++++------------- test/sentry/logger_handler_test.exs | 14 +++++----- 6 files changed, 132 insertions(+), 41 deletions(-) diff --git a/lib/sentry/client.ex b/lib/sentry/client.ex index 45f2dbb7..cc64e160 100644 --- a/lib/sentry/client.ex +++ b/lib/sentry/client.ex @@ -154,6 +154,7 @@ defmodule Sentry.Client do |> update_if_present(:user, &sanitize_non_jsonable_values(&1, json_library)) |> update_if_present(:tags, &sanitize_non_jsonable_values(&1, json_library)) |> update_if_present(:exception, fn list -> Enum.map(list, &render_exception/1) end) + |> update_if_present(:threads, fn list -> Enum.map(list, &render_thread/1) end) end defp render_exception(%Interfaces.Exception{} = exception) do @@ -164,6 +165,14 @@ defmodule Sentry.Client do end) end + defp render_thread(%Interfaces.Thread{} = thread) do + thread + |> Map.from_struct() + |> update_if_present(:stacktrace, fn %Interfaces.Stacktrace{frames: frames} -> + %{frames: Enum.map(frames, &Map.from_struct/1)} + end) + end + defp remove_nils(map) when is_map(map) do :maps.filter(fn _key, value -> not is_nil(value) end, map) end diff --git a/lib/sentry/event.ex b/lib/sentry/event.ex index d03082c9..fb93c733 100644 --- a/lib/sentry/event.ex +++ b/lib/sentry/event.ex @@ -67,6 +67,7 @@ defmodule Sentry.Event do message: String.t() | nil, request: Interfaces.Request.t() | nil, sdk: Interfaces.SDK.t() | nil, + threads: [Interfaces.Thread.t()] | nil, user: Interfaces.user() | nil, # Non-payload fields. @@ -113,6 +114,7 @@ defmodule Sentry.Event do server_name: nil, tags: %{}, transaction: nil, + threads: nil, user: %{}, # "Culprit" is not documented anymore and we should move to transactions at some point. @@ -255,7 +257,7 @@ defmodule Sentry.Event do stacktrace = Keyword.get(opts, :stacktrace) source = Keyword.get(opts, :event_source) - %__MODULE__{ + event = %__MODULE__{ breadcrumbs: breadcrumbs, contexts: generate_contexts(), culprit: culprit_from_stacktrace(Keyword.get(opts, :stacktrace, [])), @@ -277,23 +279,25 @@ defmodule Sentry.Event do timestamp: timestamp, user: user } - end - defp coerce_exception(_exception = nil, _stacktrace = nil, _message) do - nil + # If we have a message *and* a stacktrace, but no exception, we need to store the stacktrace + # information within a "thread" interface. This is how the Python SDK also does it. An issue + # was opened in the sentry-elixir repo about this, but this is also a Sentry issue (if there + # is an exception of type "message" with a stacktrace *and* a "message" attribute, it should + # still show properly). This issue is now tracked in Sentry itself: + # https://github.com/getsentry/sentry/issues/61239 + if message && stacktrace && is_nil(exception) do + add_thread_with_stacktrace(event, stacktrace) + else + event + end end - defp coerce_exception(_exception = nil, stacktrace_or_nil, message) when is_binary(message) do - stacktrace = - if is_list(stacktrace_or_nil) do - %Interfaces.Stacktrace{frames: stacktrace_to_frames(stacktrace_or_nil)} - end - - %Interfaces.Exception{ - type: "message", - value: message, - stacktrace: stacktrace - } + # If we have a message with a stacktrace, but no exceptions, for now we store the stacktrace in + # the "threads" interface and we don't fill in the "exception" interface altogether. This might + # be eventually fixed in Sentry itself: https://github.com/getsentry/sentry/issues/61239 + defp coerce_exception(_exception = nil, _stacktrace_or_nil, message) when is_binary(message) do + nil end defp coerce_exception(exception, stacktrace_or_nil, _message) when is_exception(exception) do @@ -328,6 +332,15 @@ defmodule Sentry.Event do end) end + defp add_thread_with_stacktrace(%__MODULE__{} = event, stacktrace) when is_list(stacktrace) do + thread = %Interfaces.Thread{ + id: UUID.uuid4_hex(), + stacktrace: %Interfaces.Stacktrace{frames: stacktrace_to_frames(stacktrace)} + } + + %__MODULE__{event | threads: [thread]} + end + @doc """ Transforms an exception to a Sentry event. diff --git a/lib/sentry/interfaces.ex b/lib/sentry/interfaces.ex index 7937691b..c646f37a 100644 --- a/lib/sentry/interfaces.ex +++ b/lib/sentry/interfaces.ex @@ -190,4 +190,38 @@ defmodule Sentry.Interfaces do defstruct [:type, :category, :message, :data, :level, :timestamp] end + + defmodule Thread do + @moduledoc """ + The struct for the **thread** interface. + + See . + """ + + @moduledoc since: "10.1.0" + + @typedoc since: "10.1.0" + @type t() :: %__MODULE__{ + id: term(), + crashed: boolean() | nil, + current: boolean() | nil, + main: boolean() | nil, + name: String.t() | nil, + state: term(), + held_locks: [term()], + stacktrace: Sentry.Interfaces.Stacktrace.t() | nil + } + + @enforce_keys [:id] + defstruct [ + :id, + :crashed, + :current, + :main, + :name, + :state, + :held_locks, + :stacktrace + ] + end end diff --git a/test/event_test.exs b/test/event_test.exs index d08308d4..5d864ceb 100644 --- a/test/event_test.exs +++ b/test/event_test.exs @@ -210,6 +210,41 @@ defmodule Sentry.EventTest do } = Event.create_event(message: "Test message") end + test "fills in the message and threads interfaces when passing the :message option with :stacktrace" do + {:current_stacktrace, stacktrace} = Process.info(self(), :current_stacktrace) + put_test_config(environment_name: "my_env") + + assert %Event{ + breadcrumbs: [], + environment: "my_env", + exception: [], + extra: %{}, + level: :error, + message: "Test message", + platform: :elixir, + release: nil, + request: %{}, + tags: %{}, + user: %{}, + contexts: %{os: %{name: _, version: _}, runtime: %{name: _, version: _}}, + threads: [%Interfaces.Thread{id: thread_id, stacktrace: thread_stacktrace}] + } = Event.create_event(message: "Test message", stacktrace: stacktrace) + + assert is_binary(thread_id) and byte_size(thread_id) > 0 + + assert [ + %Interfaces.Stacktrace.Frame{ + context_line: nil, + in_app: false, + lineno: _, + post_context: [], + pre_context: [], + vars: %{} + } + | _rest + ] = thread_stacktrace.frames + end + test "fills in private (:__...__) fields" do exception = %RuntimeError{message: "foo"} diff --git a/test/logger_backend_test.exs b/test/logger_backend_test.exs index 83b72aba..e526abdc 100644 --- a/test/logger_backend_test.exs +++ b/test/logger_backend_test.exs @@ -37,12 +37,13 @@ defmodule Sentry.LoggerBackendTest do pid = start_supervised!(TestGenServer) TestGenServer.run_async(pid, fn _state -> throw("I am throwing") end) assert_receive {^ref, event} - assert [exception] = event.exception - assert exception.value =~ ~s - assert exception.value =~ ~s<** (stop) bad return value: "I am throwing"\n> - assert exception.value =~ ~s - assert exception.value =~ ~s - assert exception.stacktrace.frames == [] + assert [] = event.exception + assert [thread] = event.threads + assert event.message =~ ~s + assert event.message =~ ~s<** (stop) bad return value: "I am throwing"\n> + assert event.message =~ ~s + assert event.message =~ ~s + assert thread.stacktrace.frames == [] end test "abnormal GenServer exit is reported" do @@ -51,12 +52,12 @@ defmodule Sentry.LoggerBackendTest do pid = start_supervised!(TestGenServer) TestGenServer.run_async(pid, fn state -> {:stop, :bad_exit, state} end) assert_receive {^ref, event} - assert [exception] = event.exception - assert exception.type == "message" - assert exception.value =~ ~s - assert exception.value =~ ~s<** (stop) :bad_exit\n> - assert exception.value =~ ~s - assert exception.value =~ ~s + assert [] = event.exception + assert [_thread] = event.threads + assert event.message =~ ~s + assert event.message =~ ~s<** (stop) :bad_exit\n> + assert event.message =~ ~s + assert event.message =~ ~s end test "bad function call causing GenServer crash is reported" do @@ -96,16 +97,15 @@ defmodule Sentry.LoggerBackendTest do assert_receive {^ref, event} - assert [exception] = event.exception - - assert exception.type == "message" + assert [] = event.exception + assert [thread] = event.threads - assert exception.value =~ + assert event.message =~ "Task #{inspect(task_pid)} started from #{inspect(self())} terminating\n" - assert exception.value =~ "** (stop) exited in: GenServer.call(" - assert exception.value =~ "** (EXIT) time out" - assert length(exception.stacktrace.frames) > 0 + assert event.message =~ "** (stop) exited in: GenServer.call(" + assert event.message =~ "** (EXIT) time out" + assert length(thread.stacktrace.frames) > 0 end test "captures errors from spawn/0 in Plug app" do diff --git a/test/sentry/logger_handler_test.exs b/test/sentry/logger_handler_test.exs index 75554d65..7249180d 100644 --- a/test/sentry/logger_handler_test.exs +++ b/test/sentry/logger_handler_test.exs @@ -175,7 +175,8 @@ defmodule Sentry.LoggerHandlerTest do assert event.message =~ "** (stop) :bad_exit" if System.otp_release() >= "26" do - assert hd(event.exception).type == "message" + assert [] = event.exception + assert [_thread] = event.threads end end @@ -273,13 +274,12 @@ defmodule Sentry.LoggerHandlerTest do assert_receive {^ref, event} - assert [exception] = event.exception - - assert exception.type == "message" + assert [] = event.exception + assert [thread] = event.threads - assert exception.value =~ "** (stop) exited in: GenServer.call(" - assert exception.value =~ "** (EXIT) time out" - assert length(exception.stacktrace.frames) > 0 + assert event.message =~ "** (stop) exited in: GenServer.call(" + assert event.message =~ "** (EXIT) time out" + assert length(thread.stacktrace.frames) > 0 end test "reports crashes on c:GenServer.init/1", %{sender_ref: ref} do