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

Configurable HTTP client #313

Open
brandur opened this issue Oct 2, 2015 · 11 comments
Open

Configurable HTTP client #313

brandur opened this issue Oct 2, 2015 · 11 comments
Labels

Comments

@brandur
Copy link
Contributor

brandur commented Oct 2, 2015

Add a mechanism for configuring an HTTP client that stripe-ruby will use. This is designed to solve such problems as:

This change is pseudo-breaking given that it will make significant changes to the plumbing of stripe-ruby and there's probably a fair number of people who are anchored to the current implementation through monkey patches and the like.

Replaces #124 and #219.

@brandur brandur added the future label Oct 2, 2015
This was referenced Oct 2, 2015
brandur pushed a commit that referenced this issue Mar 14, 2016
This attempts to give some semblance of thread safety to parallel
library requests by pre-assigning a certificate store, the
initialization of which seems to be a weak point for thread safety. Note
that while this does seem to empirically improve the situation, it
doesn't guarantee actual thread safety.

A longer term solution to the problem is probably to assign a per-thread
HTTP client for much stronger guarantees, but this is a little further
out. More discussion on that topic in #313.

Fixes #382.
brandur pushed a commit that referenced this issue Mar 14, 2016
This attempts to give some semblance of thread safety to parallel
library requests by pre-assigning a certificate store, the
initialization of which seems to be a weak point for thread safety. Note
that while this does seem to empirically improve the situation, it
doesn't guarantee actual thread safety.

A longer term solution to the problem is probably to assign a per-thread
HTTP client for much stronger guarantees, but this is a little further
out. More discussion on that topic in #313.

Fixes #382.
@MattWalston
Copy link

Consider making Net::HTTP the default if going down this path.

@ab
Copy link
Contributor

ab commented Sep 25, 2016

For what it's worth, rest-client 2.0 does support per-request proxies. The rest of it is definitely showing its age, though.

Another option to consider might be to vendor whatever HTTP library is used. This would still break monkey patches, but might be faster to implement.

Pros:

  • Avoids dependency hell, like when folks install the unfortunately named rest_client when they want rest-client.
  • You have one gem to upgrade, stripe-ruby, and it brings everything it needs.

Cons:

  • People tend not to upgrade stripe-ruby very often, if at all.
  • Makes it somewhat harder for users to make global changes to all HTTP clients in their app.
  • Ew vendoring

@jensljungblad
Copy link

jensljungblad commented Sep 6, 2019

@brandur I was wondering what the status is of this issue? Perhaps something like what the elasticsearch and chewy gems do could be done here as well: https://github.com/toptal/chewy/blob/ac461c4d954c3f41f5677e216a19a1166d428190/README.md#aws-elastic-search

Basically provide a config option which is a proc that receives the Faraday builder. That way it's easy to enable for instance the instrumentation middleware.

