Asynchronous notifier shouldn't block if queue full #47
Conversation
| @@ -54,7 +55,8 @@ | |||
|
|
|||
| it "prints the debug message the correct number of times" do | |||
| log = @stderr.string.split("\n") | |||
| expect(log.grep(/\*\*Airbrake: \{\}/).size).to eq(300) | |||
| expect(log.grep(/\*\*Airbrake: \{\}/).size).to \ | |||
| be >= @sender.instance_variable_get(:@unsent).max | |||
There was a problem hiding this comment.
Nitpick: could you please break it into two lines like this instead:
expect(log.grep(/\*\*Airbrake: \{\}/).size).
to be >= @sender.instance_variable_get(:@unsent).maxIt is more consistent with the current style.
|
Thanks for the PR, it makes sense. Fixes #48.
|
|
If it helps, here's a reference to an implementation I created a while back: |
| @unsent << notice | ||
| if @unsent.size >= @unsent.max | ||
| msg = "#{LOG_LABEL} dropping notice since sending queue is full" | ||
| @config.logger.error msg |
There was a problem hiding this comment.
It was more of a question. I am not sure if we need to log, but maybe you would find it useful?
If we do want, I am not sure about the error severity. It's more like a warn to me, but I am open to other suggestions.
There was a problem hiding this comment.
I like the idea to log, so the information it's not lost and can be retrieved if necessary.
Having the queue full of errors and starting to not send them to the error log platform it's, for me, a critical situation. If, for some reason, I'm not aware of this situation and I'm looking at the logs I would like to see it quickly since it's an ERROR. What do you think ?
I'm almost done with the changes, I'm having some trouble with the specs but I hope to fix it soon.
|
Done! I've followed @stevecrozz style. As I was saying in the previous comment, I think is interesting to log the notice (so the information is not lost) and I would consider it with Error severity. Like I've said: "Having the queue full of errors and starting to not send them to the error log platform it's, for me, a critical situation. If, for some reason, I'm not aware of this situation and I'm looking at the logs I would like to see it quickly since it's an ERROR". What do you think ? |
|
|
||
| def will_not_deliver(notice) | ||
| backtrace = notice[:errors][0][:backtrace].map do |line| | ||
| "#{line[:file]}:#{line[:line]} in `#{line[:function]}`" |
There was a problem hiding this comment.
Should be ``#{line[:function]}'` (no backtick in the end) if you want to make it resemble a Ruby stack frame.
|
Makes sense to me, thanks for the explanation. I am happy to merge this with a couple minor fixes that I mentioned above. |
|
Applied all your fixes and commented about the condition! |
|
Looks great, thanks 👍 |
Asynchronous notifier shouldn't block if queue full
|
Thanks! When do you plan to release the next version of airbrake-ruby @kyrylo ? |
|
I plan to release it sometime during this week. Waiting on #46 to be resolved and then we're ready to release :) |
SizedQueue blocks if the queue is full. I think this behaviour is completely unexpected in an asynchronous component.
I think it would be better to drop the error in this situation. I can picture 2 scenarios where this could happen and dropping the extra errors would make sense:
I have fixed the specs accordingly