From b282b9494a1c87cbaa245879eb06e1de357a522d Mon Sep 17 00:00:00 2001 From: Ignacio Aguirrezabal Date: Wed, 6 Apr 2022 09:56:57 -0300 Subject: [PATCH 1/8] Refactor `ErrorStorage` module Currently, we are grouping every error information in several lists according to its error type. Eventually, this could consume a lot of memory without providing any important benefit (the only piece of information that changes is the timestamp and the request data). This commit replaces that with a map structure where its key is the hashed error info and its value is the actual error info plus: * the number of times this error occurred, * the first and the list time it happened. --- lib/boom_notifier/error_storage.ex | 154 +++++++++++--- test/error_storage_test.exs | 321 ++++++++++++++++++----------- 2 files changed, 331 insertions(+), 144 deletions(-) diff --git a/lib/boom_notifier/error_storage.ex b/lib/boom_notifier/error_storage.ex index d7e7d8b..80d839c 100644 --- a/lib/boom_notifier/error_storage.ex +++ b/lib/boom_notifier/error_storage.ex @@ -4,6 +4,17 @@ defmodule BoomNotifier.ErrorStorage do # Keeps track of the errors grouped by type and a counter so the notifier # knows the next time it should be executed + @enforce_keys [:accumulated_occurrences, :first_occurrence, :last_occurrence] + defstruct [ + :accumulated_occurrences, + :first_occurrence, + :last_occurrence, + :__max_storage_capacity__ + ] + + @type t :: %__MODULE__{} + @type error_strategy :: :always | :exponential | [exponential: [limit: non_neg_integer()]] + use Agent, start: {__MODULE__, :start_link, []} @spec start_link() :: Agent.on_start() @@ -11,57 +22,144 @@ defmodule BoomNotifier.ErrorStorage do Agent.start_link(fn -> %{} end, name: :boom_notifier) end - @spec add_errors(atom(), ErrorInfo.t()) :: :ok - def add_errors(error_kind, error_info) do + @doc """ + Stores information about how many times an error occurred. + + This information is stored in a map where the key is the error info hash. + Every time this function is called with an error info, the accumulated + occurrences is increased and it also updates the first and last time it + happened. + """ + @spec store_error(ErrorInfo.t()) :: :ok + def store_error(error_info) do + error_hash_key = generate_error_key(error_info) + timestamp = error_info.timestamp || DateTime.utc_now() + + default_error_storage_info = %__MODULE__{ + accumulated_occurrences: 1, + first_occurrence: timestamp, + last_occurrence: timestamp, + __max_storage_capacity__: 1 + } + Agent.update( :boom_notifier, - &Map.update(&1, error_kind, {1, [error_info]}, fn {counter, errors} -> - {counter, [error_info | errors]} - end) + &Map.update( + &1, + error_hash_key, + default_error_storage_info, + fn error_storage_item -> + error_storage_item + |> Map.update!(:accumulated_occurrences, fn current -> current + 1 end) + |> Map.update!(:first_occurrence, fn first_occurrence -> + first_occurrence || timestamp + end) + |> Map.update!(:last_occurrence, fn _ -> timestamp end) + end + ) ) end - @spec get_errors(atom()) :: list(ErrorInfo.t()) - def get_errors(error_kind) do + @doc """ + Given an error info, it returns the aggregated info stored in the agent. + """ + @spec get_error_stats(ErrorInfo.t()) :: %__MODULE__{} + def get_error_stats(error_info) do + error_hash_key = generate_error_key(error_info) + Agent.get(:boom_notifier, fn state -> state end) - |> Map.get(error_kind) - |> case do - nil -> nil - {_counter, errors} -> errors - end + |> Map.get(error_hash_key) end - @spec send_notification?(atom()) :: boolean() - def send_notification?(error_kind) do - Agent.get(:boom_notifier, fn state -> state end) - |> Map.get(error_kind) - |> case do - nil -> false - {counter, errors} -> length(errors) >= counter - end + @doc """ + Tells whether a notification for an error info is ready to be sent. + + Returns true if the accumulated_occurrences is above or equal to the max + storage capacity. + """ + @spec send_notification?(ErrorInfo.t()) :: boolean() + def send_notification?(error_info) do + error_hash_key = generate_error_key(error_info) + + error_storage_item = + Agent.get(:boom_notifier, fn state -> state end) + |> Map.get(error_hash_key) + + do_send_notification?(error_storage_item) end - @type error_strategy :: :always | :exponential | [exponential: [limit: non_neg_integer()]] + @doc """ + Reset the accumulated_occurrences for the given error info to zero. It also + increments the max storage capacity based on the notification strategy. + """ + @spec reset_accumulated_errors(error_strategy, ErrorInfo.t()) :: :ok + def reset_accumulated_errors(:exponential, error_info) do + error_hash_key = generate_error_key(error_info) - @spec clear_errors(error_strategy, atom()) :: :ok - def clear_errors(:exponential, error_kind) do Agent.update( :boom_notifier, - &Map.update!(&1, error_kind, fn {counter, _errors} -> {counter * 2, []} end) + &Map.update!(&1, error_hash_key, fn error_storage_item -> + clear_values(error_storage_item) + |> Map.update!(:__max_storage_capacity__, fn current -> current * 2 end) + end) ) end - def clear_errors([exponential: [limit: limit]], error_kind) do + def reset_accumulated_errors([exponential: [limit: limit]], error_info) do + error_hash_key = generate_error_key(error_info) + Agent.update( :boom_notifier, - &Map.update!(&1, error_kind, fn {counter, _errors} -> {min(counter * 2, limit), []} end) + &Map.update!(&1, error_hash_key, fn error_storage_item -> + clear_values(error_storage_item) + |> Map.update!(:__max_storage_capacity__, fn current -> min(current * 2, limit) end) + end) ) end - def clear_errors(:always, error_kind) do + def reset_accumulated_errors(:always, error_info) do + error_hash_key = generate_error_key(error_info) + Agent.update( :boom_notifier, - &Map.update!(&1, error_kind, fn _value -> {1, []} end) + &Map.update!(&1, error_hash_key, fn error_storage_item -> + clear_values(error_storage_item) + |> Map.replace!(:__max_storage_capacity__, 1) + end) ) end + + # Generates a unique hash key based on the error info. The timestamp and the + # request info is removed so we don't get different keys for the same error. + # + # The map is converted to a string using `inspect()` so we can hash it using + # the crc32 algorithm that was taken from the Exception Notification library + # for Rails + @spec generate_error_key(ErrorInfo.t()) :: non_neg_integer() + defp generate_error_key(error_info) do + error_info + |> Map.delete(:request) + |> Map.delete(:metadata) + |> Map.delete(:timestamp) + |> Map.update(:stack, nil, fn stacktrace -> List.first(stacktrace) end) + |> inspect() + |> :erlang.crc32() + end + + defp clear_values(error_storage_item) do + error_storage_item + |> Map.replace!(:accumulated_occurrences, 0) + |> Map.replace!(:first_occurrence, nil) + |> Map.replace!(:last_occurrence, nil) + end + + @spec do_send_notification?(ErrorInfo.t() | nil) :: boolean() + defp do_send_notification?(nil), do: false + + defp do_send_notification?(error_storage_item) do + accumulated_occurrences = Map.get(error_storage_item, :accumulated_occurrences) + max_storage_capacity = Map.get(error_storage_item, :__max_storage_capacity__) + + if accumulated_occurrences >= max_storage_capacity, do: true, else: false + end end diff --git a/test/error_storage_test.exs b/test/error_storage_test.exs index dc9cd8d..afe8636 100644 --- a/test/error_storage_test.exs +++ b/test/error_storage_test.exs @@ -1,11 +1,17 @@ defmodule ErrorStorageTest do use ExUnit.Case, async: true + alias BoomNotifier.ErrorStorage - @error_info "Some error information" - @error_kind :error_kind + @timestamp DateTime.utc_now() + + @error_info %ErrorInfo{ + reason: "Some error information", + stack: ["line 1"], + timestamp: @timestamp + } setup_all do - BoomNotifier.ErrorStorage.start_link() + ErrorStorage.start_link() :ok end @@ -13,158 +19,241 @@ defmodule ErrorStorageTest do Agent.update(:boom_notifier, fn _ -> %{} end) end - describe "add_errors/2" do - test "appends the error to its proper error kind" do - BoomNotifier.ErrorStorage.add_errors(@error_kind, @error_info) - - assert %{@error_kind => {1, [@error_info]}} == - Agent.get(:boom_notifier, fn state -> state end) - - BoomNotifier.ErrorStorage.add_errors(@error_kind, @error_info) - - assert %{@error_kind => {1, [@error_info, @error_info]}} == - Agent.get(:boom_notifier, fn state -> state end) - - BoomNotifier.ErrorStorage.add_errors(:another_error_kind, "Another error information") + describe "store_error/1" do + test "groups errors by type" do + another_timestamp = DateTime.utc_now() - assert %{ - @error_kind => {1, [@error_info, @error_info]}, - :another_error_kind => {1, ["Another error information"]} + another_error_info = %ErrorInfo{ + reason: "Another error information", + stack: ["line 2"], + timestamp: another_timestamp } + + ErrorStorage.store_error(@error_info) + ErrorStorage.store_error(@error_info) + ErrorStorage.store_error(another_error_info) + + [error_stat_1, error_stat_2] = + Agent.get(:boom_notifier, fn state -> state end) + |> Map.values() + + assert error_stat_1 == %ErrorStorage{ + __max_storage_capacity__: 1, + accumulated_occurrences: 2, + first_occurrence: @timestamp, + last_occurrence: @timestamp + } + + assert error_stat_2 == %ErrorStorage{ + __max_storage_capacity__: 1, + accumulated_occurrences: 1, + first_occurrence: another_timestamp, + last_occurrence: another_timestamp + } end end - describe "get_errors/1" do + describe "get_error_stats/1" do test "returns the errors for the proper error kind" do - Agent.update(:boom_notifier, fn _ -> - %{ - @error_kind => {1, [@error_info, @error_info]}, - :another_error_kind => {1, ["another_error"]} - } - end) - - assert [@error_info, @error_info] == BoomNotifier.ErrorStorage.get_errors(@error_kind) - assert ["another_error"] == BoomNotifier.ErrorStorage.get_errors(:another_error_kind) + ErrorStorage.store_error(@error_info) + ErrorStorage.store_error(@error_info) + + assert ErrorStorage.get_error_stats(@error_info) == + %ErrorStorage{ + __max_storage_capacity__: 1, + accumulated_occurrences: 2, + first_occurrence: @timestamp, + last_occurrence: @timestamp + } + + another_timestamp = DateTime.utc_now() + + another_error_info = %ErrorInfo{ + reason: "Another error information", + stack: ["line 2"], + timestamp: another_timestamp + } + + ErrorStorage.store_error(another_error_info) + + assert ErrorStorage.get_error_stats(another_error_info) == + %ErrorStorage{ + __max_storage_capacity__: 1, + accumulated_occurrences: 1, + first_occurrence: another_timestamp, + last_occurrence: another_timestamp + } end - test "returns nil if error kind does not exist" do - assert nil == BoomNotifier.ErrorStorage.get_errors(:wrong_error_kind) + test "returns nil if error info does not exist" do + assert ErrorStorage.get_error_stats(@error_info) == nil end end describe "send_notification?/1" do test "returns false when count is smaller than the error length" do - Agent.update(:boom_notifier, fn _ -> %{@error_kind => {2, [@error_info]}} end) - assert false == BoomNotifier.ErrorStorage.send_notification?(@error_kind) + # increase the max capacity to 2 + ErrorStorage.store_error(@error_info) + ErrorStorage.reset_accumulated_errors(:exponential, @error_info) + ErrorStorage.store_error(@error_info) + + refute ErrorStorage.send_notification?(@error_info) end test "returns true when error length is greater or equal than count" do - Agent.update(:boom_notifier, fn _ -> %{@error_kind => {2, [@error_info, @error_info]}} end) - assert true == BoomNotifier.ErrorStorage.send_notification?(@error_kind) + # creates the error key + ErrorStorage.store_error(@error_info) + # increase the max capacity to 2 + ErrorStorage.reset_accumulated_errors(:exponential, @error_info) + ErrorStorage.store_error(@error_info) + ErrorStorage.store_error(@error_info) + + another_error_info = %ErrorInfo{ + reason: "Another error information", + stack: ["line 2"], + timestamp: @timestamp + } + + # creates the error key + ErrorStorage.store_error(another_error_info) + # increase the max capacity to 2 + ErrorStorage.reset_accumulated_errors(:exponential, another_error_info) + ErrorStorage.store_error(another_error_info) + ErrorStorage.store_error(another_error_info) + ErrorStorage.store_error(another_error_info) + + assert ErrorStorage.send_notification?(@error_info) + assert ErrorStorage.send_notification?(another_error_info) end test "returns false when error kind does not exist" do - assert false == BoomNotifier.ErrorStorage.send_notification?(:wrong_error_kind) + refute ErrorStorage.send_notification?(%{}) end end - describe "clear_errors/2" do - test "flushes error list" do - Agent.update(:boom_notifier, fn _ -> %{@error_kind => {2, [@error_info, @error_info]}} end) - BoomNotifier.ErrorStorage.clear_errors(:exponential, @error_kind) - - {_count, errors} = Agent.get(:boom_notifier, fn state -> state end) |> Map.get(@error_kind) - assert errors == [] - - Agent.update(:boom_notifier, fn _ -> %{@error_kind => {2, [@error_info, @error_info]}} end) - BoomNotifier.ErrorStorage.clear_errors(:always, @error_kind) - - {_count, errors} = Agent.get(:boom_notifier, fn state -> state end) |> Map.get(@error_kind) - assert errors == [] - end - + describe "reset_accumulated_errors/2" do test "increases the counter when notification trigger is :exponential" do - Agent.update(:boom_notifier, fn _ -> %{@error_kind => {1, []}} end) - - BoomNotifier.ErrorStorage.clear_errors(:exponential, @error_kind) - - {counter, _errors} = - Agent.get(:boom_notifier, fn state -> state end) |> Map.get(@error_kind) - - assert counter === 2 - - BoomNotifier.ErrorStorage.clear_errors(:exponential, @error_kind) - - {counter, _errors} = - Agent.get(:boom_notifier, fn state -> state end) |> Map.get(@error_kind) - - assert counter === 4 - - BoomNotifier.ErrorStorage.clear_errors(:exponential, @error_kind) - - {counter, _errors} = - Agent.get(:boom_notifier, fn state -> state end) |> Map.get(@error_kind) - - assert counter === 8 + ErrorStorage.store_error(@error_info) + + ErrorStorage.reset_accumulated_errors(:exponential, @error_info) + [error_stat] = Agent.get(:boom_notifier, fn state -> state end) |> Map.values() + + assert error_stat == %ErrorStorage{ + __max_storage_capacity__: 2, + accumulated_occurrences: 0, + first_occurrence: nil, + last_occurrence: nil + } + + ErrorStorage.reset_accumulated_errors(:exponential, @error_info) + [error_stat] = Agent.get(:boom_notifier, fn state -> state end) |> Map.values() + + assert error_stat == %ErrorStorage{ + __max_storage_capacity__: 4, + accumulated_occurrences: 0, + first_occurrence: nil, + last_occurrence: nil + } + + ErrorStorage.reset_accumulated_errors(:exponential, @error_info) + [error_stat] = Agent.get(:boom_notifier, fn state -> state end) |> Map.values() + + assert error_stat == %ErrorStorage{ + __max_storage_capacity__: 8, + accumulated_occurrences: 0, + first_occurrence: nil, + last_occurrence: nil + } end test "increases the counter when notification trigger is :exponential and :limit is set" do - Agent.update(:boom_notifier, fn _ -> %{@error_kind => {1, []}} end) - - BoomNotifier.ErrorStorage.clear_errors([exponential: [limit: 5]], @error_kind) - - {counter, _errors} = - Agent.get(:boom_notifier, fn state -> state end) |> Map.get(@error_kind) - - assert counter === 2 - - BoomNotifier.ErrorStorage.clear_errors([exponential: [limit: 5]], @error_kind) - - {counter, _errors} = - Agent.get(:boom_notifier, fn state -> state end) |> Map.get(@error_kind) - - assert counter === 4 - - BoomNotifier.ErrorStorage.clear_errors([exponential: [limit: 5]], @error_kind) - - {counter, _errors} = - Agent.get(:boom_notifier, fn state -> state end) |> Map.get(@error_kind) - - assert counter === 5 + ErrorStorage.store_error(@error_info) + + ErrorStorage.reset_accumulated_errors([exponential: [limit: 5]], @error_info) + [error_stat] = Agent.get(:boom_notifier, fn state -> state end) |> Map.values() + + assert error_stat == %ErrorStorage{ + __max_storage_capacity__: 2, + accumulated_occurrences: 0, + first_occurrence: nil, + last_occurrence: nil + } + + ErrorStorage.reset_accumulated_errors([exponential: [limit: 5]], @error_info) + [error_stat] = Agent.get(:boom_notifier, fn state -> state end) |> Map.values() + + assert error_stat == %ErrorStorage{ + __max_storage_capacity__: 4, + accumulated_occurrences: 0, + first_occurrence: nil, + last_occurrence: nil + } + + ErrorStorage.reset_accumulated_errors([exponential: [limit: 5]], @error_info) + [error_stat] = Agent.get(:boom_notifier, fn state -> state end) |> Map.values() + + assert error_stat == %ErrorStorage{ + __max_storage_capacity__: 5, + accumulated_occurrences: 0, + first_occurrence: nil, + last_occurrence: nil + } end test "does not increase the counter when notification_trigger is :always" do - Agent.update(:boom_notifier, fn _ -> %{@error_kind => {1, []}} end) - BoomNotifier.ErrorStorage.clear_errors(:always, @error_kind) + ErrorStorage.store_error(@error_info) - {counter, _errors} = - Agent.get(:boom_notifier, fn state -> state end) |> Map.get(@error_kind) + ErrorStorage.reset_accumulated_errors(:always, @error_info) + [error_stat] = Agent.get(:boom_notifier, fn state -> state end) |> Map.values() - assert counter === 1 + assert error_stat == %ErrorStorage{ + __max_storage_capacity__: 1, + accumulated_occurrences: 0, + first_occurrence: nil, + last_occurrence: nil + } - BoomNotifier.ErrorStorage.clear_errors(:always, @error_kind) + ErrorStorage.reset_accumulated_errors(:always, @error_info) + [error_stat] = Agent.get(:boom_notifier, fn state -> state end) |> Map.values() - {counter, _errors} = - Agent.get(:boom_notifier, fn state -> state end) |> Map.get(@error_kind) + assert error_stat == %ErrorStorage{ + __max_storage_capacity__: 1, + accumulated_occurrences: 0, + first_occurrence: nil, + last_occurrence: nil + } - assert counter === 1 + ErrorStorage.reset_accumulated_errors(:always, @error_info) end - test "updates the proper error counter" do - Agent.update(:boom_notifier, fn _ -> - %{@error_kind => {1, ["error1", "error2"]}, :another_error_kind => {1, ["another_error"]}} - end) + test "updates the proper error max capacity" do + another_error_info = %ErrorInfo{ + reason: "Another error information", + stack: ["line 2"], + timestamp: @timestamp + } + + ErrorStorage.store_error(@error_info) + ErrorStorage.store_error(another_error_info) + + ErrorStorage.reset_accumulated_errors(:exponential, @error_info) - BoomNotifier.ErrorStorage.clear_errors(:exponential, @error_kind) - {counter, errors} = Agent.get(:boom_notifier, fn state -> state end) |> Map.get(@error_kind) - assert counter == 2 - assert errors == [] + [error_stat_1, error_stat_2] = + Agent.get(:boom_notifier, fn state -> state end) |> Map.values() - {counter, errors} = - Agent.get(:boom_notifier, fn state -> state end) |> Map.get(:another_error_kind) + assert error_stat_1 == %ErrorStorage{ + __max_storage_capacity__: 2, + accumulated_occurrences: 0, + first_occurrence: nil, + last_occurrence: nil + } - assert counter == 1 - assert errors == ["another_error"] + assert error_stat_2 == %ErrorStorage{ + __max_storage_capacity__: 1, + accumulated_occurrences: 1, + first_occurrence: @timestamp, + last_occurrence: @timestamp + } end end end From 51ea32594594824e1f313347b0d11e8364753b87 Mon Sep 17 00:00:00 2001 From: Ignacio Aguirrezabal Date: Wed, 6 Apr 2022 10:50:03 -0300 Subject: [PATCH 2/8] Send error stats to notifiers --- lib/boom_notifier.ex | 12 +++++++----- lib/boom_notifier/error_info.ex | 12 +++++++++++- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/lib/boom_notifier.ex b/lib/boom_notifier.ex index 68794e4..d8a6234 100644 --- a/lib/boom_notifier.ex +++ b/lib/boom_notifier.ex @@ -87,20 +87,22 @@ defmodule BoomNotifier do {custom_data, _settings} = Keyword.pop(settings, :custom_data, :nothing) {error_kind, error_info} = ErrorInfo.build(error, conn, custom_data) - ErrorStorage.add_errors(error_kind, error_info) + ErrorStorage.store_error(error_info) - if ErrorStorage.send_notification?(error_kind) do - occurrences = ErrorStorage.get_errors(error_kind) + if ErrorStorage.send_notification?(error_info) do + notification_data = [ + Map.put(error_info, :occurrences, ErrorStorage.get_error_stats(error_info)) + ] # Triggers the notification in each notifier walkthrough_notifiers(settings, fn notifier, options -> - NotifierSenderServer.send(notifier, occurrences, options) + NotifierSenderServer.send(notifier, notification_data, options) end) {notification_trigger, _settings} = Keyword.pop(settings, :notification_trigger, :always) - ErrorStorage.clear_errors(notification_trigger, error_kind) + ErrorStorage.reset_accumulated_errors(notification_trigger, error_info) end end end diff --git a/lib/boom_notifier/error_info.ex b/lib/boom_notifier/error_info.ex index 6c0c4af..4b47b02 100644 --- a/lib/boom_notifier/error_info.ex +++ b/lib/boom_notifier/error_info.ex @@ -7,7 +7,17 @@ defmodule ErrorInfo do # among other things) and custom data depending on the configuration. @enforce_keys [:reason, :stack, :timestamp] - defstruct [:name, :reason, :stack, :controller, :action, :request, :timestamp, :metadata] + defstruct [ + :name, + :reason, + :stack, + :controller, + :action, + :request, + :timestamp, + :metadata, + :occurrences + ] @type t :: %ErrorInfo{} From 1c9372b020c5dd59751fe44a149c60ad3a7dfa63 Mon Sep 17 00:00:00 2001 From: Ignacio Aguirrezabal Date: Wed, 6 Apr 2022 10:53:56 -0300 Subject: [PATCH 3/8] Update notifiers --- .../controllers/page_controller.ex | 4 +- .../page/check_custom_notifier.html.heex | 9 +- example_app/test/send_notification_test.exs | 21 +--- lib/boom_notifier.ex | 3 +- lib/boom_notifier/mail_notifier.ex | 4 +- lib/boom_notifier/mail_notifier/bamboo.ex | 2 +- .../mail_notifier/html_content.ex | 7 +- lib/boom_notifier/mail_notifier/swoosh.ex | 2 +- .../templates/email_body.html.eex | 111 +++++++++--------- .../templates/email_body.text.eex | 60 +++++----- .../mail_notifier/text_content.ex | 7 +- lib/boom_notifier/notifier.ex | 2 +- lib/boom_notifier/webhook_notifier.ex | 12 +- test/mailer_notifier_test.exs | 5 +- test/notifier_test.exs | 32 ++--- test/webhook_notifier_test.exs | 16 ++- 16 files changed, 125 insertions(+), 172 deletions(-) diff --git a/example_app/lib/example_app_web/controllers/page_controller.ex b/example_app/lib/example_app_web/controllers/page_controller.ex index d9a9a2e..650325a 100644 --- a/example_app/lib/example_app_web/controllers/page_controller.ex +++ b/example_app/lib/example_app_web/controllers/page_controller.ex @@ -38,7 +38,7 @@ defmodule ExampleAppWeb.PageController do end def check_custom_notifier(conn, _params) do - errors = ExampleApp.MemoryErrorStorage.get_error() - render(conn, "check_custom_notifier.html", errors: errors) + error = ExampleApp.MemoryErrorStorage.get_error() + render(conn, "check_custom_notifier.html", error: error) end end diff --git a/example_app/lib/example_app_web/templates/page/check_custom_notifier.html.heex b/example_app/lib/example_app_web/templates/page/check_custom_notifier.html.heex index 6dd6e2f..66dd829 100644 --- a/example_app/lib/example_app_web/templates/page/check_custom_notifier.html.heex +++ b/example_app/lib/example_app_web/templates/page/check_custom_notifier.html.heex @@ -1,6 +1,3 @@ -<%= for error <- @errors do %> - 
-        <%= inspect(error) %>
-
-
-<% end %> \ No newline at end of file +
+    <%= inspect(@error) %>
+
diff --git a/example_app/test/send_notification_test.exs b/example_app/test/send_notification_test.exs index 8eec3bf..b9a0a69 100644 --- a/example_app/test/send_notification_test.exs +++ b/example_app/test/send_notification_test.exs @@ -88,21 +88,10 @@ defmodule ExampleAppWeb.SendNotificationTest do |> visit("/group-exception") |> assert_text("GroupExceptionError at GET /group-exception") - items = - session - |> visit("/mailbox") - |> find(Query.css(".list-group")) - |> find(Query.css(".list-group-item", count: expected_emails)) - - item = if expected_emails == 1, do: items, else: List.first(items) - - email_page = select_email(session, item) - - email_body_sections = - text(email_page, Query.css(".body-text")) - |> String.split("----------------------------------------", trim: true) - - assert(length(email_body_sections) == expected_notifications) + session + |> visit("/mailbox") + |> find(Query.css(".list-group")) + |> find(Query.css(".list-group-item", count: expected_emails)) end end @@ -115,7 +104,7 @@ defmodule ExampleAppWeb.SendNotificationTest do |> visit("/check-custom-notifier") |> assert_has( Query.css(".error", - text: "name: CustomNotifierExceptionError, reason: \"custom notifier exception error\"" + text: "name: CustomNotifierExceptionError" ) ) diff --git a/lib/boom_notifier.ex b/lib/boom_notifier.ex index d8a6234..1d61b37 100644 --- a/lib/boom_notifier.ex +++ b/lib/boom_notifier.ex @@ -90,9 +90,8 @@ defmodule BoomNotifier do ErrorStorage.store_error(error_info) if ErrorStorage.send_notification?(error_info) do - notification_data = [ + notification_data = Map.put(error_info, :occurrences, ErrorStorage.get_error_stats(error_info)) - ] # Triggers the notification in each notifier walkthrough_notifiers(settings, fn notifier, options -> diff --git a/lib/boom_notifier/mail_notifier.ex b/lib/boom_notifier/mail_notifier.ex index d0c0a0b..242e240 100644 --- a/lib/boom_notifier/mail_notifier.ex +++ b/lib/boom_notifier/mail_notifier.ex @@ -14,8 +14,8 @@ defmodule BoomNotifier.MailNotifier do @doc """ Creates mail subject line from a subject prefix and error reason message. """ - @spec build_subject(String.t(), list(ErrorInfo.t()), non_neg_integer()) :: String.t() - def build_subject(prefix, [%ErrorInfo{reason: reason} | _], max_length) do + @spec build_subject(String.t(), ErrorInfo.t(), non_neg_integer()) :: String.t() + def build_subject(prefix, %ErrorInfo{reason: reason}, max_length) do String.slice("#{prefix}: #{reason}", 0..(max_length - 1)) end diff --git a/lib/boom_notifier/mail_notifier/bamboo.ex b/lib/boom_notifier/mail_notifier/bamboo.ex index 4ff18f8..7f5b768 100644 --- a/lib/boom_notifier/mail_notifier/bamboo.ex +++ b/lib/boom_notifier/mail_notifier/bamboo.ex @@ -37,7 +37,7 @@ if Code.ensure_loaded?(Bamboo) do defdelegate validate_config(options), to: MailNotifier @impl BoomNotifier.Notifier - @spec notify(list(ErrorInfo.t()), options) :: no_return() + @spec notify(ErrorInfo.t(), options) :: no_return() def notify(error_info, options) do subject = MailNotifier.build_subject( diff --git a/lib/boom_notifier/mail_notifier/html_content.ex b/lib/boom_notifier/mail_notifier/html_content.ex index 87b72af..f182f23 100644 --- a/lib/boom_notifier/mail_notifier/html_content.ex +++ b/lib/boom_notifier/mail_notifier/html_content.ex @@ -8,13 +8,9 @@ defmodule BoomNotifier.MailNotifier.HTMLContent do :def, :email_body, Path.join([Path.dirname(__ENV__.file), "templates", "email_body.html.eex"]), - [:errors] + [:error] ) - def build(errors) when is_list(errors) do - email_body(Enum.map(errors, &build/1)) - end - def build(%ErrorInfo{ name: name, controller: controller, @@ -38,6 +34,7 @@ defmodule BoomNotifier.MailNotifier.HTMLContent do metadata: metadata, reason: reason } + |> email_body() end defp format_timestamp(timestamp) do diff --git a/lib/boom_notifier/mail_notifier/swoosh.ex b/lib/boom_notifier/mail_notifier/swoosh.ex index c723922..b967f84 100644 --- a/lib/boom_notifier/mail_notifier/swoosh.ex +++ b/lib/boom_notifier/mail_notifier/swoosh.ex @@ -37,7 +37,7 @@ if Code.ensure_loaded?(Swoosh) do defdelegate validate_config(options), to: MailNotifier @impl BoomNotifier.Notifier - @spec notify(list(ErrorInfo.t()), options) :: no_return() + @spec notify(ErrorInfo.t(), options) :: no_return() def notify(error_info, options) do # Note, unlike Bamboo, Swoosh will raise while creating the mail if it is # invalid (has a bad recipient, etc). diff --git a/lib/boom_notifier/mail_notifier/templates/email_body.html.eex b/lib/boom_notifier/mail_notifier/templates/email_body.html.eex index 41b68d4..09aacea 100644 --- a/lib/boom_notifier/mail_notifier/templates/email_body.html.eex +++ b/lib/boom_notifier/mail_notifier/templates/email_body.html.eex @@ -71,74 +71,69 @@
- <%= for error <- errors do %> - - -

