jonallured / braze_ruby

A wrapper gem for the Braze REST API.
MIT License
18 stars 21 forks source link

Add retry configuration pass through #31

Closed JerrodCarpenter closed 3 years ago

JerrodCarpenter commented 3 years ago

We're seeing many errors due to Faraday::ConnectionFailed: execution expired. Might be nice to be able to configure retries within the internal faraday client to avoid this.

trevorrjohn commented 3 years ago

@jonallured any feedback on this?

jonallured commented 3 years ago

Welp I definitely did not have this repo watched so didn't see this until today! 😬

I think I'm maybe more interested in supplying a custom Faraday connection instead of the one that the gem provides. That seems like a cooler way to set all kinds of finicky options. Maybe something like:

custom_connection = Faraday.new(...blah...)
BrazeRuby.config do |c|
  c.set_connection(custom_connection)
end

But that's just sketch, not really something that could be easily done today I guess. I just did a little skimming through the codebase and it appears we have cached clients in the REST base class:

https://github.com/jonallured/braze_ruby/blob/5b34717f9f36e9ada465561df7ca78e13720527b/lib/braze_ruby/rest/base.rb#L19

Which I guess is ok but I'd rather promote this client somewhere closer to the top namespace so that configuring it could be easier. I will ponder this. What do you yall think @JerrodCarpenter @trevorrjohn?

trevorrjohn commented 3 years ago

@jonallured what about having options take a block:

options[:connection] = Proc.new do |connection|
  connection.request :retry
  connection.options[:open_timeout] = 2
  // etc.
end

BrazeRuby::API.new(key, host, options)

I am not sure this is the best solution, but it would take minimal changes.

jonallured commented 3 years ago

Hey @JerrodCarpenter can you rebase on latest main branch - that should cause CircleCI to build and verify this work. Thanks!

JerrodCarpenter commented 3 years ago

@jonallured thanks! CI worked like a charm.

JerrodCarpenter commented 3 years ago

LGTM?

jonallured commented 3 years ago

Ugh I totally owe you a response on this, sorry for the delay! I'm headed out but will follow up tomorrow.

jonallured commented 3 years ago

Ok here's what I'm thinking: going to merge #31 and #34 and cut a release. That at least gets us to a point where the gem can accept these options and you're off and running.

Ultimately I'd like to move the gem in the direction of more standard config in the Ruby ecosystem with like a config block rather than sending in options as the api class is instantiated. That change is probably enough to warrant a major version bump. At that point it would make more sense to be to allow for one to send in a configured Faraday connection and then the gem can use that instead. All that to say, let's merge these suckers and publish!