Use one Guzzle instance for everything #587
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change unifies the SessionTracker and Client WRT how they use Guzzle. They now both use the HttpClient abstraction, which also allows them to share a Guzzle instance. This means that we can't use the base URI feature of Guzzle anymore because the notify and session endpoints are on separate subdomains. However this isn't a big deal as we POST to the root in most cases anyway; the only exception being the deploy notification, which is '/deploy' BC breaks: - Removal of Client and Configuration 'getSessionClient' This doesn't make sense to keep given the session client is now the same as the notify Guzzle client. Keeping this would mean changes to the "session" client also propagate to the notify client, which could lead to things being delivered to the wrong endpoint - We no longer use the Guzzle base URI for requests This means if a Guzzle client is passed in the constructor with a pre-defined base URI, we will still send requests to the notify URI. Client::setNotifyEndpoint now needs to be called manually instead. This changed because the Guzzle base URI is ambiguous now given that it is shared between notifications and sessions - Removal of SessionTracker::setConfig This was unused internally and doesn't seem to be needed as the configuration can be changed via the Client or Configuration instance itself. Having the posibility for there to be two different Configuration instances could be dangerous Additionally several constants have been deprecated as they are no longer used. Specifically these are: - Client::ENDPOINT This is ambiguous as we have three separate endpoints Use Configuration::NOTIFY_ENDPOINT instead - HttpClient::PAYLOAD_VERSION This is ambiguous as there is a session payload version too Use HttpClient::NOTIFICATION_PAYLOAD_VERSION instead - Report::PAYLOAD_VERSION As above. This was also unused by the notifier Use HttpClient::NOTIFICATION_PAYLOAD_VERSION instead - SessionTracker::$SESSION_PAYLOAD_VERSION Use HttpClient::SESSION_PAYLOAD_VERSION instead Finally, Client::makeGuzzle has been deprecated as it is only used in one place internally to Client and therefore doesn't need to be public
Use HttpClient in SessionTracker
The SessionData middleware is intended to keep counters for the number of handled and unhandled errors that occured in the current request. However it was not actually incrementing the counts, so the would always be reported as '1' or '0' This change ensures that we tell the SessionTracker about the updated data, so the counters are correct There is a small BC break here in that the constructor arguments for SessionData have changed from Client to SessionTracker, but this is constructed internally so shouldn't have a major impact
Fix SessionData counters across multiple errors
This reverses the BC breaks and makes them deprecations instead
This change is only necessary because it's possible to extend the Bugsnag\Client class, which means constructing a SessionTracker is handled by the user. This would then break as the SessionTracker constructor signature has changed, so we can't require a HttpClient to be passed
This also requires bumping the minimum "sebastian/version" version to 1.0.3 because prior to this it would recursively walk up the directory tree until it found a git repository and use the tagged version there. This totally breaks it when installed via dist because it would find our latest tagged release, rather than PHPUnit's version number and so would report totally incorrect versions
This speeds up the test suite by a lot — it now only takes ~5 seconds to run. There's no need for process isolation to be globally enabled as the vast majority of our tests don't need it
GrahamCampbell
suggested changes
Jun 17, 2020
tomlongridge
reviewed
Jul 4, 2020
Co-authored-by: Tom Longridge <tom@bugsnag.com>
tomlongridge
approved these changes
Aug 12, 2020
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Goal
This change unifies the SessionTracker and Client WRT how they use Guzzle. They now both use the HttpClient abstraction, which also allows them to share a Guzzle instance, saving the cost of instantiating Guzzle multiple times and allowing configuration to be shared
This means that we can't use the base URI feature of Guzzle anymore because the notify and session endpoints are on separate subdomains. However we always POST to the root so weren't really taking advantage of this anyway
I've also fixed a bug with
trimOldestSessionswhere it was trimming random sessions because of an unstable sorting function. Now it works as expected and has a test tooThis PR builds on #582, which made similar changes but with BC breaks as we've decided to avoid a major version bump. See also #521
The deprecations are listed in the changelog, but the exact wording needs some work — I'd like to get the PR reviewed first. The majority of these are also quite unlikely to affect many people as they relate to internal constants or protected methods (which are technically part of the public API, but only if you extend the class)
Tests
I've added a bunch of tests to the
SessionTrackerand change the tests around theClientendpointsMost tests are essentially the same as before, so BC should be preserved (at least for the things that are tested)
Linked issues
Related to #582
Related to #521