Message:

-

- <%= error.exception_summary %> -

- - + + +

Message:

+

+ <%= error.exception_summary %> +

+ + + + +

+ Request Information: +

+ <%= if error.request do %> +
    +
  • URL: <%= error.request.url %>
    • +
  • Path: <%= error.request.path %>
    • +
  • Method: <%= error.request.method %>
    • +
  • Port: <%= error.request.port %>
    • +
  • Scheme: <%= error.request.scheme %>
    • +
  • Query String: <%= error.request.query_string %>
    • +
  • Client IP: <%= error.request.client_ip %>
    • +
  • Occurred on: <%= error.timestamp %>
    • +
+ <% end %> + + + <%= if error.metadata do %>

- Request Information: + Metadata:

- <%= if error.request do %> -
    -
  • URL: <%= error.request.url %>
    • -
  • Path: <%= error.request.path %>
    • -
  • Method: <%= error.request.method %>
    • -
  • Port: <%= error.request.port %>
    • -
  • Scheme: <%= error.request.scheme %>
    • -
  • Query String: <%= error.request.query_string %>
    • -
  • Client IP: <%= error.request.client_ip %>
    • -
  • Occurred on: <%= error.timestamp %>
    • -
- <% end %> +
    + <%= for {source, fields} <- error.metadata do %> + <%= source %>: +
      + <%= for {k, v} <- fields do %> +
    • <%= k %>: <%= v %>
      • + <% end %> +
    + <% end %> +
