diff --git a/.gitignore b/.gitignore index 87e6320..9a5d073 100644 --- a/.gitignore +++ b/.gitignore @@ -23,4 +23,4 @@ erl_crash.dump boom-*.tar .DS_Store -priv/ +/priv/ diff --git a/README.md b/README.md index c19e399..0c70eee 100644 --- a/README.md +++ b/README.md @@ -149,6 +149,26 @@ defmodule YourApp.Router do ] ``` +### Notification trigger time limit + +If you've defined a triggering strategy which holds off notification delivering you can define a time limit value +which will be used to deliver the notification after a time limit milliseconds have passed from the last error. The +time counter is reset on new errors and only applies for cases where notifications are not sent. + +```elixir +defmodule YourApp.Router do + use Phoenix.Router + + use BoomNotifier, + notification_trigger: [:exponential], + time_limit: :timer.minutes(30), + notifier: CustomNotifier + + # ... +end +``` + + ## Custom data or Metadata By default, `BoomNotifier` will **not** include any custom data from your diff --git a/lib/boom_notifier.ex b/lib/boom_notifier.ex index db1d413..c5bdc40 100644 --- a/lib/boom_notifier.ex +++ b/lib/boom_notifier.ex @@ -3,29 +3,22 @@ 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 - def run_callback(settings, callback) do - missing_keys = Enum.reject([:notifier, :options], &Keyword.has_key?(settings, &1)) - - case missing_keys do - [] -> - callback.(settings[:notifier], settings[:options]) + alias BoomNotifier.ErrorInfo + alias BoomNotifier.NotificationSender - [missing_key] -> - Logger.error("(BoomNotifier) #{inspect(missing_key)} parameter is missing") + def notify_error(settings, conn, %{kind: :error, reason: %mod{}} = error) do + ignored_exceptions = Keyword.get(settings, :ignore_exceptions, []) - _ -> - Logger.error( - "(BoomNotifier) The following parameters are missing: #{inspect(missing_keys)}" - ) + unless Enum.member?(ignored_exceptions, mod) do + trigger_notify_error(settings, conn, error) end end + def notify_error(settings, conn, error), + do: trigger_notify_error(settings, conn, error) + def walkthrough_notifiers(settings, callback) do case Keyword.get(settings, :notifiers) do nil -> @@ -50,8 +43,32 @@ defmodule BoomNotifier do end end + defp run_callback(settings, callback) do + missing_keys = Enum.reject([:notifier, :options], &Keyword.has_key?(settings, &1)) + + case missing_keys do + [] -> + callback.(settings[:notifier], settings[:options]) + + [missing_key] -> + Logger.error("(BoomNotifier) #{inspect(missing_key)} parameter is missing") + + _ -> + Logger.error( + "(BoomNotifier) The following parameters are missing: #{inspect(missing_keys)}" + ) + end + end + + defp trigger_notify_error(settings, conn, error) do + custom_data = Keyword.get(settings, :custom_data, :nothing) + error_info = ErrorInfo.build(error, conn, custom_data) + + NotificationSender.async_trigger_notify(settings, error_info) + end + defmacro __using__(config) do - quote do + quote location: :keep, bind_quoted: [config: config] do import BoomNotifier error_handler_in_use = Plug.ErrorHandler in @behaviour @@ -68,44 +85,13 @@ defmodule BoomNotifier do end # Notifiers validation - walkthrough_notifiers( - unquote(config), - fn notifier, options -> validate_notifiers(notifier, options) end + BoomNotifier.walkthrough_notifiers( + config, + fn notifier, options -> BoomNotifier.validate_notifiers(notifier, options) end ) - def notify_error(conn, %{kind: :error, reason: %mod{}} = error) do - settings = unquote(config) - {ignored_exceptions, _settings} = Keyword.pop(settings, :ignore_exceptions, []) - - unless Enum.member?(ignored_exceptions, mod) do - do_notify_error(conn, settings, error) - end - end - def notify_error(conn, error) do - do_notify_error(conn, unquote(config), error) - end - - defp do_notify_error(conn, settings, error) do - {custom_data, _settings} = Keyword.pop(settings, :custom_data, :nothing) - error_info = ErrorInfo.build(error, conn, custom_data) - - ErrorStorage.store_error(error_info) - - 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, notification_data, options) - end) - - {notification_trigger, _settings} = - Keyword.pop(settings, :notification_trigger, :always) - - ErrorStorage.reset_accumulated_errors(notification_trigger, error_info) - end + BoomNotifier.notify_error(unquote(config), conn, error) end end end diff --git a/lib/boom_notifier/application.ex b/lib/boom_notifier/application.ex index 9916e9a..e77d2a1 100644 --- a/lib/boom_notifier/application.ex +++ b/lib/boom_notifier/application.ex @@ -6,7 +6,7 @@ defmodule BoomNotifier.Application do def start(_type, _args) do children = [ BoomNotifier.ErrorStorage, - BoomNotifier.NotifierSenderServer + BoomNotifier.NotificationSender ] Supervisor.start_link(children, strategy: :one_for_one) diff --git a/lib/boom_notifier/error_info.ex b/lib/boom_notifier/error_info.ex index ee54187..01a2fce 100644 --- a/lib/boom_notifier/error_info.ex +++ b/lib/boom_notifier/error_info.ex @@ -8,6 +8,7 @@ defmodule BoomNotifier.ErrorInfo do @enforce_keys [:reason, :stack, :timestamp] defstruct [ + :key, :name, :reason, :stack, @@ -34,13 +35,13 @@ defmodule BoomNotifier.ErrorInfo do required(:stack) => Exception.stacktrace(), optional(any()) => any() }, - map(), + Plug.Conn.t(), custom_data_strategy_type ) :: __MODULE__.t() def build(%{reason: reason, stack: stack}, conn, custom_data_strategy) do {error_reason, error_name} = error_reason(reason) - %__MODULE__{ + error_info = %__MODULE__{ reason: error_reason, stack: stack, controller: get_in(conn.private, [:phoenix_controller]), @@ -50,6 +51,8 @@ defmodule BoomNotifier.ErrorInfo do name: error_name, metadata: build_custom_data(conn, custom_data_strategy) } + + error_info |> Map.put(:key, generate_error_key(error_info)) end defp error_reason(%name{message: reason}), do: {reason, name} @@ -116,4 +119,20 @@ defmodule BoomNotifier.ErrorInfo do Enum.reduce(options, %{}, fn opt, acc -> Map.merge(acc, build_custom_data(conn, opt)) 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 + 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 end diff --git a/lib/boom_notifier/error_storage.ex b/lib/boom_notifier/error_storage.ex index e2510de..547b850 100644 --- a/lib/boom_notifier/error_storage.ex +++ b/lib/boom_notifier/error_storage.ex @@ -34,7 +34,7 @@ defmodule BoomNotifier.ErrorStorage do """ @spec store_error(ErrorInfo.t()) :: :ok def store_error(error_info) do - error_hash_key = generate_error_key(error_info) + %{key: error_hash_key} = error_info timestamp = error_info.timestamp || DateTime.utc_now() default_error_storage_info = %__MODULE__{ @@ -67,7 +67,7 @@ defmodule BoomNotifier.ErrorStorage do """ @spec get_error_stats(ErrorInfo.t()) :: %__MODULE__{} def get_error_stats(error_info) do - error_hash_key = generate_error_key(error_info) + %{key: error_hash_key} = error_info Agent.get(:boom_notifier, fn state -> state end) |> Map.get(error_hash_key) @@ -81,7 +81,7 @@ defmodule BoomNotifier.ErrorStorage do """ @spec send_notification?(ErrorInfo.t()) :: boolean() def send_notification?(error_info) do - error_hash_key = generate_error_key(error_info) + %{key: error_hash_key} = error_info error_storage_item = Agent.get(:boom_notifier, fn state -> state end) @@ -96,7 +96,7 @@ defmodule BoomNotifier.ErrorStorage do """ @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) + %{key: error_hash_key} = error_info Agent.update( :boom_notifier, @@ -108,7 +108,7 @@ defmodule BoomNotifier.ErrorStorage do end def reset_accumulated_errors([exponential: [limit: limit]], error_info) do - error_hash_key = generate_error_key(error_info) + %{key: error_hash_key} = error_info Agent.update( :boom_notifier, @@ -120,7 +120,7 @@ defmodule BoomNotifier.ErrorStorage do end def reset_accumulated_errors(:always, error_info) do - error_hash_key = generate_error_key(error_info) + %{key: error_hash_key} = error_info Agent.update( :boom_notifier, @@ -131,23 +131,6 @@ defmodule BoomNotifier.ErrorStorage do ) 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() - def 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) diff --git a/lib/boom_notifier/notification_sender.ex b/lib/boom_notifier/notification_sender.ex new file mode 100644 index 0000000..4ccd991 --- /dev/null +++ b/lib/boom_notifier/notification_sender.ex @@ -0,0 +1,121 @@ +defmodule BoomNotifier.NotificationSender do + @moduledoc false + + # This GenServer is responsible for sending the notifications in background. + + require Logger + use GenServer + + alias BoomNotifier.ErrorStorage + + # Client API + + def start_link(_opts) do + GenServer.start_link(__MODULE__, :ok, name: __MODULE__) + end + + def async_notify(notifier, occurrences, options) do + GenServer.cast(__MODULE__, {:notify, notifier, occurrences, options}) + end + + def async_trigger_notify(settings, error_info) do + GenServer.cast(__MODULE__, {:trigger_notify, settings, error_info}) + end + + def notify(notifier, occurrences, options) do + spawn_link(fn -> + notifier.notify(occurrences, options) + end) + end + + def notify_all(settings, error_info) do + notification_trigger = Keyword.get(settings, :notification_trigger, :always) + occurrences = Map.put(error_info, :occurrences, ErrorStorage.get_error_stats(error_info)) + + ErrorStorage.reset_accumulated_errors(notification_trigger, error_info) + + BoomNotifier.walkthrough_notifiers( + settings, + fn notifier, options -> notify(notifier, occurrences, options) end + ) + end + + def trigger_notify(settings, error_info) do + timeout = Keyword.get(settings, :time_limit) + + ErrorStorage.store_error(error_info) + + if ErrorStorage.send_notification?(error_info) do + notify_all(settings, error_info) + :ok + else + if timeout do + {:schedule, timeout} + else + :ok + end + end + end + + # Server callbacks + + @impl true + def init(_opts) do + Process.flag(:trap_exit, true) + {:ok, %{}} + end + + @impl true + def handle_cast({:notify, notifier, occurrences, options}, state) do + notify(notifier, occurrences, options) + + {:noreply, state} + end + + @impl true + def handle_cast({:trigger_notify, settings, error_info}, state) do + {timer, state} = Map.pop(state, error_info.key) + + cancel_timer(timer) + + case trigger_notify(settings, error_info) do + :ok -> + {:noreply, state} + + {:schedule, timeout} -> + timer = Process.send_after(self(), {:notify_all, settings, error_info}, timeout) + {:noreply, state |> Map.put(error_info.key, timer)} + end + end + + @impl true + def handle_info({:EXIT, _pid, :normal}, state), do: {:noreply, state} + + def handle_info({:EXIT, _pid, {reason, stacktrace}}, state) do + error_info = Exception.format_banner(:error, reason, stacktrace) + + failing_notifier = + case stacktrace do + [{module, function, arity, _} | _] -> + Exception.format_mfa(module, function, arity) + + [first_stack_entry | _] -> + Exception.format_stacktrace_entry(first_stack_entry) + end + + Logger.error( + "An error occurred when sending a notification: #{error_info} in #{failing_notifier}" + ) + + {:noreply, state} + end + + def handle_info({:notify_all, settings, error_info}, state) do + notify_all(settings, error_info) + + {:noreply, state |> Map.delete(error_info.key)} + end + + defp cancel_timer(nil), do: nil + defp cancel_timer(timer), do: Process.cancel_timer(timer) +end diff --git a/lib/boom_notifier/notifier_sender_server.ex b/lib/boom_notifier/notifier_sender_server.ex deleted file mode 100644 index 1b8d1a2..0000000 --- a/lib/boom_notifier/notifier_sender_server.ex +++ /dev/null @@ -1,57 +0,0 @@ -defmodule BoomNotifier.NotifierSenderServer do - @moduledoc false - - # This GenServer is responsible for sending the notifications in background. - - require Logger - use GenServer - - # Client API - - def start_link(_opts) do - GenServer.start_link(__MODULE__, :ok, name: __MODULE__) - end - - def send(notifier, occurrences, options) do - GenServer.cast(__MODULE__, {:notify, notifier, occurrences, options}) - end - - # Server callbacks - - @impl true - def init(_opts) do - Process.flag(:trap_exit, true) - {:ok, nil} - end - - @impl true - def handle_cast({:notify, notifier, occurrences, options}, _state) do - spawn_link(fn -> - notifier.notify(occurrences, options) - end) - - {:noreply, nil} - end - - @impl true - def handle_info({:EXIT, _pid, :normal}, _state), do: {:noreply, nil} - - def handle_info({:EXIT, _pid, {reason, stacktrace}}, _state) do - error_info = Exception.format_banner(:error, reason, stacktrace) - - failing_notifier = - case stacktrace do - [{module, function, arity, _} | _] -> - Exception.format_mfa(module, function, arity) - - [first_stack_entry | _] -> - Exception.format_stacktrace_entry(first_stack_entry) - end - - Logger.error( - "An error occurred when sending a notification: #{error_info} in #{failing_notifier}" - ) - - {:noreply, nil} - end -end diff --git a/test/example_app/priv/static/favicon.ico b/test/example_app/priv/static/favicon.ico new file mode 100644 index 0000000..e69de29 diff --git a/test/support/bamboo_fake_mailer.ex b/test/support/bamboo_fake_mailer.ex deleted file mode 100644 index dbfc41d..0000000 --- a/test/support/bamboo_fake_mailer.ex +++ /dev/null @@ -1,26 +0,0 @@ -defmodule Support.BambooFakeMailer do - @moduledoc false - - # Overrides the Bamboo `deliver_later!/1` function. - # Instead of sending an email it puts the fields in a mailbox so they can be - # received in the test - - def deliver_later!(email) do - # Since to addresses must be of the correct type, we send the PID through - # as a string. Convert it back into a true pid() for mailbox delivery. - "#PID" <> pid_string = email.to - pid = :erlang.list_to_pid(~c"#{pid_string}") - - send(pid, {:email_subject, email.subject}) - send(pid, {:email_from, email.from}) - send(pid, {:email_to, email.to}) - send(pid, {:email_text_body, text_body_lines(email.text_body)}) - send(pid, {:email_html_body, email.html_body}) - end - - defp text_body_lines(body) do - String.split(body, "\n") - |> Enum.map(&String.trim/1) - |> Enum.reject(&(String.length(&1) == 0)) - end -end diff --git a/test/support/case.ex b/test/support/case.ex index 06d1e38..a01815e 100644 --- a/test/support/case.ex +++ b/test/support/case.ex @@ -6,4 +6,10 @@ defmodule BoomNotifier.Case do setup do BoomNotifier.TestMessageProxy.subscribe(self()) end + + setup_all do + on_exit(fn -> + TestUtils.cancel_notification_sender_timers() + end) + end end diff --git a/test/support/dummy_mailer.ex b/test/support/dummy_mailer.ex new file mode 100644 index 0000000..fc85bee --- /dev/null +++ b/test/support/dummy_mailer.ex @@ -0,0 +1,7 @@ +defmodule BoomNotifier.DummyMailer do + @moduledoc """ + A nop mailer for Swoosh and Bamboo + """ + def deliver_later!(_), do: nil + def deliver!(_), do: nil +end diff --git a/test/support/fake_swoosh_mailer.ex b/test/support/fake_swoosh_mailer.ex deleted file mode 100644 index 1984cb1..0000000 --- a/test/support/fake_swoosh_mailer.ex +++ /dev/null @@ -1,45 +0,0 @@ -defmodule Support.SwooshFakeMailer do - @moduledoc false - - # Overrides the Swoosh `deliver!/1` function. - # - # Instead of sending an email it puts the fields in a mailbox so they can be - # received in the test. - # - # Swoosh does include Swoosh.Adapters.Test, but its behaviour seems unreliable - # in our case because we are catching then sending mail, meaning tests need - # a lot of Process.sleep/1 calls to wait for mailboxes. - # - # It's also possible to use Swoosh.Adapters.Local and - # Swoosh.Adapter.Local.Storage.Memory which you can directly inspect. This - # could be used to check for specific mail. - - def deliver!(email) do - # Swoosh uses a {name, address} tuple for to and from. - # The `name` will default to `""` when not specified in the email creation, - # as is the case in our tests, so extract just the address sections. - # Note: tests only currently support one recipient. - - # this crashes OTP 21, specifically getting the from_addr via tuple match, - # so get from_addr via elem/2 separately... - # %{to: [{_name, "#PID" <> pid_string}], from: {_, from_addr}} = email - %{to: [{_name, "#PID" <> pid_string}], from: from} = email - from_addr = elem(from, 1) - - # Since to addresses must be of the correct type, we send the PID through - # as a string. Convert it back into a true pid() for mailbox delivery. - pid = :erlang.list_to_pid(~c"#{pid_string}") - - send(pid, {:email_subject, email.subject}) - send(pid, {:email_from, from_addr}) - send(pid, {:email_to, pid}) - send(pid, {:email_text_body, text_body_lines(email.text_body)}) - send(pid, {:email_html_body, email.html_body}) - end - - defp text_body_lines(body) do - String.split(body, "\n") - |> Enum.map(&String.trim/1) - |> Enum.reject(&(String.length(&1) == 0)) - end -end diff --git a/test/support/test_mailer.ex b/test/support/test_mailer.ex new file mode 100644 index 0000000..e063da7 --- /dev/null +++ b/test/support/test_mailer.ex @@ -0,0 +1,58 @@ +defmodule BoomNotifier.TestMailer do + @moduledoc """ + Fake mailer for Swoosh and Bamboo + """ + + @doc """ + Bamboo's email deliver that just sends message back + """ + def deliver_later!(email) do + # Use email.to to specify test target pid + email.to + |> to_pid() + |> send_back(email.to, email.from, email) + end + + @doc """ + Swoosh's email deliver that just sends message back + """ + def deliver!(email) do + # Swoosh uses a {name, address} tuple for to and from. + # The `name` will default to `""` when not specified in the email creation, + # as is the case in our tests, so extract just the address sections. + # Note: tests only currently support one recipient. + + # this crashes OTP 21, specifically getting the from_addr via tuple match, + # so get from_addr via elem/2 separately... + # %{to: [{_name, email_to}], from: {_, from_addr}} = email + %{to: [{_name, email_to}], from: from} = email + email_from = elem(from, 1) + + # Use email.to to specify test target pid + email_to + |> to_pid() + |> send_back(email_to, email_from, email) + end + + defp send_back(nil, _, _, _), do: nil + + defp send_back(pid, email_to, email_from, email) do + send(pid, {:email_subject, email.subject}) + send(pid, {:email_from, email_from}) + send(pid, {:email_to, email_to}) + send(pid, {:email_text_body, text_body_lines(email.text_body)}) + send(pid, {:email_html_body, email.html_body}) + end + + defp text_body_lines(body) do + String.split(body, "\n") + |> Enum.map(&String.trim/1) + |> Enum.reject(&(String.length(&1) == 0)) + end + + defp to_pid(value) do + value + |> String.to_existing_atom() + |> Process.whereis() + end +end diff --git a/test/support/test_utils.ex b/test/support/test_utils.ex index df44d88..9634f4f 100644 --- a/test/support/test_utils.ex +++ b/test/support/test_utils.ex @@ -6,4 +6,24 @@ defmodule TestUtils do def above_version?(boundary) do Version.compare(System.version(), boundary) == :gt end + + def clear_error_storage do + Agent.update(:boom_notifier, fn _ -> %{} end) + end + + def cancel_notification_sender_timers do + BoomNotifier.NotificationSender + |> Process.whereis() + |> :sys.get_state() + |> Map.values() + |> Enum.each(&Process.cancel_timer/1) + end + + def flush_messages(timeout \\ 10) do + receive do + _message -> flush_messages() + after + timeout -> nil + end + end end diff --git a/test/unit/error_info_test.exs b/test/unit/error_info_test.exs index 9f7f988..bc587d2 100644 --- a/test/unit/error_info_test.exs +++ b/test/unit/error_info_test.exs @@ -1,5 +1,5 @@ defmodule ErrorInfoTest do - use ExUnit.Case + use BoomNotifier.Case import Plug.Conn import Phoenix.ConnTest @@ -37,9 +37,9 @@ defmodule ErrorInfoTest do use BoomNotifier, notifier: BoomNotifier.MailNotifier.Bamboo, options: [ - mailer: Support.BambooFakeMailer, + mailer: BoomNotifier.DummyMailer, from: "me@example.com", - to: inspect(self()), + to: "me@example.com", subject: "BOOM error caught" ] diff --git a/test/unit/error_storage_test.exs b/test/unit/error_storage_test.exs index d8c6951..7d56bd5 100644 --- a/test/unit/error_storage_test.exs +++ b/test/unit/error_storage_test.exs @@ -1,39 +1,45 @@ defmodule ErrorStorageTest do - use ExUnit.Case, async: true + use BoomNotifier.Case alias BoomNotifier.ErrorInfo alias BoomNotifier.ErrorStorage - @timestamp DateTime.utc_now() + import TestUtils - @error_info %ErrorInfo{ - reason: "Some error information", - stack: ["line 1"], - timestamp: @timestamp - } + @timestamp DateTime.utc_now() - @error_hash ErrorStorage.generate_error_key(@error_info) + @error_info ErrorInfo.build( + %{ + reason: "Some error information", + stack: ["line 1"] + }, + Plug.Test.conn(:get, "/"), + :nothing + ) + |> Map.put(:timestamp, @timestamp) - setup_all do - ErrorStorage.start_link() - :ok - end + @error_hash @error_info.key setup do - Agent.update(:boom_notifier, fn _ -> %{} end) + clear_error_storage() end describe "store_error/1" do test "groups errors by type" do another_timestamp = DateTime.utc_now() - another_error_info = %ErrorInfo{ - reason: "Another error information", - stack: ["line 2"], - timestamp: another_timestamp - } + another_error_info = + ErrorInfo.build( + %{ + reason: "Another error information", + stack: ["line 2"] + }, + Plug.Test.conn(:get, "/"), + :nothing + ) + |> Map.put(:timestamp, another_timestamp) - another_error_hash = ErrorStorage.generate_error_key(another_error_info) + %{key: another_error_hash} = another_error_info ErrorStorage.store_error(@error_info) ErrorStorage.store_error(@error_info) @@ -131,8 +137,8 @@ defmodule ErrorStorageTest do assert ErrorStorage.send_notification?(another_error_info) end - test "returns false when error kind does not exist" do - refute ErrorStorage.send_notification?(%{}) + test "returns false when error key is not stored" do + refute ErrorStorage.send_notification?(%{key: 123}) end end @@ -232,13 +238,18 @@ defmodule ErrorStorageTest do end test "updates the proper error max capacity" do - another_error_info = %ErrorInfo{ - reason: "Another error information", - stack: ["line 2"], - timestamp: @timestamp - } - - another_error_hash = ErrorStorage.generate_error_key(another_error_info) + another_error_info = + ErrorInfo.build( + %{ + reason: "Another error information", + stack: ["line 2"] + }, + Plug.Test.conn(:get, "/"), + :nothing + ) + |> Map.put(:timestamp, @timestamp) + + %{key: another_error_hash} = another_error_info ErrorStorage.store_error(@error_info) ErrorStorage.store_error(another_error_info) diff --git a/test/unit/mailer_notifier_test.exs b/test/unit/mailer_notifier_test.exs index febfd5d..0218df6 100644 --- a/test/unit/mailer_notifier_test.exs +++ b/test/unit/mailer_notifier_test.exs @@ -1,7 +1,9 @@ defmodule MailerNotifierTest do - use ExUnit.Case + use BoomNotifier.Case use Plug.Test + import TestUtils + alias BoomNotifier.MailNotifier doctest BoomNotifier @@ -46,11 +48,11 @@ defmodule MailerNotifierTest do # since we are redefining the router modules for each notifier. notifiers = [ - {"Bamboo", BoomNotifier.MailNotifier.Bamboo, Support.BambooFakeMailer}, - {"Swoosh", BoomNotifier.MailNotifier.Swoosh, Support.SwooshFakeMailer} + {"Bamboo", BoomNotifier.MailNotifier.Bamboo}, + {"Swoosh", BoomNotifier.MailNotifier.Swoosh} ] - for {name, mail_notifier_module, fake_mailer_module} <- notifiers do + for {name, mail_notifier_module} <- notifiers do describe name do # Disable "redefining module" warnings, we have intent. :elixir_config.put(:ignore_module_conflict, true) @@ -60,13 +62,13 @@ defmodule MailerNotifierTest do import Phoenix.Controller @mail_notifier_module mail_notifier_module - @fake_mailer_module fake_mailer_module + @fake_mailer_module BoomNotifier.TestMailer use BoomNotifier, notifier: @mail_notifier_module, options: [ mailer: @fake_mailer_module, from: "me@example.com", - to: inspect(self()), + to: to_string(BoomNotifier.TestMessageProxy), subject: "BOOM error caught" ], custom_data: [:assigns, :logger] @@ -93,13 +95,13 @@ defmodule MailerNotifierTest do use Phoenix.Router @mail_notifier_module mail_notifier_module - @fake_mailer_module fake_mailer_module + @fake_mailer_module BoomNotifier.TestMailer use BoomNotifier, notifier: @mail_notifier_module, options: [ mailer: @fake_mailer_module, from: "me@example.com", - to: inspect(self()), + to: to_string(BoomNotifier.TestMessageProxy), subject: "BOOM error caught", max_subject_length: 25 ] @@ -114,6 +116,8 @@ defmodule MailerNotifierTest do setup do Logger.metadata(name: "Dennis", age: 17) + + on_exit(&flush_messages/0) end test "Raising an error on failure" do @@ -153,7 +157,7 @@ defmodule MailerNotifierTest do test "Set email using proper from and to addresses" do conn = conn(:get, "/") catch_error(TestRouter.call(conn, [])) - email_to = self() + email_to = to_string(BoomNotifier.TestMessageProxy) assert_receive({:email_from, "me@example.com"}, @receive_timeout) assert_receive({:email_to, ^email_to}, @receive_timeout) @@ -287,11 +291,11 @@ defmodule MailerNotifierTest do [second_stack_line | _] = stacktrace_list - assert "
  • test/unit/mailer_notifier_test.exs:20: MailerNotifierTest.TestController.index/2
  • " = - first_stack_line + assert first_stack_line =~ + ~r"
  • test/unit/mailer_notifier_test.exs:\d+: MailerNotifierTest.TestController.index/2
  • " - assert "
  • test/unit/mailer_notifier_test.exs:11: MailerNotifierTest.TestController.action/2
  • " = - second_stack_line + assert second_stack_line =~ + ~r"
  • test/unit/mailer_notifier_test.exs:\d+: MailerNotifierTest.TestController.action/2
  • " end end @@ -372,7 +376,7 @@ defmodule MailerNotifierTest do describe "MailNotifier.validate_config" do setup do options = [ - mailer: Support.BambooFakeMailer, + mailer: BoomNotifier.TestMailer, from: "boom@mailer", to: "user@mail", subject: "boom mail", diff --git a/test/unit/notification_sender_test.exs b/test/unit/notification_sender_test.exs new file mode 100644 index 0000000..8f0592b --- /dev/null +++ b/test/unit/notification_sender_test.exs @@ -0,0 +1,162 @@ +defmodule BoomNotifier.NotificationSenderTest do + use BoomNotifier.Case + + import TestUtils + + alias BoomNotifier.{ + ErrorInfo, + ErrorStorage, + NotificationSender + } + + @time_limit 500 + @receive_timeout 100 + + @settings_basic [ + notifier: __MODULE__.NotificationSenderTestNotifier, + options: [pid_name: BoomNotifier.TestMessageProxy] + ] + + @settings_groupping @settings_basic ++ + [ + time_limit: @time_limit, + notification_trigger: :exponential + ] + + defmodule NotificationSenderTestNotifier do + def notify(error_info, opts) do + pid = Process.whereis(opts[:pid_name]) + + if pid, do: send(pid, {:notify_called, error_info}) + end + end + + def build_error_info(_) do + raise "an error" + rescue + e -> + error_info = + ErrorInfo.build( + %{reason: e, stack: __STACKTRACE__}, + Plug.Test.conn(:get, "/"), + :nothing + ) + + %{error_info: error_info} + end + + setup do + clear_error_storage() + + on_exit(&flush_messages/0) + on_exit(&cancel_notification_sender_timers/0) + + :ok + end + + setup :build_error_info + + describe "with default notification trigger (always)" do + test "sends a notification", %{error_info: error_info} do + NotificationSender.trigger_notify(@settings_basic, error_info) + + {_, rcv_error_info} = assert_receive({:notify_called, _}, @receive_timeout) + assert Map.delete(rcv_error_info, :occurrences) == Map.delete(error_info, :occurrences) + end + end + + describe "sync call with exponential notification trigger" do + test "sends a notification", %{error_info: error_info} do + NotificationSender.trigger_notify(@settings_groupping, error_info) + + {_, rcv_error_info} = assert_receive({:notify_called, _}, @receive_timeout) + assert Map.delete(rcv_error_info, :occurrences) == Map.delete(error_info, :occurrences) + end + + test "does not send a second notification", %{error_info: error_info} do + ErrorStorage.store_error(error_info) + ErrorStorage.reset_accumulated_errors(:exponential, error_info) + + trigger_notify_resp = NotificationSender.trigger_notify(@settings_groupping, error_info) + + refute_receive({:notify_called, _}, @receive_timeout) + assert {:schedule, @time_limit} = trigger_notify_resp + end + + test "sends notification occurrences along error info", %{error_info: error_info} do + for _ <- 1..7 do + NotificationSender.trigger_notify(@settings_groupping, error_info) + end + + assert_receive({:notify_called, %{occurrences: %{accumulated_occurrences: 1}}}) + assert_receive({:notify_called, %{occurrences: %{accumulated_occurrences: 2}}}) + assert_receive({:notify_called, %{occurrences: %{accumulated_occurrences: 4}}}) + end + end + + describe "async call with exponential notification trigger" do + test "sends a notification", %{error_info: error_info} do + NotificationSender.async_trigger_notify(@settings_groupping, error_info) + + assert_receive({:notify_called, _}, @receive_timeout) + end + end + + describe "repeated async call with exponential notification trigger" do + setup(%{error_info: error_info}) do + ErrorStorage.store_error(error_info) + ErrorStorage.reset_accumulated_errors(:exponential, error_info) + end + + test "sends a second notification after a timeout", %{error_info: error_info} do + NotificationSender.async_trigger_notify(@settings_groupping, error_info) + + assert_receive({:notify_called, _}, @time_limit + @receive_timeout) + + assert error_info |> ErrorStorage.get_error_stats() |> Map.get(:accumulated_occurrences) == + 0 + end + + test "does not send a second notification before a timeout", %{error_info: error_info} do + NotificationSender.async_trigger_notify(@settings_groupping, error_info) + + refute_receive({:notify_called, _}, @time_limit - 50) + + assert ErrorStorage.get_error_stats(error_info) |> Map.get(:accumulated_occurrences) > 0 + end + + test( + "it does not sends a scheduled notification if another error happens", + %{error_info: error_info} + ) do + NotificationSender.async_trigger_notify(@settings_groupping, error_info) + NotificationSender.async_trigger_notify(@settings_groupping, error_info) + NotificationSender.async_trigger_notify(@settings_groupping, error_info) + + notification_sender_state = + NotificationSender + |> Process.whereis() + |> :sys.get_state() + + error_key = error_info.key + + assert notification_sender_state |> Map.keys() |> length() == 1 + assert %{^error_key => _} = notification_sender_state + end + + test( + "it does not schedule a notification if time_limit is not specified", + %{error_info: error_info} + ) do + settings = Keyword.delete(@settings_groupping, :time_limit) + + NotificationSender.async_trigger_notify(settings, error_info) + NotificationSender.async_trigger_notify(settings, error_info) + NotificationSender.async_trigger_notify(settings, error_info) + + notification_sender_state = :sys.get_state(Process.whereis(NotificationSender)) + + assert notification_sender_state |> Map.keys() |> length() == 0 + end + end +end diff --git a/test/unit/notifier_test.exs b/test/unit/notifier_test.exs index b84d9de..f857b5d 100644 --- a/test/unit/notifier_test.exs +++ b/test/unit/notifier_test.exs @@ -1,8 +1,11 @@ defmodule NotifierTest do - use ExUnit.Case + use BoomNotifier.Case use Plug.Test import ExUnit.CaptureLog + import TestUtils + + alias BoomNotifier.TestMessageProxy doctest BoomNotifier @@ -14,12 +17,14 @@ defmodule NotifierTest do @impl BoomNotifier.Notifier def notify(error_info, options) do subject_prefix = Keyword.get(options, :subject) - to_respond_pid = Keyword.get(options, :sender_pid) + to_respond_pid = Keyword.get(options, :sender_pid_name) |> Process.whereis() 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}}) + # If test does not wait for a message, pid might be already dead at this point + if to_respond_pid, + do: send(to_respond_pid, %{exception: %{subject: subject, body: body}}) end end @@ -59,7 +64,7 @@ defmodule NotifierTest do notifier: FakeNotifier, options: [ subject: "BOOM error caught", - sender_pid: self() + sender_pid_name: TestMessageProxy ] def call(_conn, _opts) do @@ -74,7 +79,7 @@ defmodule NotifierTest do notifier: FakeNotifier, options: [ subject: "BOOM error caught", - sender_pid: self() + sender_pid_name: TestMessageProxy ] ] ] @@ -89,7 +94,7 @@ defmodule NotifierTest do notifier: FakeNotifier, options: [ subject: "BOOM error caught", - sender_pid: self() + sender_pid_name: TestMessageProxy ] def call(_conn, _opts) do @@ -102,7 +107,7 @@ defmodule NotifierTest do notifier: FakeNotifier, options: [ subject: "BOOM error caught", - sender_pid: self() + sender_pid_name: TestMessageProxy ] def call(_conn, _opts) do @@ -116,7 +121,7 @@ defmodule NotifierTest do notification_trigger: :exponential, options: [ subject: "BOOM error caught", - sender_pid: self() + sender_pid_name: TestMessageProxy ] def call(_conn, _opts) do @@ -130,7 +135,7 @@ defmodule NotifierTest do notification_trigger: [exponential: [limit: 3]], options: [ subject: "BOOM error caught", - sender_pid: self() + sender_pid_name: TestMessageProxy ] def call(_conn, _opts) do @@ -151,7 +156,9 @@ defmodule NotifierTest do end setup do - Agent.update(:boom_notifier, fn _state -> %{} end) + clear_error_storage() + + on_exit(&flush_messages/0) end test "keeps raising an error on exception" do @@ -353,7 +360,7 @@ defmodule NotifierTest do notifier: FakeNotifier, options: [ subject: "BOOM error caught", - sender_pid: self() + sender_pid_name: TestMessageProxy ], ignore_exceptions: [TestException] @@ -367,7 +374,7 @@ defmodule NotifierTest do notifier: FakeNotifier, options: [ subject: "BOOM error caught", - sender_pid: self() + sender_pid_name: TestMessageProxy ], ignore_exceptions: [TestException] @@ -396,13 +403,14 @@ defmodule NotifierTest do use Plug.ErrorHandler def handle_errors(_conn, _error) do - send(self(), :handle_errors_called) + test_pid = Process.whereis(TestMessageProxy) + send(test_pid, :handle_errors_called) end use BoomNotifier, notifier: FakeNotifier, options: [ - sender_pid: self() + sender_pid_name: TestMessageProxy ] def call(_conn, _opts) do diff --git a/test/unit/webhook_notifier_test.exs b/test/unit/webhook_notifier_test.exs index 839b8c2..d10140a 100644 --- a/test/unit/webhook_notifier_test.exs +++ b/test/unit/webhook_notifier_test.exs @@ -1,5 +1,5 @@ defmodule WebhookNotifierTest do - use ExUnit.Case, async: false + use BoomNotifier.Case use Plug.Test doctest BoomNotifier