Skip to content
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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Manual notify exception #76

wants to merge 2 commits into from

Conversation

mcass19
Copy link
Contributor

@mcass19 mcass19 commented Jan 12, 2022

This PR aims to implement the ability to manually send an exception. It's a first try so feel free to discuss / suggest whatever you want. Actually, there are a few things to discuss to set a direction to go.

To do / discuss

  • The manual exceptions are always sending an empty stacktrace for now
  • I added an extra check to see if we are on the router when defining handle_errors. I don't like it too much but I think it covers most of the cases (it doesn't cover if the user defines a module ending in .Router)
  • I changed the configuration so that it is read from the application environment, since now we can do use BoomNotifier anywhere and with the current implementation it would be necessary to always write it
    • But maybe the user wants different settings in different places or am I crazy?
    • Update tests if we keep this change
    • Update readme if we keep this change
  • Do we want to always send the conn object or do we want to be more open to any elixir code and just send the exception without much more information?
  • I defined a new manual_notify_error callback, but maybe it is better to have only one notify_error that handles all cases?

@jmbejar
Copy link
Member

jmbejar commented Jan 17, 2022

  • The manual exceptions are always sending an empty stacktrace for now

I think we can send (or teach the user to send) the stacktrace from the rescue block, using STACKTRACE/0.

@jmbejar
Copy link
Member

jmbejar commented Jan 17, 2022

I changed the configuration so that it is read from the application environment, since now we can do use BoomNotifier anywhere and with the current implementation it would be necessary to always write it

  • But maybe the user wants different settings in different places or am I crazy?

I think it is fair to have a unique global configuration, so we have no other choice :)


```elixir
defmodule MyController do
use BoomNotifier
Copy link
Member

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 a use. So, you can avoid all the extra complexity of checking if we are in the context of a router or a controller in BoomNotifier.__using__.

In fact, it is probably fine to say that the user should invoke BoomNotifier.notify_error directly (the user can or not add an import)

Copy link
Member

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.

do_notify_error(conn, error)
end

def manual_notify_error(conn, error) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using the already existing notify_error function? I think all the logic there applies here (limits & filters for exceptions). So, we would have similar ways in router and controller to notify errors when exceptions are being intercepted by user code (error_handler or simple try-catch-rescue blocks).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants