Feature/webhook transfer time #60
Conversation
src/CallWebhookJob.php
Outdated
| ], $body)); | ||
| 'on_stats' => function (TransferStats $stats) { | ||
|
|
||
| $this->response = $stats->getResponse(); |
There was a problem hiding this comment.
Do we have to do this, the response is already captured on line 72, no?
There was a problem hiding this comment.
It is redundant I've removed this and just set the entire TransferStats instead.
src/CallWebhookJob.php
Outdated
| 'on_stats' => function (TransferStats $stats) { | ||
|
|
||
| $this->response = $stats->getResponse(); | ||
| $this->transferTime = $stats->getTransferTime(); |
There was a problem hiding this comment.
Instead of just passing transfertime, let's pass the entire $stats
src/CallWebhookJob.php
Outdated
| @@ -133,6 +141,7 @@ private function dispatchEvent(string $eventClass) | |||
| $this->response, | |||
| $this->errorType, | |||
| $this->errorMessage, | |||
| $this->transferTime, | |||
There was a problem hiding this comment.
This is a breaking change. Move the added option as the last one and make it optional.
… object instead of only the transferTime property
src/CallWebhookJob.php
Outdated
|
|
||
| $this->dispatchEvent(WebhookCallSucceededEvent::class); | ||
| if (! Str::startsWith($response->getStatusCode(), 2)) { |
There was a problem hiding this comment.
Why do we throw the exception in on_stats? The previous code looks cleaner in this regard. Imho we just get the stats and nothing more here.
There was a problem hiding this comment.
I was originally under the impression that stats would not get set since the on_stats hook is called after the responsee
and the event would be dispatched without the stats var set. Anyway, I've reverted and it works the same. I'll be adding test ASAP and updating the branch.
|
The tests are failing for this PR, could you take a look at that? |
|
The code seems fine, but the tests are failing. Can you take a look at that. Could you also add an example of the new functionality in the readme? |
|
Closing due to inactivity. |
This uses the
on_statsrequest options to set the transferTime metric