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

Failover handlers when async_handler fails #135

Merged
merged 12 commits into from
Oct 7, 2014

Conversation

jondeandres
Copy link
Contributor

This PR allow the users to define an array of failover handlers when the async_handler fails, for example in a Redis push, connection etc...

The users can use it in this way:

Rollbar.configure do |config|
  config.failover_handlers = [failover1, failover2] # failover1 and failover2 are just objects responding to #call
end

It's also possible to use the default async handler:

Rollbar.configure do |config|
  config.failover_handlers = Rollbar.default_async_handler
end

An interesting refactor could be replace the Rollbar.default_async_handler in order to use classes like Rollbar::Delay::Thread or Rollbar::Delay::GirlFriday. I think that Rollbar::Delay::Thread could be the most used failover handler.

@jondeandres jondeandres force-pushed the failover branch 4 times, most recently from 5332536 to 216ee56 Compare October 3, 2014 09:41
Jon de Andres added 2 commits October 3, 2014 12:46
The users can now configure the async_handler in this way:

Rollbar.configure do |config|
  config.async_handler = Rollbar::Delay::Thread
end

Also allowed for config.failover_handlers.
configuration.async_handler ||= default_async_handler
configuration.async_handler.call(payload)
rescue
raise unless configuration.failover_handlers.any?
Copy link
Member

Choose a reason for hiding this comment

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

Where is this exception going to go?

Asking because I want to make sure that:

  • it won't bubble up to the end-user
  • it still gets logged somewhere (i.e. to the rails log)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I'm only reraising the exception in case that the handler failed to process the payload. Really this is the actual behaviour. If you don't have any failover handler, then it just crashes, isn't it?

Well, it will not crash cause it's rescued in https://github.com/jondeandres/rollbar-gem/blob/failover/lib/rollbar.rb#L111.

This will be logged in https://github.com/jondeandres/rollbar-gem/blob/failover/lib/rollbar.rb#L566 finally

@brianr
Copy link
Member

brianr commented Oct 6, 2014

OK one last concern:

In the case where all failovers fail, we are eventually going to call report_internal_error which will call schedule_payload. But if the failovers all failed, then that schedule_payload call is also (probably) going to fail. That will result in a call to send_failsafe, which will call schedule_payload again, fail again, and then finally log an error here: https://github.com/jondeandres/rollbar-gem/blob/failover/lib/rollbar.rb#L620

The system is probably under stress when this happens so I think we should probably just short-circuit all of that and log, but not re-raise, the error when the failovers are exhausted. What do you think?

@jondeandres
Copy link
Contributor Author

Mmmm. it's true @brianr...

What if we finally send it inline calling process_payload? Another option is disable use_async, call report_internal_error and re-enable it. But perhaps too complex.

Something we should do it's perhaps recommend the customers to use at least the Thread handler at the last. If that handler it's failing it's probably a problem with the operating system or in the API.

The easiest solution is just log an error here, of course.

@brianr
Copy link
Member

brianr commented Oct 6, 2014

I think recommending Thread last is a good idea. That plus logging an error here seems like the way to go.

@jondeandres
Copy link
Contributor Author

One question @brianr, actually the behaviour you've explained it's really the actual behaviour, isn't it? I mean, if the only async_handler fails report_intenral_error will try to use that failing handler no?

Just to understand this.

@brianr
Copy link
Member

brianr commented Oct 6, 2014

Yeah, right now, in master, it's going to go around again (and probably fail again).

@jondeandres
Copy link
Contributor Author

@brianr f810954

Do you think it's more correct now?

Jon de Andres added 5 commits October 7, 2014 02:35
This method will set Rollbar::Delay::Thread as the async handler. It just use a Thread in order to process the payload.
Now users can set Resque in this way:
config.use_resque queue: 'errors' # queue is optional.
brianr added a commit that referenced this pull request Oct 7, 2014
Failover handlers when async_handler fails
@brianr brianr merged commit a9dd476 into rollbar:master Oct 7, 2014
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