airbrake-ruby: return nil instead of raising error #75
Conversation
kyrylo
force-pushed
the
546-default-notifier-error-fix
branch
3 times, most recently
from
5ad79ba to
d4b09cd
Compare
| raise Airbrake::Error, | ||
| "the '#{notifier}' notifier isn't configured" | ||
| unless @notifiers.key?(notifier) | ||
| return { 'error' => "the '#{notifier}' notifier isn't configured" } |
There was a problem hiding this comment.
Could it be helpful to explain the consequences in this message, to help explain what this means and to help decide if they want to fix it? Also, is it obvious to the user that this is related to Airbrake? Maybe something like: { 'error' => "the '#{notifier}' notifier isn't configured, Airbrake will not be notified of errors" }
There was a problem hiding this comment.
Could it be helpful to explain the consequences in this message, to help explain what this means and to help decide if they want to fix it?
You mean, in the readme?
Also, is it obvious to the user that this is related to Airbrake
I guess it can be more obvious, yes.
There was a problem hiding this comment.
You mean, in the readme?
No, in the message. I tried to resolve both points I made with my suggestion.
There was a problem hiding this comment.
How about: **Airbrake: the '#{notifier}' notifier isn't configured, '#{method}' was not called.
This error can occur even if you call add_filter, for example. So if you call that and see Airbrake will not be notified of errors, it might be a little bit confusing. On the other hand, I doubt anybody will be checking the return value of add_filter or any other value than notify_sync. Thoughts?
kyrylo
force-pushed
the
546-default-notifier-error-fix
branch
2 times, most recently
from
66e5b20 to
64727dc
Compare
|
PTAL |
| return { | ||
| 'error' => "#{LOG_LABEL} the '#{notifier}' notifier isn't configured, " \ | ||
| "'#{method}' was not called" | ||
| } |
There was a problem hiding this comment.
Why not to log the same error message? What is the point of returning this if none really will handle it?
There was a problem hiding this comment.
I was thinking about doing it like so, but the problem is that we log through user configured loggers, via @config.logger.debug(...). If none of the notifiers exist, it also means loggers don't exist. I thought about logging to STDERR, but decided not to do so because there's a risk of polluting it with countless messages, which is something that people who want to disable Airbrake would hate.
There was a problem hiding this comment.
The just remove this return. None will ever use it.
There was a problem hiding this comment.
- We still need to return early in order to prevent execution (which will crash), so I assume you want to remove the hash
- I can see your point and I partially agree with it, but my question is what harm will this hash introduce? If nobody uses the return value, it's still useful. If a customer complains that Airbrake doesn't send errors, my first question will be "what is the return value?". Think of it as a support tool.
There was a problem hiding this comment.
This looks wrong to me, because now every notifier method can return hash {error: "qqqq"} (even method that return bool etc) and that is a bad design.
There was a problem hiding this comment.
Stopped returning the Hash. PTAL
kyrylo
force-pushed
the
546-default-notifier-error-fix
branch
from
43bf37d to
146fcdf
Compare
Fixes airbrake/airbrake#546 (Disable Airbrake when not configured?) This is probably the most annoying thing about this library. People often don't see the original error when we raise ours (the original error becomes nested and it is hard to notice). Sometimes users also want to disable Airbrake, but they can't do so because once the library is required, it assumes that the user wants to configure it immediately. So if you don't configure it, your app crashes. The solution is to stop raising this error. If not the complaints, I would probably leave it as is. The current behaviour makes a lot of sense to me. However the amount of confusion is immense, so "we must deal with it".
kyrylo
force-pushed
the
546-default-notifier-error-fix
branch
from
146fcdf to
d3f04d3
Compare
kyrylo
changed the title
|
LGTM, but I would add a log message |
|
Added a log message. |
|
After trying this branch I am getting this (when Airbrake is not configured): undefined method `[]' for nil:NilClass |
|
@khustochka thanks for letting me know! I'll prepare a fix tomorrow. |
This change along with airbrake/airbrake-ruby#75 fix #546. Due to the breaking change in airbrake-ruby we need to update some of our integrations that use `Airbrake.build_notice`.
This change along with airbrake/airbrake-ruby#75 fix #546. Due to the breaking change in airbrake-ruby we need to update some of our integrations that use `Airbrake.build_notice`.
Fixes airbrake/airbrake#546
(Disable Airbrake when not configured?)
This is probably the most annoying thing about this library. People
often don't see the original error when we raise ours (the original
error becomes nested and it is hard to notice). Sometimes users also
want to disable Airbrake, but they can't do so because once the library
is required, it assumes that the user wants to configure it
immediately. So if you don't configure it, your app crashes.
The solution is to stop raising this error. If not the complaints, I
would probably leave it as is. The current behaviour makes a lot of
sense to me. However the amount of confusion is immense, so "we must
deal with it".