-
Notifications
You must be signed in to change notification settings - Fork 424
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
Fix Stripe Client Telemetry #530
Conversation
aa0e7d6
to
243ad3f
Compare
@jameshageman-stripe You can rebase on the latest master to fix the annoying PyPy3 build issue. Overall this looks good to me, minus the potential thread-safety issue I mentioned in #526 (comment). I think API requests themselves might still work (I just did a simple test and nothing visibly blew up) but threads would probably overwrite each other's metrics if the client instance is shared. |
243ad3f
to
a8995d4
Compare
@ob-stripe We might be fine by just storing |
@ob-stripe @brandur-stripe I've updated the client to store thread local metrics. I stress tested the client (on master) with multithreading and the only thing I saw was the occasional network error (broken pipe or connection reset by peer). These were unpredictable, and could be avoided by setting I don't know how we'd create a thread local http client when the client needs to live on |
Thanks for running those extra threading tests James! This LGTM, but going to leave final okay to OB lest I make another merge faux pas :) cc @ob-stripe (And BTW, may want to "squash and merge" this one unless James rebases.) |
72f4db7
to
f79f602
Compare
@brandur After rebasing over #531 , I'm seeing this exception in my integration test:
Which I find puzzling since @ob-stripe aside from my one failing test, could you review? |
f79f602
to
a52a282
Compare
The broken test should be fixed after #534 is merged. |
Looks like those new integration tests might really help suss out some bugs in the future :) But yep, sorry about the breakage here. We should have #530 in soon. |
a52a282
to
b976801
Compare
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.
Left a couple more minor comments here, but I think we're close to ready to go.
ptal @jameshageman-stripe
"last_request_metrics": self._last_request_metrics().payload() | ||
} | ||
) | ||
|
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.
Just now that I'm taking a second look at this, would you mind moving this into a helper method called something like _add_telemetry_header(headers)
? Just so we have less code in the main body here.
stripe/http_client.py
Outdated
request_duration_ms = _now_ms() - request_start | ||
self._set_last_request_metrics( | ||
RequestMetrics(request_id, request_duration_ms) | ||
) |
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.
And maybe extract this one to something like _store_telemetry(request_start, rheaders)
.
You may want to unwind _last_request_metrics
and _set_last_request_metrics
into the new helpers too.
Done! |
Thanks @jameshageman-stripe! @ob-stripe Going to pull this one in for now, but if you'd like any additional changes, let us know. |
Released as 2.20.3. |
In #518 I erroneously stored request metrics on the
APIRequestor
, as a new instance ofAPIRequestor
is created on each request. This PR moves the tracking of request metrics to the baseHTTPClient
, so that metrics can be stored between requests.This PR depended on #526, which changed the client to reuse the
HTTPClient
between requests in the default case.r? @ob-stripe @brandur-stripe
cc @dcarney-stripe @akropp-stripe