Add Stripe client telemetry to request headers

[jameshageman-stripe]

Adds opt-in client telemetry similar to stripe/stripe-node#557, stripe/stripe-go#766, stripe/stripe-ruby#696, etc.

Request metrics are recorded in LiveStripeResponseGetter and stored on a thread safe buffer using ConcurrentLinkedQueue. If the buffer exceeds 100 elements, metrics are dropped. When a request is sent, if the buffer is not empty, one of the metrics is popped off and sent as JSON in the X-Stripe-Client-Telemetry header.

Clients can enable the telemetry using:

Stripe.enableTelemetry = true;

r? @brandur-stripe
cc @dcarney-stripe @akropp-stripe

[devshorts]

Lookin good! made some comments

[jameshageman-stripe]

Updated!
ptal @akropp-stripe @brandur-stripe

[brandur-stripe]

Looks largely good!

My one major objection is that I'd prefer not to pull in a new dependency and establish a new testing convention for this. All the tests around threading are nice, but I'm personally pretty comfortable assuming that ConcurrentLinkedQueue's implementation is correct and isn't going to cause any problems around thread safety.

I'd prefer to just augment verifyRequest to be able to deal with headers, and then just check that we got the telemetry one. If we were going to move everything over to the new testing system, then that would be a totally different story.

Do you have an opinion on this @ob-stripe?

[jameshageman-stripe]

@brandur-stripe

All the tests around threading are nice, but I'm personally pretty comfortable assuming that ConcurrentLinkedQueue's implementation is correct and isn't going to cause any problems around thread safety.

I'd assume the same thing, but I think the purpose of the test is to make sure our logic is thread safe, not ConcurrentLinkedQueue's.

I'd prefer to just augment verifyRequest to be able to deal with headers, and then just check that we got the telemetry one. If we were going to move everything over to the new testing system, then that would be a totally different story.

Makes sense, let me see what I can do!

[brandur-stripe]

I'd assume the same thing, but I think the purpose of the test is to make sure our logic is thread safe, not ConcurrentLinkedQueue's.

Ah, right. I just meant that our logic is a pretty thin shim around the queue — ConcurrentLinkedQueue is doing all the heavy lifting when it comes to guaranteeing thread safety.

[jameshageman-stripe]

@brandur-stripe re:

I'd prefer to just augment verifyRequest to be able to deal with headers, and then just check that we got the telemetry one. If we were going to move everything over to the new testing system, then that would be a totally different story.

I poked around to try to see how I could write the tests using our existing tools, and I ran into the following problem:

verifyRequest verifies calls to the public StripeResponseGetter interface, but the headers are added behind the scenes by the static method LiveStripeResponseGetter::getHeaders. Mockito does not support mocking static code, so trying to spy on the headers will require a non-trivial refactor.

Maybe it's a result of how I modified the LiveStripeResponseGetter, but I think it will be more difficult than it seems to rewrite the new tests using mocks and spies. Do you think we could reconsider adding MockWebServer? We have used the same end-to-end style strategy to test the implementation of client telemetry in stripe-go, stripe-python, and stripe-node.

[ob-stripe]

Mockito does not support mocking static code, so trying to spy on the headers will require a non-trivial refactor.

Yeah, that's the same conclusion I reached back when I wrote verifyRequest. It was good enough for most of our tests, where we mostly want to assert that each resource method sends the correct request (i.e. HTTP verb + path) to the API, but it doesn't let us inspect headers.

I think MockWebServer might be the path of least resistance here, and it's just a dev dependency so it's not too bad.

[brandur-stripe]

Okay, thanks for looking into it at least!

I don't feel too strongly about the new dependency, but if we decide to include it, it'd be nice to have some sort of idea of what our end game is so that we don't end up with half the tests running on one system and half running on the other a year for now. Stated differently: is MockWebServer the direction we want to start moving overall?

But anyway, if OB's happy with it, then LGTM too.

[jameshageman-stripe]

Thanks @brandur-stripe. I'll leave it to you and @ob-stripe to merge this when you're ready.

[brandur-stripe]

Ack. @ob-stripe Mind taking a quick look before we merge?

[ob-stripe]

LGTM!

[ob-stripe]

Released as 7.19.0.