-
Notifications
You must be signed in to change notification settings - Fork 56
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
Fix custom class and message notices #359
Conversation
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.
Nice catch and thanks for the PR. The test looks perfect, and the implementation itself is great. I'd like to rework it a little and avoid a do_new
private function, though.
lib/honeybadger/notice.ex
Outdated
@@ -47,15 +47,27 @@ defmodule Honeybadger.Notice do | |||
new(%RuntimeError{message: message}, metadata, stacktrace, fingerprint) | |||
end | |||
|
|||
def new(%{class: exception_name, message: message}, metadata, stacktrace, fingerprint) do | |||
do_new(exception_name, message, metadata, stacktrace, fingerprint) |
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.
Can we do avoid the do_new
private function, modify the payload instead, and pass it to new/4
again? We don't have much (any?) use of do_
prefixed private functions elsewhere.
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.
Err... Pass it to which new/4
?
I can't pass it as a string, seems against the point of passing a custom class and message.
I can't bundle it back into an exception or we are back where we started with ErlangError wrapping.
I could instead extract the exception into a %{class: ..., message: ...}
call?
At least, as I understand it, the "class" name given isn't beholden to being a real type, just something you want HB to show in the UI, as here where I can have an "AnyClass" error:
Maybe I misunderstand the intention and it should be creating real error types.
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.
Excellent question. I was only looking at a subset of the clauses and didn't see the one before this. The whole class
thing is strange anyhow!
We can't use a guard like is_exception
because we support older Elixir versions. How about a private new/5
function? It's what you wrote, but without the do_prefix
.
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.
Can do chief.
Should it also handle %{class: xxx}
, and set a default message (even just a blank string)? Right now that will miss the match and end up as a ErlangError.
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.
The only way to create one of these errors is by manually calling notify with a class payload, and we only document doing that with a message
key as well. It's reasonable that sending an improper format causes an ErlangError.
Honestly, we shouldn't encourage people to use the class/message variant and should only document sending a straight message or a proper exception.
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.
My reason for wanting %{class..., message: ...}
is paths that are a failure in some respect, I do want a notification about it, but it isn't actually a runtime error.
In my case I may have a DB transaction fail. It's caught and handled but I don't want a specific error type for each transaction, I just want to know it went wrong and be able to tag them separately. So I need to be able to specify a type class for them, otherwise they call get bundled under "RuntimeError". Fingerprinting will separate them out but they're still ugly to parse in the UI.
I should just have a proper type but... well, passing some strings around can be a lot simpler.
Really I wish I could go Honeybadger.notify("app.module_a.transaction_type", "failed: reason")
. Turn the "message" api into "title & message".
dbaec51
to
8c07a5d
Compare
ef89974
to
8372182
Compare
8372182
to
7d12436
Compare
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.
The change looks great now 👍
Thanks for explaining your use case, that makes sense to me. It's pretty light weight to create custom errors and I suggest that you go that route. But, considering that the class/message format is already available, we need to support it cleanly.
It seemed that
Were not being reported in the way they should:
Adds new function head to match the custom format. I can't imagine any "real" exception would ever match that pattern, they should all be structs, so ... probably safe?
See also: #336
NB "class" is kind of a weird term (over maybe "type" or even just "name") in Elixir but I assume it's to keep the API uniform across languages.