Skip to content
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 HTTP request monitoring callback #764

Merged
merged 1 commit into from
Oct 16, 2019

Conversation

ob-stripe
Copy link
Contributor

r? @rattrayalex-stripe
cc @stripe/api-libraries @suadhuskic

Add support for passing a callback that is called after every HTTP request. This can be used for logging, monitoring, stats, or anything else.

Used like this:

$curl = new CurlClient();
$curl->setRequestStatusCallback(function ($rbody, $rcode, $rheaders, $errno, $message, $willBeRetried, $numRetries) {
  // Do whatever
});
\Stripe\ApiRequestor::setHttpClient($curl);

@suadhuskic This is a bit more complex than what you submitted but also more powerful, since it lets you access more information about the request (e.g. the HTTP status code, the request ID in the headers, etc.). I think it would also work for your use case, what do you think?

@ob-stripe
Copy link
Contributor Author

ptal @rattrayalex-stripe

Copy link
Contributor

@rattrayalex-stripe rattrayalex-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slightly worried about users throwing from within the callback and blocking a retry intentionally or unintentionally, but I don't think it's worth losing sleep over

lib/HttpClient/CurlClient.php Show resolved Hide resolved
@suadhuskic
Copy link

Doesn't seem like a fun usage from a client perspective knowing all network retry logic is on the Stripe class and now i have to manually set this curl client up before attempting my charge.

Currently I have:

   Stripe::setApiKey('magic');
   Stripe::$maxNetworkRetries = 3;

   $paymentIntent = PaymentIntent::create($requestParams, $requestOptions);

This is why my proposal was what it was in #763 to keep the pattern there. Thank you for quickly putting this up!

@ob-stripe
Copy link
Contributor Author

Doesn't seem like a fun usage from a client perspective knowing all network retry logic is on the Stripe class and now i have to manually set this curl client up

That's a fair criticism. The max retries number should really be a property of CurlClient since that's the class actually handling the retries. I want to clean up the interfaces of both the Stripe global class and CurlClient, but that will have to wait for a future major version since it would likely include some non-backwards compatible changes.

@ob-stripe ob-stripe merged commit b078bfd into master Oct 16, 2019
@ob-stripe ob-stripe deleted the ob-callback-retries branch October 16, 2019 00:23
@ob-stripe
Copy link
Contributor Author

Released as 7.4.0.

@jmraker
Copy link

jmraker commented Oct 24, 2019

As a user who alters the CurlClient.php source to add logging it's not quite there. I log the url +parameters of the api call and I don't see a way to do that. It would be nice if it also passed the $opts array.

@ob-stripe
Copy link
Contributor Author

@jmraker Interesting use case! I think it might be more appropriate to have two callbacks: one before the request with the request properties (method, URL, parameters) and one after the request with the response properties (status code, response body, etc.).

I don't want to rush into things though as I'm planning on some major changes to the HTTP client interface. In the meantime, I think a cleaner workaround that would not require you to modify the library's source code would be to subclass CurlClient:

class LoggingCurlClient extends \Stripe\HttpClient\CurlClient
{
  public function request($method, $absUrl, $headers, $params, $hasFile)
  {
    // log request parameters

    list($rbody, $rcode, $rheaders) = parent::request($method, $absUrl, $headers, $params, $hasFile);

    // log response parameters

    return [$rbody, $rcode, $rheaders];
  }
}

$loggingClient = new LoggingCurlClient();
\Stripe\ApiRequestor::setHttpClient($loggingClient);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants