-
Notifications
You must be signed in to change notification settings - Fork 850
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
Adding client telemetry headers #549
Conversation
c90b9ef
to
b03f398
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 comments, but looks fine! Sorry about the delayed review.
ptal @akropp-stripe
lib/Stripe.php
Outdated
@@ -46,6 +46,9 @@ class Stripe | |||
// @var int Maximum number of request retries | |||
public static $maxNetworkRetries = 0; | |||
|
|||
// @var boolean Defaults to false. |
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.
Can we expand on this comment a bit to look like the other ones around it? e.g.:
Whether client telemetry is enabled. Defaults to false.
lib/Stripe.php
Outdated
/** | ||
* @return bool Whether client telemetry is enabled | ||
*/ | ||
public static function isEnableClientTelemetry() |
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.
This reads pretty awkward. I think we should either:
- Stick to just a standard
get
pattern likegetEnableClientTelemetry
. This might be the better option just because there are no otheris*
style methods in here. - Invert the name so that this makes more sense: name becomes
clientTelemetryEnabled
so that this could beisClientTelemetryEnabled
.
It'd also be nice to keep this consistent across languages though, and I noticed that in Ruby we didn't have the word "client" and it's just enable_telemetry
.
All in all, my preference would be to (1) copy Ruby, and (2) just continue to use only get*
/set*
, so:
$enableTelemetry
getEnableTelemetry
setEnableTelemetry
lib/Stripe.php
Outdated
} | ||
|
||
/** | ||
* @param bool $enableClientTelemetry Enables client telemetry |
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.
Can we put in one of these comments a description of what this feature does? "Client telemetry" by itself doesn't say anything — let's explain that it's a low-overhead method of timing requests from the client side and transmitting them to Stripe so that we can build accurate API latency profiles (or something like that).
tests/TestCase.php
Outdated
@@ -52,6 +52,7 @@ protected function tearDown() | |||
{ | |||
// Restore original values | |||
Stripe::$apiBase = $this->origApiBase; | |||
Stripe::setEnableClientTelemetry(false); |
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.
I think this would be the default anyway? I probably would have left it out, but it doesn't matter that much. Could be useful if we ever decide to flip the default in the future.
lib/ApiRequestor.php
Outdated
@@ -39,6 +44,28 @@ public function __construct($apiKey = null, $apiBase = null) | |||
$this->_apiBase = $apiBase; | |||
} | |||
|
|||
/** | |||
* @static Creates a telemetry json blob for use in 'X-Stripe-Client-Telemetry' headers |
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.
Can you move the @static
keyword down to its own line. By my reading of the @static
docs, I think it's supposed to look something like this:
/**
* a static function
* @static
*/
function mystaticfunction()
{
...
}
)); | ||
|
||
$result = json_encode($payload); | ||
if ($result != false) { |
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.
I don't think there's any sane circumstances where we'd expect json_encode
to fail is there? I'd probably just return the result directly — in the unlikely event that there is a problem, it'd be nice to find out what it is.
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.
My concern was that IF something did happen, all we'd get in the header is false
, since the return is either json or a boolean. If we had false
then we'd probably have to special case any analysis tooling in stripe to skip that as we'd only be expecting json. I can definitely pass it through, but I didn't think it would be helpful.
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.
Okay fair enough. Could you add a logger line in the failure case like you did below then? IMO it's best to never paper over errors completely.
lib/ApiRequestor.php
Outdated
$rheaders['request-id'], | ||
Util\Util::currentTimeMillis() - $requestStartMs | ||
); | ||
} catch (\Exception $e) { |
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.
Is there any circumstances that you can think of that constructing this basic class would fail? (Or the simple time calculation arithmetic?) I'd just leave out the catch
. As with the above, if this ever was a problem, it'd be better to find out about it.
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.
I was just being extra paranoid
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.
IMO we should still take this one out. Basically the list of things we're doing inside this try
is: memory allocation, a call to round
, a call to microtime
, and subtraction. If any of those things happened to fail it seems like we'd have bigger problems than an uncaught exception.
Being conservative is generally good, but this sort of thing does bear some cost in that it can be somewhat mystifying to future maintainers who are trying to refactor things. Without the benefit of our current context, they'll tend to just cargo cult what's already in here, and every little thing adds a little bit to making changes more time consuming. So if we're ~certain that this is never going to be needed, I'd tend to leave it out.
Thoughts?
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.
Works for me. I just didn't want API calls to fail for some reason if metrics got screwy, but I see your point. Incoming!
$this->assertEquals('123', $data['last_request_metrics']['request_id']); | ||
$this->assertNotNull($data['last_request_metrics']['request_duration_ms']); | ||
|
||
ApiRequestor::setHttpClient(null); |
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.
As with Ruby, I'd probably put these tests in the ApiRequestor
suite because it's really that class that's doing all the heavy lifting.
I don't feel super strongly about it though. (And thanks for writing tests in the first place!)
tests/Stripe/StripeTelemetryTest.php
Outdated
ApiRequestor::resetTelemetry(); | ||
} | ||
|
||
|
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.
Mind just stripping out this extra newline?
b03f398
to
098aa40
Compare
Thanks for the changes @akropp-stripe! Added a few more comments on here, but looks very close. ptal @akropp-stripe |
098aa40
to
b6ac17d
Compare
@brandur-stripe game on! |
@devshorts Thanks! Okay, speaking of hidden exceptions, it looks like removing that
I'm not sure if this is because we need to use a different name for that header (case?), or if it's because we have synthetic requests that don't include a request ID, but either way, we should handle the case of a missing |
b6ac17d
to
893196f
Compare
Heyo! 🙏 for tests! Ok, made sure to check the key exists in the assoc array. Thanks for the multiple rounds, hopefully this is the last one :) @brandur-stripe |
LGTM! |
Released as 6.22.0. |
*/ | ||
public static function currentTimeMillis() | ||
{ | ||
return round(microtime(true) * 1000); |
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.
it's probably should be casted to int like:
return (int) round(microtime(true) * 1000);
public $requestId; | ||
public $requestDuration; | ||
|
||
public function __construct($requestId, $requestDuration) |
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.
missing phpDoc
$requestId
probably must be a string
$requestDuration
must be int
in milliseconds
A few minor changes in response to code review comments.
Add some minor code niceties following up #549
Addressed extra feedback in #561. |
Analogous to stripe/stripe-ruby#696 adding the stripe telemetry header data into the PHP client.