config.faraday = -> (f) { f.request(:instrumentation }

@brandur
Copy link
Contributor Author

brandur commented Sep 6, 2019

@jensljungblad Oof, I hate to say it, but although this briefly became possible around V2 where you could reach into the library's default connection and modify Faraday options, it's now back to not possible as of V5 where we've once again dropped Faraday again. This issue is so old that we forgot to update it.

Rather than a hyper-configurable connection/request, in general we've gone in the direction of opting for global-level configuration like Stripe.proxy, Stripe.open_timeout, Stripe.read_timeout, etc. These has the advantage of being less prone to breakage as we modify the implementation (e.g. we can keep them working even moving from Faraday to Net::HTTP).

Of course those still don't give any particularly easy instrumentation path. Possibly purely coincidentally, we got another request for something similar only an hour or so ago in this comment.

I said in there that I think it might be a good idea to provide some callback "hooks" for various core operations that users could hook into. I haven't done anymore work into looking into that, but given that these days I'd prefer not to give access to any of the library's deeper HTTP internals (in case one day we want to say for example, add HTTP/2 support, and thus break everyone again), that sort of system seems like the natural evolution of the original feature described by this ticket.

In the meantime, the public parts of StripeClient like execute_request are likely to stay stable until another major happens, so an interim solution might be to hook into that as the user from the comment above suggested.

@jensljungblad
Copy link

Yeah, if there were some kind of hook around execute_request, that would probably be even better! I agree it's weird to expose the internal HTTP library.

@jensljungblad jensljungblad mentioned this issue Sep 6, 2019
17 tasks
@Senjai
Copy link

Senjai commented Sep 6, 2019

Bringing the context over from another PR here

The interface for execute_request appears to be public, could we make an assumption that the signature of the method will be backwards compatible for minor releases?

Yes, we're pretty careful about backwards compatibility, and this method is considered public "enough" that we we'd only break it on a major.

In general I agree though that the introduction of some kind of hooks into the library would be a good thing. I was thinking about them for use on a retried request so that users could instrument how often this is happening as opposed to it being mostly opaque, but having something on the request lifecycle would be a good idea too IMO. I'm not quite 100% sure yet what these would look like though.

Honestly, I'm not sure if it's relevant to configuring the http client directly, I think it's fine to be opinionated about that.

In terms of adding hooks, we could take a stab at what an implimentation would look like.

I really like what Faraday has done with its instrument module, reproduced here in entirety

# frozen_string_literal: true

module Faraday
  class Request
    # Middleware for instrumenting Requests.
    class Instrumentation < Faraday::Middleware
      # Options class used in Request::Instrumentation class.
      class Options < Faraday::Options.new(:name, :instrumenter)
        # @return [String]
        def name
          self[:name] ||= 'request.faraday'
        end

        # @return [Class]
        def instrumenter
          self[:instrumenter] ||= ActiveSupport::Notifications
        end
      end

      # Instruments requests using Active Support.
      #
      # Measures time spent only for synchronous requests.
      #
      # @example Using ActiveSupport::Notifications to measure time spent
      #   for Faraday requests.
      #   ActiveSupport::Notifications
      #     .subscribe('request.faraday') do |name, starts, ends, _, env|
      #     url = env[:url]
      #     http_method = env[:method].to_s.upcase
      #     duration = ends - starts
      #     $stderr.puts '[%s] %s %s (%.3f s)' %
      #       [url.host, http_method, url.request_uri, duration]
      #   end
      # @param app [#call]
      # @param options [nil, Hash] Options hash
      # @option options [String] :name ('request.faraday')
      #   Name of the instrumenter
      # @option options [Class] :instrumenter (ActiveSupport::Notifications)
      #   Active Support instrumenter class.
      def initialize(app, options = nil)
        super(app)
        @name, @instrumenter = Options.from(options)
                                      .values_at(:name, :instrumenter)
      end

      # @param env [Faraday::Env]
      def call(env)
        @instrumenter.instrument(@name, env) do
          @app.call(env)
        end
      end
    end
  end
end

Besides the object reference to ActiveSupport, we could do something along the lines of

Stripe.instrumentation_handler = ActiveSupport::Notifications

but we wouldn't have to declare activesupport as a dependency. If an instrumentation handler is set, we'd only have to declare the interface we expect it to receive and can use a mock for testing.

Like with Faraday (even though it defaults to AS:N), we could expect whatever is set to this value to respond to #instrument, take a string name for the name of the event, and a set of unstructured data.

We could then define standard instrumentation hooks, and pass in the raw Net::HTTP response in the unstructured data.

Rails does this like so
image

https://edgeguides.rubyonrails.org/active_support_instrumentation.html

That might be overkill, all our team is looking for is request instrumentation which we use for dashboards. But it'd allow us to avoid monkeypatching, and avoid adding dependency on ActiveSupport in the stripe ruby gem, while defining an easy interface that custom integrations can use, and one that works out of the box if the user is using rails or active support directly.

AND we would only emit these events if Stripe.instrumentation_handler is defined.

WDYT? @brandur @jensljungblad

@jensljungblad
Copy link

For reference, this is how the HTTP gem's instrumentation feature is used: https://github.com/httprb/http/wiki/Logging-and-Instrumentation.

@bdewater
Copy link
Contributor

I recently came across WeTransfer's measurometer via one of their other gems which seems close to @Senjai's idea.

@bdewater
Copy link
Contributor

I just opened a PR for this: #870

@ioquatix
Copy link

ioquatix commented Jun 2, 2020

It would be nice if we could plug in async-http.

@remi-stripe
Copy link
Contributor

@ioquatix We don't have short term plans to add support for async-http for now but it's definitely something we've discussed before. Hopefully something we can introduce in the future!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants