Refactor raygun client #117

anfly0 · 2019-11-12T12:59:38Z

Hi!
This pull request builds on the changes made in pr #116
Backwards compatibility with 1.x will be broken by this pr!

The purpose of this pr is to move the message transmission implementation, from the RaygunClient to a new separate class.
These changes have the benefit of making any future changes to the transmission mechanism much more straightforward and safer.

As an added bonus, this pr implements two 'transports' built on guzzle.
One async and one sync, thus closing #111

My thinking is that this pr is a solid stepping stone towards releasing 2.0.

Things that are missing or need discussing:

It would probably be nice to have a client factory.
The naming of classes and interfaces, prepend 'Raygun' yes or no?
Might be some dead code left in RaygunClient.
Readme needs updating.

Anyhow, I'm looking forward to hearing what you guys have to say.

robbieaverill

I've added some cursory comments. Some other notes:

breaking changes - all good, this is 2.x-dev already in master
"Raygun" prefixes on existing classes - I think Raygun4php\Client would probably be better than Raygun4php\RaygunClient, especially since you've changed the constructor signature so people's implementations will break obviously anyway, this is the right time to rename classes if you want to do so
"It would probably be nice to have a client factory." - yes, agreed, particularly since you have provided both sync and async transport options

Thanks for the PR!

I'd like to get some feedback from the Raygun team before this gets merged in, but I'm happy to provide some suggestions along the way.

composer.json

robbieaverill · 2019-11-12T17:47:19Z

src/Raygun4php/interfaces/RaygunMessageInterface.php

+{
+    public function build(\Exception $exception): void;
+
+    public function toJson(): string;


I think I've mentioned this before, but perhaps we should use JsonSerializable here?

I mentioned in #116 my reasoning behind toJson() is to not have to deal with the encoding issus solved by toJsonRemoveUnicodeSequences() and removeNullBytes().
It is at mather of taste, but I like it when an object takes care of all aspects of serializing itself.

robbieaverill · 2019-11-12T17:48:14Z

src/Raygun4php/transports/GuzzleAsync.php

+            $this->httpPromises[] = $this->httpClient
+                                 ->requestAsync('POST', '/entries', ['body' => $messageJson]);
+        } catch (TransferException $th) {
+            return false;


Providing some pluggability here somehow would probably be a good thing, this will be hard to debug for people having failed API calls

Yes, that is definitely true.
One idea is to implement PSR-3 LoggerAwareInterface and add appropriate logging

robbieaverill · 2019-11-12T17:49:05Z

src/Raygun4php/transports/GuzzleSync.php

+        }
+
+        $responseCode = $httpResponse->getStatusCode();
+        return $responseCode === 202;


Is 202 always returned from the Raygun API?

No, but 202 is the only one that signifies success.
success = true
fail = false

robbieaverill · 2019-11-12T17:54:31Z

I've merged #116 for you

anfly0 · 2019-11-15T09:59:57Z

@robbieaverill I'm looking into some of the things you commented on. But I don't think I will push any more commits to this pr before the Raygun team has taken a look.
Thank you for the great and quick feedback!

robbieaverill · 2019-11-28T21:25:32Z

cc @MindscapeHQ/external-php-admins for review when you have a chance

samuel-holt

Great work with this, these changes are looking good and the transmission of data a lot more reliable. I've just pointed out a few minor stylistic issues and some questions around how things work. The tests are good, I assume you've tested this with a sample app and it's all working as expected?

There were a couple of items in the PR description which I feel might need addressing:

Might be some dead code left in RaygunClient. Is this still relevant? Ideally any unused code would be deleted
Readme needs updating. Could this be done as part of this PR? Would we need separate docs for v1.x?

samuel-holt · 2019-12-01T23:48:56Z

src/Raygun4php/RaygunMessage.php

+
+    private function toJsonRemoveUnicodeSequences($struct)
+    {
+        return preg_replace_callback("/\\\\u([a-f0-9]{4})/", function ($matches) {


It would good to have a comment explaining exactly what's happening here...

It looks like we're replacing all instances of unicode strings with UTF-8 encoded strings. Is this a standard operation? Why UCS-4LE? Why is the V (unsigned long) format string used?

This is a leftover from version 1, and I'm not really sure of the what's and the why's of this implementation. The change I made was to move this method from the client class to the message class.

Anyhow, there is definitely room for improvements regarding this method, but I think it is a bit out of scope for this pr.

👍 I agree, the improvements to this are out of scope

samuel-holt · 2019-12-01T23:50:49Z

src/Raygun4php/Transports/GuzzleAsync.php

+            $this->httpPromises[] = $this->httpClient
+                                 ->requestAsync('POST', '/entries', ['body' => $messageJson]);
+        } catch (TransferException $th) {
+            return false;


Can we do something besides just returning false here? Could the exception at least be logged if in debug mode?

Yes.
One idea is to implement PSR-3 LoggerAwareInterface and add appropriate logging.
Do you have any other suggestions?

We could create a callback (e.g onTransmissionFailure($th, $message)) which can be defined in the initial RaygunClient setup. That way developers could choose what to do with the exception. If in debug mode we could log the exception - the LoggerAwareInterface looks really good.

samuel-holt · 2019-12-01T23:52:19Z

src/Raygun4php/Transports/GuzzleSync.php

+     * Synchronously transmits the message to the raygun API.
+     *
+     * @param RaygunMessageInterface $message
+     * @return boolean False if request fails or of the response status code is not 202.


/s "... fails or if the response status ..."

samuel-holt · 2019-12-01T23:53:11Z

src/Raygun4php/Transports/GuzzleSync.php

+            $httpResponse = $this->httpClient
+                                 ->request('POST', '/entries', ['body' => $messageJson]);
+        } catch (TransferException $th) {
+            return false;


Same as above, would be good to log something if the request fails

Same as above, PSR-3.

samuel-holt · 2019-12-01T23:54:13Z

tests/RaygunClientTest.php

+        $this->transportMock = $this->getMockBuilder(TransportInterface::class)
+                                    ->setMethods(['transmit'])
+                                    ->getMock();
+


Delete extra space here

samuel-holt · 2019-12-01T23:57:10Z

tests/RaygunClientTest.php

+        $client->SendException(new \Exception('test'));
+        $raygunMessage = $transportStub->getMessage();
+
+        $this->assertJsonMatchesSchemaString($this->jsonSchema, json_decode($raygunMessage->toJson()));


Is this necessary? It looks like it's encoding as JSON, then decoding again: json_decode($raygunMessage->toJson()) - is this to do with the removal of unicode chars and null strings?

Yes, I think it is.
The reason for this is that I want the test to be decoupled from the implementation of RaygunMessage.

The test is essentially asserting that in the client $transport->transmit($message) is called with a message the can produce a valid JSON representation of itself.

👍 Cool, makes sense

samuel-holt · 2019-12-02T00:02:31Z

src/Raygun4php/RaygunClient.php

-    public function removeNullBytes($string)
-    {
-        return str_replace("\0", '', $string);
+        return $this->transport->transmit($message);


Great job deleting this spaghetti code. This is so much better! 😀

samuel-holt · 2019-12-02T00:18:41Z

src/Raygun4php/RaygunClient.php

-     *                                          202 if accepted, 403 if invalid JSON payload
-     * @throws Raygun4PhpException
+     * @return bool Returns true if the transmission attempt is successful.
+     *              However, this does not guarantee that the message is delivered.


Does this mean that it can return true when it has not delivered the data? If it does return true and the message has not been delivered, is there any way to know this (e.g. logging)?

Yes, you could have a case where some async implementation of TransportInterface successfully queues a message for delivery and returns True, but the actual delivery fails for whatever reason. The natural place to log such an event would be in the transport implementation.

anfly0 · 2019-12-02T07:32:43Z

@samuel-holt
Thanks for the review!

I have done some basic testing, and have not noticed any problems.

I rechecked the client class, and there seems to be some dead code left, but I will remove it.

The documentation most definitely needs to be separate from v1.x as the instantiation of the client is completely different. For now, I can add a separate usage example for v2.x.

anfly0 · 2019-12-02T22:32:46Z

Ok, I have pushed some minor changes and synced this branch with the master
This pr is growing and becoming a bit unwieldy.
But I take it that you guys think that this pr is moving the codebase in the right direction.

Maybe this is a good point to merge and open up new issues for:

Logging in the transports
Updating documentation
(Testing)

samuel-holt · 2019-12-04T04:33:47Z

@anfly0 I agree, these things can be done in a separate PR

samuel-holt

👍 Awesome work improving the transport, a much needed refactor. As you mentioned, this PR is getting big and handling the transport logging in a separate PR is a good approach.

robbieaverill

LGTM too, thank you for contributing, and for persisting with us through review!

anfly0 added 17 commits October 28, 2019 22:14

Adds RaygunMessageIntreface

bbabbf9

Adds TransportInterface

901f5f7

Adds autoloader for interfaces

5e9c662

Makes RaygunMessage implement RaygunMessageInterface

c502513

Adds .vscode to .gitignore

7730605

Updates RaygunClient to use transport implementation to send message.

22fa0be

Updates RaygunClient unit-tests broken by refactroing.

6625ec9

Removes unused unit-test

5f97828

Adds guzzle as dependency

e00203b

Adds Guzzle async/sync transport implementations

6bad055

Fixes formatting / style issues

6b7714c

Renames RaygunMessage->Build() to RaygunMessage->build()

272d8d7

Adds missing comments

9490a06

Adds autoloader for test stubs

9b88c42

Adds Transport test stub

1f574ee

Adds Raygun client unit-test

34477b5

Fixes formatting issue

75b5420

robbieaverill reviewed Nov 12, 2019

View reviewed changes

robbieaverill added the enhancement label Nov 12, 2019

Removes unused autoloads

937ace1

samuel-holt suggested changes Dec 2, 2019

View reviewed changes

anfly0 requested a review from samuel-holt December 2, 2019 07:06

anfly0 added 4 commits December 2, 2019 08:57

Removes dead code from RaygunClient and affected tests

6157d9a

Syncs fork

488cd8e

Chages RaygunMessage->build signature to use Throwable

b816060

Fixes typo and formatting

5e36909

Add basic usage example for v2.x client.

3673f82

samuel-holt approved these changes Dec 4, 2019

View reviewed changes

robbieaverill approved these changes Dec 5, 2019

View reviewed changes

robbieaverill merged commit 7c01a9a into MindscapeHQ:master Dec 5, 2019

This was referenced Dec 5, 2019

Use Guzzle instead of cURL #111

Closed

Update for Raygun PHP SDK v2 when it is released silverstripe/silverstripe-raygun#59

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor raygun client #117

Refactor raygun client #117

anfly0 commented Nov 12, 2019

robbieaverill left a comment •

edited

robbieaverill Nov 12, 2019

anfly0 Nov 12, 2019

robbieaverill Nov 12, 2019

anfly0 Nov 12, 2019

robbieaverill Nov 12, 2019

anfly0 Nov 12, 2019 •

edited

robbieaverill commented Nov 12, 2019

anfly0 commented Nov 15, 2019

robbieaverill commented Nov 28, 2019

samuel-holt left a comment

samuel-holt Dec 1, 2019

anfly0 Dec 2, 2019

samuel-holt Dec 4, 2019 •

edited

samuel-holt Dec 1, 2019

anfly0 Dec 2, 2019

samuel-holt Dec 4, 2019

samuel-holt Dec 1, 2019

samuel-holt Dec 1, 2019

anfly0 Dec 2, 2019

samuel-holt Dec 1, 2019

samuel-holt Dec 1, 2019

anfly0 Dec 2, 2019

samuel-holt Dec 4, 2019

samuel-holt Dec 2, 2019

samuel-holt Dec 2, 2019

anfly0 Dec 2, 2019 •

edited

anfly0 commented Dec 2, 2019

anfly0 commented Dec 2, 2019

samuel-holt commented Dec 4, 2019

samuel-holt left a comment

robbieaverill left a comment

Refactor raygun client #117

Refactor raygun client #117

Conversation

anfly0 commented Nov 12, 2019

robbieaverill left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anfly0 Nov 12, 2019 • edited

Choose a reason for hiding this comment

robbieaverill commented Nov 12, 2019

anfly0 commented Nov 15, 2019

robbieaverill commented Nov 28, 2019

samuel-holt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuel-holt Dec 4, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anfly0 Dec 2, 2019 • edited

Choose a reason for hiding this comment

anfly0 commented Dec 2, 2019

anfly0 commented Dec 2, 2019

samuel-holt commented Dec 4, 2019

samuel-holt left a comment

Choose a reason for hiding this comment

robbieaverill left a comment

Choose a reason for hiding this comment

robbieaverill left a comment •

edited

anfly0 Nov 12, 2019 •

edited

samuel-holt Dec 4, 2019 •

edited

anfly0 Dec 2, 2019 •

edited