-
Notifications
You must be signed in to change notification settings - Fork 570
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
Add support for telemetry #1513
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this @ob-stripe! As far as I can tell the implementation looks good, and the tests are fantastic.
I added one small note about the concurrent test, but it really is a nitpick more than anything else.
5f91033
to
3838f65
Compare
ptal @jameshageman-stripe |
52ca0c9
to
6b9169c
Compare
The pr tests indicate that both of the two requests sent in the concurrent test that have telemetry are both reporting
Don't suspect that ptal @ob-stripe |
Ah yes, I think there's a potential race condition in the test itself. Incrementing |
6b9169c
to
b688732
Compare
Okay, I've factorized the code for setting up the requests in a ptal @jameshageman-stripe |
@ob-stripe LGTM, thanks! |
r? @brandur-stripe @jameshageman-stripe
Add support for telemetry. The implementation is fairly similar to stripe-java's: stripe/stripe-java#661.