-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Manual notify exception #76
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ defmodule BoomNotifier do | |
# Responsible for sending a notification to each notifier every time an | ||
# exception is raised. | ||
|
||
alias BoomNotifier.Config | ||
alias BoomNotifier.ErrorStorage | ||
alias BoomNotifier.NotifierSenderServer | ||
require Logger | ||
|
@@ -25,10 +26,10 @@ defmodule BoomNotifier do | |
end | ||
end | ||
|
||
def walkthrough_notifiers(settings, callback) do | ||
case Keyword.get(settings, :notifiers) do | ||
nil -> | ||
run_callback(settings, callback) | ||
def walkthrough_notifiers(callback) do | ||
case Config.notifiers() do | ||
[] -> | ||
run_callback(Config.single_notifier_config(), callback) | ||
|
||
notifiers_settings when is_list(notifiers_settings) -> | ||
Enum.each(notifiers_settings, &run_callback(&1, callback)) | ||
|
@@ -49,13 +50,14 @@ defmodule BoomNotifier do | |
end | ||
end | ||
|
||
defmacro __using__(config) do | ||
defmacro __using__(_config) do | ||
quote location: :keep do | ||
import BoomNotifier | ||
|
||
error_handler_in_use = {:handle_errors, 2} in Module.definitions_in(__MODULE__) | ||
is_router = __MODULE__ |> to_string() |> String.split(".") |> List.last() == "Router" | ||
|
||
unless error_handler_in_use do | ||
if is_router and not error_handler_in_use do | ||
use Plug.ErrorHandler | ||
|
||
@impl Plug.ErrorHandler | ||
|
@@ -65,40 +67,46 @@ defmodule BoomNotifier do | |
end | ||
|
||
# Notifiers validation | ||
walkthrough_notifiers( | ||
unquote(config), | ||
fn notifier, options -> validate_notifiers(notifier, options) end | ||
) | ||
walkthrough_notifiers(fn notifier, options -> 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) | ||
unless Enum.member?(Config.ignore_exceptions(), mod) do | ||
do_notify_error(conn, error) | ||
end | ||
end | ||
|
||
def notify_error(conn, error) do | ||
do_notify_error(conn, unquote(config), error) | ||
do_notify_error(conn, error) | ||
end | ||
|
||
def manual_notify_error(conn, error) do | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about using the already existing |
||
{_error_kind, error_info} = | ||
ErrorInfo.build(%{kind: :error, reason: error, stack: []}, conn, Config.custom_data()) | ||
|
||
# Triggers the notification in each notifier | ||
walkthrough_notifiers(fn notifier, options -> | ||
NotifierSenderServer.send( | ||
notifier, | ||
[error_info], | ||
options | ||
) | ||
end) | ||
end | ||
|
||
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) | ||
defp do_notify_error(conn, error) do | ||
{error_kind, error_info} = ErrorInfo.build(error, conn, Config.custom_data()) | ||
|
||
ErrorStorage.add_errors(error_kind, error_info) | ||
|
||
if ErrorStorage.send_notification?(error_kind) do | ||
occurrences = ErrorStorage.get_errors(error_kind) | ||
|
||
# Triggers the notification in each notifier | ||
walkthrough_notifiers(settings, fn notifier, options -> | ||
walkthrough_notifiers(fn notifier, options -> | ||
NotifierSenderServer.send(notifier, occurrences, options) | ||
end) | ||
|
||
{notification_trigger, _settings} = | ||
Keyword.pop(settings, :notification_trigger, :always) | ||
notification_trigger = Config.notification_trigger() | ||
|
||
ErrorStorage.clear_errors(notification_trigger, error_kind) | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
defmodule BoomNotifier.Config do | ||
@moduledoc """ | ||
This module provides the functionality for fetching configuration settings and their defaults. | ||
""" | ||
|
||
@custom_data_default :nothing | ||
@ignore_exceptions_default [] | ||
@notifiers_default [] | ||
@notification_trigger_default :always | ||
@notifier_default nil | ||
@options_default [] | ||
|
||
def custom_data do | ||
get_config(:custom_data, @custom_data_default) | ||
end | ||
|
||
def ignore_exceptions do | ||
get_config(:ignore_exceptions, @ignore_exceptions_default) | ||
end | ||
|
||
def notifiers do | ||
get_config(:notifiers, @notifiers_default) | ||
end | ||
|
||
def notification_trigger do | ||
get_config(:notification_trigger, @notification_trigger_default) | ||
end | ||
|
||
def single_notifier_config do | ||
[ | ||
notifier: get_config(:notifier, @notifier_default), | ||
options: get_config(:options, @options_default) | ||
] | ||
end | ||
|
||
defp get_config(key, default) do | ||
case Application.fetch_env(:boom_notifier, key) do | ||
{:ok, value} -> value | ||
_ -> default | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this should be an
import
instead of ause
. So, you can avoid all the extra complexity of checking if we are in the context of a router or a controller inBoomNotifier.__using__
.In fact, it is probably fine to say that the user should invoke
BoomNotifier.notify_error
directly (the user can or not add animport
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the only thing to solve here is to make sure that config validations runs at least once. If the module is used in the Router is fine, but we should make it mandatory (even if it is not going to be used there).
Another option would be to check configs directly in the new
Config
module, as part of any of the functions defined there.. or maybe as an operation that is done only once and cached.