Add simple instrumentation callback

[bdewater]

We used to insert Faraday::Request::Instrumentation (with a monkey patch) into our Faraday middleware stack to be able to instrument Stripe calls with StatsD. With Faraday being removed in version 5, this required some rework. This commit implements a simple callback system that can be used with any kind of instrumentation library.

Closes #795

[ob-stripe]

Hi @bdewater, thanks for the contribution!

@brandur-stripe is most familiar with the inner workings of stripe-ruby (having written most of it) so I want him to review this just in case he has comments. Unfortunately he's currently OOO so we won't be able to merge this right away. Thanks for your patience!

r? @brandur-stripe

[Senjai]

Hey @brandur-stripe just following up on this one, any thoughts?

[ob-stripe]

Hey @Senjai, Brandur will be back next Monday. Sorry for the delay!

[bdewater]

This has been running in our production environment since earlier today btw, everything works.

[brandur-stripe]

Hey Bart, love this implementation! Thanks a lot for taking the time to put it together. It looks like you modeled it after some prior art Active Support too, which is great.

I think all the broad strokes are basically right, but here are a couple changes that I'd like to see:

1. Just for maximum future compatibility, like Active Support, I think that subscribe should also take the name of an incoming event, so in our case, something like Stripe::Instrumentation.subscribe "request" do .... Of course we only have one event for now, but it's possible we might want to add ones in the future — consider for example "on connect" or "on error" callbacks.

2. Let's deviate from Active Support a little bit by not taking position parameters like |context, response_code, duration, retries| in the block, and instead have a typed "instrumentation event" like:

   class RequestInstrumentationEvent
     attr_accessor :response
     ...
   end
   
   Stripe::Instrumentation.subscribe "request" do |event|
     puts "status: #{event.http.status}"
   end

   The reason is that a class is better for future compatibility. If we wanted to add a new field in the future then with positional parameters we'd have to detect how many arguments the given block took, and have to continue supporting every old argument combination. Active Support also allows this style of invocation, even if it's optional.

3. Can we take RequestLogContext back out of the public interface in favor of denormalizing its fields onto RequestInstrumentationEvent? I'm not too crazy about how logging is currently implemented in StripeClient and want to reserve the right to refactor that class to something better.

4. Let's rename response_code to http_status because we already expose http_status on StripeResponse and it's good for consistency.

5. Let's rename retries to num_retries to be a little more specific.

Anyway, thanks again for all the work! Looking forward to this feature.

Also, cc @ob-stripe -- even if this is Ruby, it may end up setting some precedent for how instrumentation callbacks in other languages end up looking, so we should try to come up with something we're all pretty happy with.

[bdewater]

Hi Brandur, thanks for the feedback! Those all sounds very reasonable. I have to give credit where credit is due, the immediate inspiration was from our Semian gem :)

I currently don't have the bandwidth to incorporate your ideas but I should have time to circle back to this by end of next week/beginning of the week after that.

[brandur-stripe]

Hi Brandur, thanks for the feedback! Those all sounds very reasonable. I have to give credit where credit is due, the immediate inspiration was from our Semian gem :)

@bdewater Ah, didn't notice you were with Shopify at first :)

But sounds good! I think you got all the heavy lifting already, so I think we could get the feedback turned around pretty quickly. Let me know if you need any support. Thanks.

[bdewater]

Hi @brandur-stripe I had some time on flights today 🙂 please have another look.

I incorporated most of your feedback. I wasn't sure how strong you felt about your suggestion to include the original response on the instrumentation event, but after thinking it over I felt that if you need to reach into the response it means something is missing on the event itself.

This PR is heavily based on keeping Shopify's specific version 4.x instrumentation working with 5.x so it's possible it's not covering someone else's use case. But the new event object approach lends itself to extension by others, so that should not stop a person from submitting a PR for this. What do you think?

[brandur-stripe]

Looks amazing @bdewater.

This PR is heavily based on keeping Shopify's specific version 4.x instrumentation working with 5.x so it's possible it's not covering someone else's use case. But the new event object approach lends itself to extension by others, so that should not stop a person from submitting a PR for this. What do you think?

Yep, I think this is actually the preferable approach — it's easy to expose new properties, but it's difficult to take old ones away, so let's start with something fairly minimal and grow as necessary.

Anyway, thanks again for the fine work! Pulling this in.

[brandur-stripe]

Released as 5.9.0.

[Senjai]

Awesome! Thanks for working with us on this one :D