unixcharles / acme-client

A Ruby client for the letsencrypt's ACME protocol.
MIT License
495 stars 116 forks source link

Ideas for instrumenting requests #167

Closed ryansouza closed 10 months ago

ryansouza commented 5 years ago

I would like to be able to track metrics around and log the http requests being made. Currently brainstorming something like the following pseudo-code, does it seem reasonable and worth me continuing with a PR?

# setting up middleware by accessing the faraday connection
client = Acme::Client.new(...) do |conn| # conn would be a faraday connection
  conn.response :logging, Application.logger
  conn.use AcmeMetricsMiddleware
end
# changes to Acme::Client endpoint methods
class Acme::Client
  def order(url:)
    # include a generic 'endpoint' identifier so middleware can tell what 'kind' of request it is without needing to deduce it from the url
    # could be passed down through faraday's request context https://github.com/lostisland/faraday/blob/master/spec/faraday/request_spec.rb#L86-L98
    response = get(url, endpoint: :order) 
    ...
  end
end
module AcmeMetricsMiddleware
    def initialize(app, options = {})
      super(app)
    end

    def call(env)
      endpoint = env.context[:endpoint]

      @app.call(env).tap do |response|
        Application.prometheus.acme_requests.observe(response.duration, endpoint: endpoint, status: response.status)
      end
    end
end
unixcharles commented 5 years ago

Instrumentation is so application specific that I would like to avoid the library to have any opinion on that.

Allowing the user to pass a block that yield into the new_connection just make sense in general for flexibility.

I'm wondering if we do:

client = Acme::Client.new do {|conn| conn.something }

As user, I would expect block passed to initialize to be called only once, and right away, but in practice this block would get called multiple time, possibly at a later time, because we need different type of connections.

What about passing the configuration as a lambda?

client = Acme::Client.new(..., connection_block: lambda {|conn| conn.something })

Its the same but from a dev ux it just seem less surprising.

For instrumenting, at Shopify we use the statsd-instrument metaprogramming module.

The gist of it would be something like

module AcmeInstrumentation
  def order(*)
    MetricThing.count("acme-client.order")
    MetricThing.measure("acme-client.order") { super }
  end
end
Acme::Client.include(AcmeInstrumentation)

Does that help?

ryansouza commented 5 years ago

I'm fine wrapping the methods that make requests to instrument them like that 👍 My thought was an integrated approach automatically pick up changes to the gem/acme flow but it's feeling pretty settled with V2. It also misses out on access to the response, but I can categorize them as success/failure using a rescue.

I agree with the connection block lambda, I'll PR that when I get to integrating a logger 👍

unixcharles commented 10 months ago

In the newer version you have more control over the client. I think this should solve those use cases.