- <%= if error.metadata do %> - - -

- Metadata: -

-
    - <%= for {source, fields} <- error.metadata do %> - <%= source %>: -
      - <%= for {k, v} <- fields do %> -
    • <%= k %>: <%= v %>
      • - <% end %> -
    - <% end %> -
- - - <% end %> + <% end %> + + +

Stacktrace:

+
    + <%= for entry <- error.exception_stack_entries do %> +
  • <%= entry %>
    • + <% end %> +
+ + + <%= if error.reason do %> -

Stacktrace:

-
    - <%= for entry <- error.exception_stack_entries do %> -
  • <%= entry %>
    • - <% end %> -
+

Reason:

+

<%= error.reason %>

- <%= if error.reason do %> - - -

Reason:

-

<%= error.reason %>

- - - <% end %> - -
- <% end %> diff --git a/lib/boom_notifier/mail_notifier/templates/email_body.text.eex b/lib/boom_notifier/mail_notifier/templates/email_body.text.eex index 45ccf50..987b2e0 100644 --- a/lib/boom_notifier/mail_notifier/templates/email_body.text.eex +++ b/lib/boom_notifier/mail_notifier/templates/email_body.text.eex @@ -1,34 +1,30 @@ -<%= for error <- errors do %> - <%= if error.exception_summary do %> - <%= error.exception_summary %> - <% end %> - <%= if error.request do %> - Request Information: - URL: <%= error.request.url %> - Path: <%= error.request.path %> - Method: <%= error.request.method %> - Port: <%= error.request.port %> - Scheme: <%= error.request.scheme %> - Query String: <%= error.request.query_string %> - Client IP: <%= error.request.client_ip %> - Occurred on: <%= error.timestamp %> - <% end %> - <%= if error.metadata do %> - Metadata: - <%= for {source, fields} <- error.metadata do %> - <%= source %>: - <%= for {k, v} <- fields do %> - <%= k %>: <%= v %> - <% end %> - <% end %> - <% end %> - <%= for entry <- error.exception_stack_entries do %> - <%= entry %> - <% end %> - <%= if error.reason do %> - Reason: - <%= error.reason %> +<%= if error.exception_summary do %> + <%= error.exception_summary %> +<% end %> +<%= if error.request do %> + Request Information: + URL: <%= error.request.url %> + Path: <%= error.request.path %> + Method: <%= error.request.method %> + Port: <%= error.request.port %> + Scheme: <%= error.request.scheme %> + Query String: <%= error.request.query_string %> + Client IP: <%= error.request.client_ip %> + Occurred on: <%= error.timestamp %> +<% end %> +<%= if error.metadata do %> + Metadata: + <%= for {source, fields} <- error.metadata do %> + <%= source %>: + <%= for {k, v} <- fields do %> + <%= k %>: <%= v %> + <% end %> <% end %> - - ---------------------------------------- +<% end %> +<%= for entry <- error.exception_stack_entries do %> + <%= entry %> +<% end %> +<%= if error.reason do %> + Reason: + <%= error.reason %> <% end %> diff --git a/lib/boom_notifier/mail_notifier/text_content.ex b/lib/boom_notifier/mail_notifier/text_content.ex index 85304f9..30d783d 100644 --- a/lib/boom_notifier/mail_notifier/text_content.ex +++ b/lib/boom_notifier/mail_notifier/text_content.ex @@ -8,13 +8,9 @@ defmodule BoomNotifier.MailNotifier.TextContent do :def, :email_body, Path.join([Path.dirname(__ENV__.file), "templates", "email_body.text.eex"]), - [:errors] + [:error] ) - def build(errors) when is_list(errors) do - email_body(Enum.map(errors, &build/1)) - end - def build(%ErrorInfo{ name: name, controller: controller, @@ -38,6 +34,7 @@ defmodule BoomNotifier.MailNotifier.TextContent do metadata: metadata, reason: reason } + |> email_body() end defp format_timestamp(timestamp) do diff --git a/lib/boom_notifier/notifier.ex b/lib/boom_notifier/notifier.ex index b90b85c..2ea1d0a 100644 --- a/lib/boom_notifier/notifier.ex +++ b/lib/boom_notifier/notifier.ex @@ -3,7 +3,7 @@ defmodule BoomNotifier.Notifier do Defines a callback to be used by custom notifiers """ - @callback notify(list(%ErrorInfo{}), keyword(String.t())) :: no_return() + @callback notify(ErrorInfo.t(), keyword(String.t())) :: no_return() @callback validate_config(keyword(String.t())) :: :ok | {:error, String.t()} @optional_callbacks validate_config: 1 end diff --git a/lib/boom_notifier/webhook_notifier.ex b/lib/boom_notifier/webhook_notifier.ex index 3bc2b51..f85ee5d 100644 --- a/lib/boom_notifier/webhook_notifier.ex +++ b/lib/boom_notifier/webhook_notifier.ex @@ -35,11 +35,11 @@ defmodule BoomNotifier.WebhookNotifier do end @impl BoomNotifier.Notifier - @spec notify(list(ErrorInfo.t()), options) :: no_return() - def notify(errors_info, options) do + @spec notify(ErrorInfo.t(), options) :: no_return() + def notify(error_info, options) do payload = - errors_info - |> format_errors() + error_info + |> format_error() |> Jason.encode!() headers = @@ -50,10 +50,6 @@ defmodule BoomNotifier.WebhookNotifier do HTTPoison.post!(options[:url], payload, headers) end - defp format_errors(errors) when is_list(errors) do - Enum.map(errors, &format_error/1) - end - defp format_error(%ErrorInfo{ name: name, controller: controller, diff --git a/test/mailer_notifier_test.exs b/test/mailer_notifier_test.exs index 4294df1..c532683 100644 --- a/test/mailer_notifier_test.exs +++ b/test/mailer_notifier_test.exs @@ -238,12 +238,11 @@ defmodule MailerNotifierTest do receive do {:email_text_body, body} -> - reason_lines = Enum.take(body, -3) + reason_lines = Enum.take(body, -2) assert [ "Reason:", - String.duplicate("a", 300), - "----------------------------------------" + String.duplicate("a", 300) ] == reason_lines end end diff --git a/test/notifier_test.exs b/test/notifier_test.exs index 5b9bc94..25a17ab 100644 --- a/test/notifier_test.exs +++ b/test/notifier_test.exs @@ -12,18 +12,12 @@ defmodule NotifierTest do @behaviour BoomNotifier.Notifier @impl BoomNotifier.Notifier - def notify(error_info_list, options) do - [first_error | _] = error_info_list - + def notify(error_info, options) do subject_prefix = Keyword.get(options, :subject) to_respond_pid = Keyword.get(options, :sender_pid) - subject = "#{subject_prefix}: #{first_error.reason}" - - body = - Enum.map(error_info_list, fn error_info -> - Enum.map(error_info.stack, &(Exception.format_stacktrace_entry(&1) <> "\n")) - end) + subject = "#{subject_prefix}: #{error_info.reason}" + body = Enum.map(error_info.stack, &(Exception.format_stacktrace_entry(&1) <> "\n")) send(to_respond_pid, %{exception: %{subject: subject, body: body}}) end @@ -183,12 +177,10 @@ defmodule NotifierTest do exception: %{ subject: "BOOM error caught: booom!", body: [ - [ - "test/notifier_test.exs:" <> - <<_name::binary-size(2), - ": NotifierTest.PlugErrorWithSingleNotifier.\"call \(overridable 1\)\"/2\n">> - | _ - ] + "test/notifier_test.exs:" <> + <<_name::binary-size(2), + ": NotifierTest.PlugErrorWithSingleNotifier.\"call \(overridable 1\)\"/2\n">> + | _ ] } }, @@ -205,12 +197,10 @@ defmodule NotifierTest do exception: %{ subject: "BOOM error caught: booom!", body: [ - [ - "test/notifier_test.exs:" <> - <<_name::binary-size(2), - ": NotifierTest.PlugErrorWithMultipleNotifiers.\"call \(overridable 1\)\"/2\n">> - | _ - ] + "test/notifier_test.exs:" <> + <<_name::binary-size(2), + ": NotifierTest.PlugErrorWithMultipleNotifiers.\"call \(overridable 1\)\"/2\n">> + | _ ] } }, diff --git a/test/webhook_notifier_test.exs b/test/webhook_notifier_test.exs index f9d19aa..839b8c2 100644 --- a/test/webhook_notifier_test.exs +++ b/test/webhook_notifier_test.exs @@ -98,15 +98,13 @@ defmodule WebhookNotifierTest do {:ok, body, _conn} = Plug.Conn.read_body(conn) - [ - %{ - exception_summary: exception_summary, - exception_stack_entries: [first_stack_entry | _] = exception_stack_entries, - request: request, - timestamp: timestamp, - metadata: metadata - } - ] = Jason.decode!(body, keys: :atoms) + %{ + exception_summary: exception_summary, + exception_stack_entries: [first_stack_entry | _] = exception_stack_entries, + request: request, + timestamp: timestamp, + metadata: metadata + } = Jason.decode!(body, keys: :atoms) assert exception_summary == @expected_response.exception_summary From eb0ae7a5b70b1ae86d1d18df7338e16ef92596d4 Mon Sep 17 00:00:00 2001 From: Ignacio Aguirrezabal Date: Thu, 7 Apr 2022 10:44:54 -0300 Subject: [PATCH 4/8] Show error stats in notifiers --- example_app/test/send_notification_test.exs | 66 +++++++++++-------- .../mail_notifier/html_content.ex | 6 +- .../templates/email_body.html.eex | 15 ++++- .../templates/email_body.text.eex | 7 ++ .../mail_notifier/text_content.ex | 6 +- lib/boom_notifier/webhook_notifier.ex | 12 +++- 6 files changed, 77 insertions(+), 35 deletions(-) diff --git a/example_app/test/send_notification_test.exs b/example_app/test/send_notification_test.exs index b9a0a69..67cec29 100644 --- a/example_app/test/send_notification_test.exs +++ b/example_app/test/send_notification_test.exs @@ -58,40 +58,50 @@ defmodule ExampleAppWeb.SendNotificationTest do end feature "Sends notification in groups", %{session: session} do - for %{expected_notifications: expected_notifications, expected_emails: expected_emails} <- + for %{accumulated_occurrences: accumulated_occurrences, expected_emails: expected_emails} <- [ - %{expected_notifications: 1, expected_emails: 1}, - %{expected_notifications: 1, expected_emails: 1}, - %{expected_notifications: 2, expected_emails: 2}, - %{expected_notifications: 2, expected_emails: 2}, - %{expected_notifications: 2, expected_emails: 2}, - %{expected_notifications: 2, expected_emails: 2}, - %{expected_notifications: 4, expected_emails: 3}, - %{expected_notifications: 4, expected_emails: 3}, - %{expected_notifications: 4, expected_emails: 3}, - %{expected_notifications: 4, expected_emails: 3}, - %{expected_notifications: 4, expected_emails: 3}, - %{expected_notifications: 4, expected_emails: 3}, - %{expected_notifications: 4, expected_emails: 3}, - %{expected_notifications: 4, expected_emails: 3}, - %{expected_notifications: 8, expected_emails: 4}, - %{expected_notifications: 8, expected_emails: 4}, - %{expected_notifications: 8, expected_emails: 4}, - %{expected_notifications: 8, expected_emails: 4}, - %{expected_notifications: 8, expected_emails: 4}, - %{expected_notifications: 8, expected_emails: 4}, - %{expected_notifications: 8, expected_emails: 4}, - %{expected_notifications: 8, expected_emails: 4}, - %{expected_notifications: 8, expected_emails: 5} + %{accumulated_occurrences: 1, expected_emails: 1}, + %{accumulated_occurrences: 1, expected_emails: 1}, + %{accumulated_occurrences: 2, expected_emails: 2}, + %{accumulated_occurrences: 2, expected_emails: 2}, + %{accumulated_occurrences: 2, expected_emails: 2}, + %{accumulated_occurrences: 2, expected_emails: 2}, + %{accumulated_occurrences: 4, expected_emails: 3}, + %{accumulated_occurrences: 4, expected_emails: 3}, + %{accumulated_occurrences: 4, expected_emails: 3}, + %{accumulated_occurrences: 4, expected_emails: 3}, + %{accumulated_occurrences: 4, expected_emails: 3}, + %{accumulated_occurrences: 4, expected_emails: 3}, + %{accumulated_occurrences: 4, expected_emails: 3}, + %{accumulated_occurrences: 4, expected_emails: 3}, + %{accumulated_occurrences: 8, expected_emails: 4}, + %{accumulated_occurrences: 8, expected_emails: 4}, + %{accumulated_occurrences: 8, expected_emails: 4}, + %{accumulated_occurrences: 8, expected_emails: 4}, + %{accumulated_occurrences: 8, expected_emails: 4}, + %{accumulated_occurrences: 8, expected_emails: 4}, + %{accumulated_occurrences: 8, expected_emails: 4}, + %{accumulated_occurrences: 8, expected_emails: 4}, + %{accumulated_occurrences: 8, expected_emails: 5} ] do session |> visit("/group-exception") |> assert_text("GroupExceptionError at GET /group-exception") - session - |> visit("/mailbox") - |> find(Query.css(".list-group")) - |> find(Query.css(".list-group-item", count: expected_emails)) + items = + session + |> visit("/mailbox") + |> find(Query.css(".list-group")) + |> find(Query.css(".list-group-item", count: expected_emails)) + + if expected_emails > 1 do + item = List.first(items) + email = select_email(session, item) + + email + |> find(Query.css(".body-text")) + |> assert_text("Errors: #{accumulated_occurrences}") + end end end diff --git a/lib/boom_notifier/mail_notifier/html_content.ex b/lib/boom_notifier/mail_notifier/html_content.ex index f182f23..901e379 100644 --- a/lib/boom_notifier/mail_notifier/html_content.ex +++ b/lib/boom_notifier/mail_notifier/html_content.ex @@ -19,7 +19,8 @@ defmodule BoomNotifier.MailNotifier.HTMLContent do stack: stack, timestamp: timestamp, metadata: metadata, - reason: reason + reason: reason, + occurrences: occurrences }) do exception_summary = if controller && action do @@ -32,7 +33,8 @@ defmodule BoomNotifier.MailNotifier.HTMLContent do exception_stack_entries: Enum.map(stack, &Exception.format_stacktrace_entry/1), timestamp: format_timestamp(timestamp), metadata: metadata, - reason: reason + reason: reason, + occurrences: occurrences } |> email_body() end diff --git a/lib/boom_notifier/mail_notifier/templates/email_body.html.eex b/lib/boom_notifier/mail_notifier/templates/email_body.html.eex index 09aacea..3f53a21 100644 --- a/lib/boom_notifier/mail_notifier/templates/email_body.html.eex +++ b/lib/boom_notifier/mail_notifier/templates/email_body.html.eex @@ -131,7 +131,20 @@

Reason:

-

<%= error.reason %>

+

<%= error.reason %>

+ + + <% end %> + + <%= if error.occurrences.accumulated_occurrences > 1 do %> + + +

Occurrences:

+
    +
  • Errors: <%= error.occurrences.accumulated_occurrences %>
    • +
  • First occurrence: <%= error.occurrences.first_occurrence %>
    • +
  • Last occurrence: <%= error.occurrences.last_occurrence %>
    • +
<% end %> diff --git a/lib/boom_notifier/mail_notifier/templates/email_body.text.eex b/lib/boom_notifier/mail_notifier/templates/email_body.text.eex index 987b2e0..fbb0fe2 100644 --- a/lib/boom_notifier/mail_notifier/templates/email_body.text.eex +++ b/lib/boom_notifier/mail_notifier/templates/email_body.text.eex @@ -28,3 +28,10 @@ Reason: <%= error.reason %> <% end %> + +<%= if error.occurrences.accumulated_occurrences > 1 do %> + Occurrences: + Errors: <%= error.occurrences.accumulated_occurrences %> + First occurrence: <%= error.occurrences.first_occurrence %> + Last occurrence: <%= error.occurrences.last_occurrence %> +<% end %> diff --git a/lib/boom_notifier/mail_notifier/text_content.ex b/lib/boom_notifier/mail_notifier/text_content.ex index 30d783d..a9fe113 100644 --- a/lib/boom_notifier/mail_notifier/text_content.ex +++ b/lib/boom_notifier/mail_notifier/text_content.ex @@ -19,7 +19,8 @@ defmodule BoomNotifier.MailNotifier.TextContent do stack: stack, timestamp: timestamp, metadata: metadata, - reason: reason + reason: reason, + occurrences: occurrences }) do exception_summary = if controller && action do @@ -32,7 +33,8 @@ defmodule BoomNotifier.MailNotifier.TextContent do exception_stack_entries: Enum.map(stack, &Exception.format_stacktrace_entry/1), timestamp: format_timestamp(timestamp), metadata: metadata, - reason: reason + reason: reason, + occurrences: occurrences } |> email_body() end diff --git a/lib/boom_notifier/webhook_notifier.ex b/lib/boom_notifier/webhook_notifier.ex index f85ee5d..e459adf 100644 --- a/lib/boom_notifier/webhook_notifier.ex +++ b/lib/boom_notifier/webhook_notifier.ex @@ -1,3 +1,9 @@ +require Protocol + +Protocol.derive(Jason.Encoder, BoomNotifier.ErrorStorage, + only: [:accumulated_occurrences, :first_occurrence, :last_occurrence] +) + defmodule BoomNotifier.WebhookNotifier do @moduledoc """ Send exception notification as a json using `HTTPoison`. @@ -57,7 +63,8 @@ defmodule BoomNotifier.WebhookNotifier do request: request, stack: stack, timestamp: timestamp, - metadata: metadata + metadata: metadata, + occurrences: occurrences }) do exception_summary = if controller && action do @@ -69,7 +76,8 @@ defmodule BoomNotifier.WebhookNotifier do request: request, exception_stack_entries: Enum.map(stack, &Exception.format_stacktrace_entry/1), timestamp: DateTime.to_iso8601(timestamp), - metadata: metadata + metadata: metadata, + occurrences: occurrences } end end From da24a160e9a9f898596510f56ab93ace051e94bd Mon Sep 17 00:00:00 2001 From: Ignacio Aguirrezabal Date: Thu, 7 Apr 2022 13:00:24 -0300 Subject: [PATCH 5/8] Rename `ErrorInfo` module to `BoomNotifier.ErrorInfo` --- lib/boom_notifier.ex | 1 + lib/boom_notifier/error_info.ex | 8 +++--- lib/boom_notifier/error_storage.ex | 2 ++ lib/boom_notifier/mail_notifier.ex | 2 ++ lib/boom_notifier/mail_notifier/bamboo.ex | 1 + .../mail_notifier/html_content.ex | 1 + lib/boom_notifier/mail_notifier/swoosh.ex | 1 + .../mail_notifier/text_content.ex | 1 + lib/boom_notifier/notifier.ex | 2 +- lib/boom_notifier/webhook_notifier.ex | 1 + test/error_info_test.exs | 3 +- test/error_storage_test.exs | 28 ++++++++++--------- 12 files changed, 32 insertions(+), 19 deletions(-) diff --git a/lib/boom_notifier.ex b/lib/boom_notifier.ex index 1d61b37..730c8ed 100644 --- a/lib/boom_notifier.ex +++ b/lib/boom_notifier.ex @@ -4,6 +4,7 @@ defmodule BoomNotifier do # Responsible for sending a notification to each notifier every time an # exception is raised. + alias BoomNotifier.ErrorInfo alias BoomNotifier.ErrorStorage alias BoomNotifier.NotifierSenderServer require Logger diff --git a/lib/boom_notifier/error_info.ex b/lib/boom_notifier/error_info.ex index 4b47b02..4a4593e 100644 --- a/lib/boom_notifier/error_info.ex +++ b/lib/boom_notifier/error_info.ex @@ -1,4 +1,4 @@ -defmodule ErrorInfo do +defmodule BoomNotifier.ErrorInfo do @moduledoc false # The ErrorInfo struct holds all the information about the exception. @@ -19,7 +19,7 @@ defmodule ErrorInfo do :occurrences ] - @type t :: %ErrorInfo{} + @type t :: %__MODULE__{} @type option :: :logger @@ -36,11 +36,11 @@ defmodule ErrorInfo do }, map(), custom_data_strategy_type - ) :: {atom(), ErrorInfo.t()} + ) :: {atom(), __MODULE__.t()} def build(%{reason: reason, stack: stack} = error, conn, custom_data_strategy) do {error_reason, error_name} = error_reason(reason) - error_info = %ErrorInfo{ + error_info = %__MODULE__{ reason: error_reason, stack: stack, controller: get_in(conn.private, [:phoenix_controller]), diff --git a/lib/boom_notifier/error_storage.ex b/lib/boom_notifier/error_storage.ex index 80d839c..b67b5c6 100644 --- a/lib/boom_notifier/error_storage.ex +++ b/lib/boom_notifier/error_storage.ex @@ -4,6 +4,8 @@ defmodule BoomNotifier.ErrorStorage do # Keeps track of the errors grouped by type and a counter so the notifier # knows the next time it should be executed + alias BoomNotifier.ErrorInfo + @enforce_keys [:accumulated_occurrences, :first_occurrence, :last_occurrence] defstruct [ :accumulated_occurrences, diff --git a/lib/boom_notifier/mail_notifier.ex b/lib/boom_notifier/mail_notifier.ex index 242e240..b223582 100644 --- a/lib/boom_notifier/mail_notifier.ex +++ b/lib/boom_notifier/mail_notifier.ex @@ -1,6 +1,8 @@ defmodule BoomNotifier.MailNotifier do @moduledoc false + alias BoomNotifier.ErrorInfo + @doc """ Checks given mail notifier config contains all required keys. """ diff --git a/lib/boom_notifier/mail_notifier/bamboo.ex b/lib/boom_notifier/mail_notifier/bamboo.ex index 7f5b768..d480d22 100644 --- a/lib/boom_notifier/mail_notifier/bamboo.ex +++ b/lib/boom_notifier/mail_notifier/bamboo.ex @@ -25,6 +25,7 @@ if Code.ensure_loaded?(Bamboo) do import Bamboo.Email + alias BoomNotifier.ErrorInfo alias BoomNotifier.MailNotifier alias BoomNotifier.MailNotifier.HTMLContent alias BoomNotifier.MailNotifier.TextContent diff --git a/lib/boom_notifier/mail_notifier/html_content.ex b/lib/boom_notifier/mail_notifier/html_content.ex index 901e379..9c84d3d 100644 --- a/lib/boom_notifier/mail_notifier/html_content.ex +++ b/lib/boom_notifier/mail_notifier/html_content.ex @@ -2,6 +2,7 @@ defmodule BoomNotifier.MailNotifier.HTMLContent do @moduledoc false import BoomNotifier.Helpers + alias BoomNotifier.ErrorInfo require EEx EEx.function_from_file( diff --git a/lib/boom_notifier/mail_notifier/swoosh.ex b/lib/boom_notifier/mail_notifier/swoosh.ex index b967f84..7de9676 100644 --- a/lib/boom_notifier/mail_notifier/swoosh.ex +++ b/lib/boom_notifier/mail_notifier/swoosh.ex @@ -25,6 +25,7 @@ if Code.ensure_loaded?(Swoosh) do import Swoosh.Email + alias BoomNotifier.ErrorInfo alias BoomNotifier.MailNotifier alias BoomNotifier.MailNotifier.HTMLContent alias BoomNotifier.MailNotifier.TextContent diff --git a/lib/boom_notifier/mail_notifier/text_content.ex b/lib/boom_notifier/mail_notifier/text_content.ex index a9fe113..198801d 100644 --- a/lib/boom_notifier/mail_notifier/text_content.ex +++ b/lib/boom_notifier/mail_notifier/text_content.ex @@ -2,6 +2,7 @@ defmodule BoomNotifier.MailNotifier.TextContent do @moduledoc false import BoomNotifier.Helpers + alias BoomNotifier.ErrorInfo require EEx EEx.function_from_file( diff --git a/lib/boom_notifier/notifier.ex b/lib/boom_notifier/notifier.ex index 2ea1d0a..111ce38 100644 --- a/lib/boom_notifier/notifier.ex +++ b/lib/boom_notifier/notifier.ex @@ -3,7 +3,7 @@ defmodule BoomNotifier.Notifier do Defines a callback to be used by custom notifiers """ - @callback notify(ErrorInfo.t(), keyword(String.t())) :: no_return() + @callback notify(BoomNotifier.ErrorInfo.t(), keyword(String.t())) :: no_return() @callback validate_config(keyword(String.t())) :: :ok | {:error, String.t()} @optional_callbacks validate_config: 1 end diff --git a/lib/boom_notifier/webhook_notifier.ex b/lib/boom_notifier/webhook_notifier.ex index e459adf..6def6da 100644 --- a/lib/boom_notifier/webhook_notifier.ex +++ b/lib/boom_notifier/webhook_notifier.ex @@ -27,6 +27,7 @@ defmodule BoomNotifier.WebhookNotifier do @behaviour BoomNotifier.Notifier import BoomNotifier.Helpers + alias BoomNotifier.ErrorInfo @type options :: [{:url, String.t()}] | [{:url, String.t()}, {:headers, HTTPoison.Request.headers()}] diff --git a/test/error_info_test.exs b/test/error_info_test.exs index d7a5307..9881257 100644 --- a/test/error_info_test.exs +++ b/test/error_info_test.exs @@ -3,6 +3,7 @@ defmodule ErrorInfoTest do import Plug.Conn import Phoenix.ConnTest + alias BoomNotifier.ErrorInfo defmodule TestController do use Phoenix.Controller @@ -164,7 +165,7 @@ defmodule ErrorInfoTest do } = hd(error_info_stack) assert 'test/error_info_test.exs' = Keyword.fetch!(error_info, :file) - assert 16 = Keyword.fetch!(error_info, :line) + assert 17 = Keyword.fetch!(error_info, :line) assert { ExUnit.Runner, diff --git a/test/error_storage_test.exs b/test/error_storage_test.exs index afe8636..d7acf2c 100644 --- a/test/error_storage_test.exs +++ b/test/error_storage_test.exs @@ -1,5 +1,7 @@ defmodule ErrorStorageTest do use ExUnit.Case, async: true + + alias BoomNotifier.ErrorInfo alias BoomNotifier.ErrorStorage @timestamp DateTime.utc_now() @@ -39,16 +41,16 @@ defmodule ErrorStorageTest do assert error_stat_1 == %ErrorStorage{ __max_storage_capacity__: 1, - accumulated_occurrences: 2, - first_occurrence: @timestamp, - last_occurrence: @timestamp + accumulated_occurrences: 1, + first_occurrence: another_timestamp, + last_occurrence: another_timestamp } assert error_stat_2 == %ErrorStorage{ __max_storage_capacity__: 1, - accumulated_occurrences: 1, - first_occurrence: another_timestamp, - last_occurrence: another_timestamp + accumulated_occurrences: 2, + first_occurrence: @timestamp, + last_occurrence: @timestamp } end end @@ -242,18 +244,18 @@ defmodule ErrorStorageTest do Agent.get(:boom_notifier, fn state -> state end) |> Map.values() assert error_stat_1 == %ErrorStorage{ - __max_storage_capacity__: 2, - accumulated_occurrences: 0, - first_occurrence: nil, - last_occurrence: nil - } - - assert error_stat_2 == %ErrorStorage{ __max_storage_capacity__: 1, accumulated_occurrences: 1, first_occurrence: @timestamp, last_occurrence: @timestamp } + + assert error_stat_2 == %ErrorStorage{ + __max_storage_capacity__: 2, + accumulated_occurrences: 0, + first_occurrence: nil, + last_occurrence: nil + } end end end From 9675d5f04b2f92a7e91b8e15dc13eef6f6198ca1 Mon Sep 17 00:00:00 2001 From: Ignacio Aguirrezabal Date: Thu, 7 Apr 2022 13:34:49 -0300 Subject: [PATCH 6/8] Remove unused `error_kind` from `ErrorInfo` --- lib/boom_notifier.ex | 2 +- lib/boom_notifier/error_info.ex | 12 +++------- test/error_info_test.exs | 41 +++++++++++++++------------------ 3 files changed, 22 insertions(+), 33 deletions(-) diff --git a/lib/boom_notifier.ex b/lib/boom_notifier.ex index 730c8ed..486e429 100644 --- a/lib/boom_notifier.ex +++ b/lib/boom_notifier.ex @@ -86,7 +86,7 @@ defmodule BoomNotifier do defp do_notify_error(conn, settings, error) do {custom_data, _settings} = Keyword.pop(settings, :custom_data, :nothing) - {error_kind, error_info} = ErrorInfo.build(error, conn, custom_data) + error_info = ErrorInfo.build(error, conn, custom_data) ErrorStorage.store_error(error_info) diff --git a/lib/boom_notifier/error_info.ex b/lib/boom_notifier/error_info.ex index 4a4593e..c6fb789 100644 --- a/lib/boom_notifier/error_info.ex +++ b/lib/boom_notifier/error_info.ex @@ -36,11 +36,11 @@ defmodule BoomNotifier.ErrorInfo do }, map(), custom_data_strategy_type - ) :: {atom(), __MODULE__.t()} - def build(%{reason: reason, stack: stack} = error, conn, custom_data_strategy) do + ) :: __MODULE__.t() + def build(%{reason: reason, stack: stack}, conn, custom_data_strategy) do {error_reason, error_name} = error_reason(reason) - error_info = %__MODULE__{ + %__MODULE__{ reason: error_reason, stack: stack, controller: get_in(conn.private, [:phoenix_controller]), @@ -50,8 +50,6 @@ defmodule BoomNotifier.ErrorInfo do name: error_name, metadata: build_custom_data(conn, custom_data_strategy) } - - {error_type(error), error_info} end defp error_reason(%name{message: reason}), do: {reason, name} @@ -59,10 +57,6 @@ defmodule BoomNotifier.ErrorInfo do defp error_reason(reason) when is_binary(reason), do: error_reason(%{message: reason}) defp error_reason(reason), do: error_reason(%{message: inspect(reason)}) - defp error_type(%{reason: %name{}}), do: name - defp error_type(%{error: %{kind: kind}}), do: kind - defp error_type(_), do: :error - defp build_request_info(conn) do %{ path: conn.request_path, diff --git a/test/error_info_test.exs b/test/error_info_test.exs index 9881257..5bb6aa8 100644 --- a/test/error_info_test.exs +++ b/test/error_info_test.exs @@ -77,49 +77,44 @@ defmodule ErrorInfoTest do test "Generic error without exception name" do %Plug.Conn.WrapperError{conn: conn} = catch_error(get(build_conn(), :index)) error = %{reason: "Boom", stack: []} - {error_kind, %ErrorInfo{name: name, reason: reason}} = ErrorInfo.build(error, conn, :nothing) + %ErrorInfo{name: name, reason: reason} = ErrorInfo.build(error, conn, :nothing) assert "Error" = name assert "Boom" = reason - assert :error = error_kind end test "Error without exception name but message" do %Plug.Conn.WrapperError{conn: conn} = catch_error(get(build_conn(), :index)) error = %{reason: %{message: "Boom"}, stack: []} - {error_kind, %ErrorInfo{name: name, reason: reason}} = ErrorInfo.build(error, conn, :nothing) + %ErrorInfo{name: name, reason: reason} = ErrorInfo.build(error, conn, :nothing) assert "Error" = name assert "Boom" = reason - assert :error = error_kind end test "Error with exception name" do %Plug.Conn.WrapperError{conn: conn} = catch_error(get(build_conn(), :index)) error = %{reason: %TestException{message: "Boom"}, stack: []} - - {error_kind, %ErrorInfo{name: name, reason: reason}} = ErrorInfo.build(error, conn, :nothing) + %ErrorInfo{name: name, reason: reason} = ErrorInfo.build(error, conn, :nothing) assert ErrorInfoTest.TestException = name assert "Boom" = reason - assert ErrorInfoTest.TestException = error_kind end test "Error without exception reason but error and kind" do %Plug.Conn.WrapperError{conn: conn} = catch_error(get(build_conn(), :index)) error = %{error: %{kind: :error_kind}, reason: %{message: "Boom"}, stack: []} - {error_kind, %ErrorInfo{name: name, reason: reason}} = ErrorInfo.build(error, conn, :nothing) + %ErrorInfo{name: name, reason: reason} = ErrorInfo.build(error, conn, :nothing) assert "Error" = name assert "Boom" = reason - assert :error_kind = error_kind end test "Error info includes action" do %Plug.Conn.WrapperError{conn: conn} = catch_error(get(build_conn(), :index)) - {_error_kind, %ErrorInfo{action: action}} = + %ErrorInfo{action: action} = ErrorInfo.build(%{reason: %TestException{message: "Boom"}, stack: []}, conn, :nothing) assert :index = action @@ -128,7 +123,7 @@ defmodule ErrorInfoTest do test "Error info includes controller" do %Plug.Conn.WrapperError{conn: conn} = catch_error(get(build_conn(), :index)) - {_error_kind, %ErrorInfo{controller: controller}} = + %ErrorInfo{controller: controller} = ErrorInfo.build(%{reason: %TestException{message: "Boom"}, stack: []}, conn, :nothing) assert TestController = controller @@ -137,7 +132,7 @@ defmodule ErrorInfoTest do test "Error info includes request info" do %Plug.Conn.WrapperError{conn: conn} = catch_error(post(build_conn(), "/create?foo=bar")) - {_error_kind, %ErrorInfo{request: request}} = + %ErrorInfo{request: request} = ErrorInfo.build(%{reason: %TestException{message: "Boom"}, stack: []}, conn, :nothing) assert %{ @@ -154,7 +149,7 @@ defmodule ErrorInfoTest do test "Error info includes stacktrace" do %Plug.Conn.WrapperError{conn: conn, stack: stack} = catch_error(get(build_conn(), :index)) - {_error_kind, %ErrorInfo{stack: error_info_stack}} = + %ErrorInfo{stack: error_info_stack} = ErrorInfo.build(%{reason: %TestException{message: "Boom"}, stack: stack}, conn, :nothing) assert { @@ -181,7 +176,7 @@ defmodule ErrorInfoTest do %Plug.Conn.WrapperError{conn: conn, stack: stack} = catch_error(get(build_conn(), "nil_access")) - {_error_kind, %ErrorInfo{stack: error_info_stack}} = + %ErrorInfo{stack: error_info_stack} = ErrorInfo.build(%{reason: %TestException{message: "Boom"}, stack: stack}, conn, :nothing) assert {nil, :name, [], []} = hd(error_info_stack) @@ -199,7 +194,7 @@ defmodule ErrorInfoTest do test "Error info includes timestamp" do %Plug.Conn.WrapperError{conn: conn, stack: stack} = catch_error(get(build_conn(), :index)) - {_error_kind, %ErrorInfo{timestamp: timestamp}} = + %ErrorInfo{timestamp: timestamp} = ErrorInfo.build(%{reason: %TestException{message: "Boom"}, stack: stack}, conn, :nothing) assert DateTime.diff(DateTime.utc_now(), timestamp, :second) <= 1 @@ -208,7 +203,7 @@ defmodule ErrorInfoTest do test "Error info metadata is nil when strategy is :nothing" do %Plug.Conn.WrapperError{conn: conn, stack: stack} = catch_error(get(build_conn(), :index)) - {_error_kind, %ErrorInfo{metadata: metadata}} = + %ErrorInfo{metadata: metadata} = ErrorInfo.build(%{reason: %TestException{message: "Boom"}, stack: stack}, conn, :nothing) assert nil == metadata @@ -217,7 +212,7 @@ defmodule ErrorInfoTest do test "Error info metadata includes assigns" do %Plug.Conn.WrapperError{conn: conn, stack: stack} = catch_error(get(build_conn(), :index)) - {_error_kind, %ErrorInfo{metadata: metadata}} = + %ErrorInfo{metadata: metadata} = ErrorInfo.build(%{reason: %TestException{message: "Boom"}, stack: stack}, conn, :assigns) assert %{assigns: %{age: 32, name: "Davis"}} = metadata @@ -226,7 +221,7 @@ defmodule ErrorInfoTest do test "Error info metadata includes filtered fields for assigns" do %Plug.Conn.WrapperError{conn: conn, stack: stack} = catch_error(get(build_conn(), :index)) - {_error_kind, %ErrorInfo{metadata: metadata}} = + %ErrorInfo{metadata: metadata} = ErrorInfo.build(%{reason: %TestException{message: "Boom"}, stack: stack}, conn, assigns: [fields: [:name]] ) @@ -237,7 +232,7 @@ defmodule ErrorInfoTest do test "Error info metadata includes logger" do %Plug.Conn.WrapperError{conn: conn, stack: stack} = catch_error(get(build_conn(), :index)) - {_error_kind, %ErrorInfo{metadata: metadata}} = + %ErrorInfo{metadata: metadata} = ErrorInfo.build(%{reason: %TestException{message: "Boom"}, stack: stack}, conn, :logger) assert %{logger: %{age: 17, name: "Dennis"}} = metadata @@ -246,7 +241,7 @@ defmodule ErrorInfoTest do test "Error info metadata includes filtered fields for logger" do %Plug.Conn.WrapperError{conn: conn, stack: stack} = catch_error(get(build_conn(), :index)) - {_error_kind, %ErrorInfo{metadata: metadata}} = + %ErrorInfo{metadata: metadata} = ErrorInfo.build(%{reason: %TestException{message: "Boom"}, stack: stack}, conn, logger: [fields: [:name]] ) @@ -257,7 +252,7 @@ defmodule ErrorInfoTest do test "Error info metadata includes assigns and logger" do %Plug.Conn.WrapperError{conn: conn, stack: stack} = catch_error(get(build_conn(), :index)) - {_error_kind, %ErrorInfo{metadata: metadata}} = + %ErrorInfo{metadata: metadata} = ErrorInfo.build(%{reason: %TestException{message: "Boom"}, stack: stack}, conn, [ :assigns, :logger @@ -272,7 +267,7 @@ defmodule ErrorInfoTest do test "Error info metadata includes filtered fields for assigns and logger" do %Plug.Conn.WrapperError{conn: conn, stack: stack} = catch_error(get(build_conn(), :index)) - {_error_kind, %ErrorInfo{metadata: metadata}} = + %ErrorInfo{metadata: metadata} = ErrorInfo.build(%{reason: %TestException{message: "Boom"}, stack: stack}, conn, [ [assigns: [fields: [:name]]], [logger: [fields: [:age]]] @@ -284,7 +279,7 @@ defmodule ErrorInfoTest do test "Error info metadata includes filtered equal fields for assigns and logger" do %Plug.Conn.WrapperError{conn: conn, stack: stack} = catch_error(get(build_conn(), :index)) - {_error_kind, %ErrorInfo{metadata: metadata}} = + %ErrorInfo{metadata: metadata} = ErrorInfo.build(%{reason: %TestException{message: "Boom"}, stack: stack}, conn, [ [assigns: [fields: [:name]]], [logger: [fields: [:name]]] From afd3bee94a3d7b73af855254aff02698973652b4 Mon Sep 17 00:00:00 2001 From: Ignacio Aguirrezabal Date: Mon, 11 Apr 2022 14:33:16 -0300 Subject: [PATCH 7/8] Remove useless if condition --- lib/boom_notifier/error_storage.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/boom_notifier/error_storage.ex b/lib/boom_notifier/error_storage.ex index b67b5c6..019ac4d 100644 --- a/lib/boom_notifier/error_storage.ex +++ b/lib/boom_notifier/error_storage.ex @@ -162,6 +162,6 @@ defmodule BoomNotifier.ErrorStorage do accumulated_occurrences = Map.get(error_storage_item, :accumulated_occurrences) max_storage_capacity = Map.get(error_storage_item, :__max_storage_capacity__) - if accumulated_occurrences >= max_storage_capacity, do: true, else: false + accumulated_occurrences >= max_storage_capacity end end From dfad7348bc8d3f848daeee3bef058c5ed21c85ce Mon Sep 17 00:00:00 2001 From: Ignacio Aguirrezabal Date: Mon, 11 Apr 2022 14:54:27 -0300 Subject: [PATCH 8/8] Update the README file --- README.md | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 0b249e2..cac4e39 100644 --- a/README.md +++ b/README.md @@ -102,15 +102,34 @@ defmodule YourApp.Router do To create a custom notifier, you need to implement the `BoomNotifier.Notifier` behaviour: ```elixir -@callback notify(list(%ErrorInfo{}), keyword(String.t())) :: no_return() +@callback notify(%ErrorInfo{}, keyword(String.t())) :: no_return() ``` +`ErrorInfo` is a struct that contains the following attributes: + +* `name`: the error name. +* `reason`: the error reason. +* `stack`: the error stacktrace. +* `controller`: the controller where the exception occurred. +* `action`: the action in the controller that failed. +* `request`: the request information that caused the exception. +* `timestamp`: the UTC time when the exception happened. +* `metatadata`: assigns and logger metadata. +* `occurrences`: aggregated information about the errors that are being + grouped. + * `accumulated_occurrences`: how many times an exception occurred before the + notification was sent. + * `first_occurrence`: collects the time when the first exception was raised. + * `last_occurrence`: collects the time when the last exception was raised. + +Then you can use that information in your `notify/2` implementation. + ```elixir defmodule CustomNotifier do @behaviour BoomNotifier.Notifier @impl BoomNotifier.Notifier - def notify(errors, options) do + def notify(error, options) do # ... # ... # ...