support client middleware?

[sax]

I know this is a big feature request, but wanted to start an issue to gauge interest, and see if this is something that would even be accepted into the gem. I'm not sure we have the resources to do a pull request in the near term, but might be able to help with this feature in the next few months.

We're instrumenting all our calls to the Stripe API, and this is something that would be easily accomplished with middleware IF the gem supported that design pattern. For instance, Faraday is written in a way that makes it easy to write and register new client middleware.

This could potentially make other gems like stripe-ruby-mock easier to implement.

[kyleconroy]

@sax can you include a Faraday middleware example? I'd be interested to see what that looks like.

[sax]

We have an elasticsearch client that uses Faraday, and in our application we rescue connection errors with empty responses, so our users see blank pages rather than 500 errors. I'm simplifying it here, but hopefully you can get the picture.

module ElasticsearchClient
  class ErrorCatcher < Faraday::Middleware
    BLANK_HITS_BODY = { 'hits' => { 'total' => 0, 'hits' => [] } }

    def call(env)
      @app.call(env)
    rescue Errno::ETIMEDOUT, Timeout::Error, Faraday::Error::TimeoutError, Faraday::ClientError => e
      # Elasticsearch ruby client uses this exception as a part of its normal operation
      raise if e.is_a?(Faraday::ResourceNotFound)
      Faraday::Response.new(env.merge(body: BLANK_HITS_BODY, status: 200))
    end
  end
end

You might also look at https://github.com/remiprev/her, which is built around Faraday.

I envision the metrics middleware being something like this:

class StripeMetrics < Faraday::Middleware
  attr_reader :statsd

  def initialize(app, statsd)
    super(app)
    @statsd = statsd
  end

  def resource_from(env)
    # charges, customers, accounts, etc, added into env somewhere in
    # the middleware chain or retrieved from the request body itself.
  end

  def call(env)
    @app.call(env).tap do
      statsd.increment "stripe.#{resource_from(env)}.calls"
    end
  rescue
    statsd.increment "stripe.#{resource_from(env)}.errors"
    raise
  end
end

[sax]

Another use for this would be to track API call time from the client side, for instance to report average request time and percentiles.

[brandur]

+1! Being allowed access to some of the underlying utilities for a library like this one is key to being able to properly tweak behavior and get a reasonable level of visibility. A couple loosely related notes:

• stripe-ruby is currently using rest-client. It's okay, but lacks some of the more sophisticated features of more modern Ruby HTTP libraries like Faraday (e.g. reasonable proxy support as described in No option for http_proxy #124).
• There seem to be some basic niceties that are missing right now which could stand to be improved. e.g. connection pooling.

My own preference here would be to allow a client to be handed to stripe-ruby by a user, which would allow them to configure the client exactly as required and allow more advanced configuration like client middleware. It might make sense to upgrade some of the gem's HTTP internals while making this change.

I'm going to try and tackle some of the lower hanging fruit here first, but will circle back around and write a patch for this.

/cc @kyleconroy

[brandur]

One possible complication as described in #124 is that due to the general inability to configure the gem, it seems that some users have started patching RestClient directly (e.g. RestClient.proxy = "..."). Changing out the internal HTTP client could be considered a breaking change.

[kyleconroy]

We shouldn't be afraid to make breaking changes if we can dramatically improve the usability of the SDK. If we wanted to go down this route, we would need to define an interface we expect for an HTTP client, which shouldn't be too hard, but will take some thought.

I would love for people to not have to use RestClient.

[brandur]

We shouldn't be afraid to make breaking changes if we can dramatically improve the usability of the SDK.

Cool! This one is especially complicated though because removing the use of RestClient wouldn't technically be a breaking change in the sense that our API would stay the same. Any users who were relying on the internal implementation may be broken though.

[brandur]

Created umbrella issue #313 and am closing this in favor of that.