-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
<?php | ||
|
||
namespace Stripe; | ||
|
||
/** | ||
* Class RequestTelemetry | ||
* | ||
* Tracks client request telemetry | ||
* @package Stripe | ||
*/ | ||
class RequestTelemetry | ||
{ | ||
public $requestId; | ||
public $requestDuration; | ||
|
||
public function __construct($requestId, $requestDuration) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. missing |
||
{ | ||
$this->requestId = $requestId; | ||
$this->requestDuration = $requestDuration; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -333,4 +333,14 @@ public static function normalizeId($id) | |
} | ||
return [$id, $params]; | ||
} | ||
|
||
/** | ||
* Returns UNIX timestamp in milliseconds | ||
* | ||
* @return float current time in millis | ||
*/ | ||
public static function currentTimeMillis() | ||
{ | ||
return round(microtime(true) * 1000); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's probably should be casted to int like: |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,111 @@ | ||
<?php | ||
|
||
namespace Stripe; | ||
|
||
class StripeTelemetryTest extends TestCase | ||
{ | ||
const TEST_RESOURCE_ID = 'acct_123'; | ||
const TEST_EXTERNALACCOUNT_ID = 'ba_123'; | ||
const TEST_PERSON_ID = 'person_123'; | ||
|
||
const FAKE_VALID_RESPONSE = '{ | ||
"data": [], | ||
"has_more": false, | ||
"object": "list", | ||
"url": "/v1/accounts" | ||
}'; | ||
|
||
protected function setUp() | ||
{ | ||
parent::setUp(); | ||
|
||
// clear static telemetry data | ||
ApiRequestor::resetTelemetry(); | ||
} | ||
|
||
public function testNoTelemetrySentIfNotEnabled() | ||
{ | ||
$requestheaders = null; | ||
|
||
$stub = $this | ||
->getMockBuilder("HttpClient\ClientInterface") | ||
->setMethods(array('request')) | ||
->getMock(); | ||
|
||
$stub->expects($this->any()) | ||
->method("request") | ||
->with( | ||
$this->anything(), | ||
$this->anything(), | ||
$this->callback(function ($headers) use (&$requestheaders) { | ||
foreach ($headers as $index => $header) { | ||
// capture the requested headers and format back to into an assoc array | ||
$components = explode(": ", $header, 2); | ||
$requestheaders[$components[0]] = $components[1]; | ||
} | ||
|
||
return true; | ||
}), | ||
$this->anything(), | ||
$this->anything() | ||
)->willReturn(array(self::FAKE_VALID_RESPONSE, 200, ["request-id" => "123"])); | ||
|
||
ApiRequestor::setHttpClient($stub); | ||
|
||
// make one request to capture its result | ||
Charge::all(); | ||
$this->assertArrayNotHasKey('X-Stripe-Client-Telemetry', $requestheaders); | ||
|
||
// make another request and verify telemetry isn't sent | ||
Charge::all(); | ||
$this->assertArrayNotHasKey('X-Stripe-Client-Telemetry', $requestheaders); | ||
|
||
ApiRequestor::setHttpClient(null); | ||
} | ||
|
||
public function testTelemetrySetIfEnabled() | ||
{ | ||
Stripe::setEnableTelemetry(true); | ||
|
||
$requestheaders = null; | ||
|
||
$stub = $this | ||
->getMockBuilder("HttpClient\ClientInterface") | ||
->setMethods(array('request')) | ||
->getMock(); | ||
|
||
$stub->expects($this->any()) | ||
->method("request") | ||
->with( | ||
$this->anything(), | ||
$this->anything(), | ||
$this->callback(function ($headers) use (&$requestheaders) { | ||
// capture the requested headers and format back to into an assoc array | ||
foreach ($headers as $index => $header) { | ||
$components = explode(": ", $header, 2); | ||
$requestheaders[$components[0]] = $components[1]; | ||
} | ||
|
||
return true; | ||
}), | ||
$this->anything(), | ||
$this->anything() | ||
)->willReturn(array(self::FAKE_VALID_RESPONSE, 200, ["request-id" => "123"])); | ||
|
||
ApiRequestor::setHttpClient($stub); | ||
|
||
// make one request to capture its result | ||
Charge::all(); | ||
$this->assertArrayNotHasKey('X-Stripe-Client-Telemetry', $requestheaders); | ||
|
||
// make another request to send the previous | ||
Charge::all(); | ||
$this->assertArrayHasKey('X-Stripe-Client-Telemetry', $requestheaders); | ||
|
||
$data = json_decode($requestheaders['X-Stripe-Client-Telemetry'], true); | ||
$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 commentThe reason will be displayed to describe this comment to others. Learn more. As with Ruby, I'd probably put these tests in the I don't feel super strongly about it though. (And thanks for writing tests in the first place!) |
||
} | ||
} |
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 hadfalse
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.