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.

Sign up for GitHub

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

Use HttpClient in SessionTracker #582

Merged
merged 3 commits into from
Jun 5, 2020
Merged

Use HttpClient in SessionTracker #582

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
484d1ca
Use HttpClient in SessionTracker
imjoehaines Jun 3, 2020
324c6c2
Remove deprecations
imjoehaines Jun 5, 2020
dd2bd2d
Update changelog
imjoehaines Jun 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,41 @@
Changelog
=========

## TBD

### Enhancements

- The Client and SessionTracker now share a single Guzzle instance (#582)

### BC breaks

- Removal of `getSessionClient` method in `Client` and `Configuration` (#582)
This doesn't make sense to keep given the session client is now the same as the notify Guzzle client. Keeping this would either mean changes to the "session" client also propagate to the notify client or changes to this do nothing. Either of which could lead to things being delivered to the wrong endpoint and expected Guzzle changes not being reflected in the actual requests

- We no longer use the Guzzle base URI for requests (#582)
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` (#582)
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 possibility for there to be two different `Configuration` instances could be dangerous as changes may not propagate as expected

- `Client::ENDPOINT` (#582)
This is ambiguous as we have three separate endpoints
Use `Configuration::NOTIFY_ENDPOINT` instead

- `HttpClient::PAYLOAD_VERSION` (#582)
This is ambiguous as there is a session payload version too
Use `HttpClient::NOTIFY_PAYLOAD_VERSION` instead

- `Report::PAYLOAD_VERSION` (#582)
As above. This was also unused by the notifier
Use `HttpClient::NOTIFY_PAYLOAD_VERSION` instead

- `SessionTracker::$SESSION_PAYLOAD_VERSION` (#582)
Use `HttpClient::SESSION_PAYLOAD_VERSION` instead

- `Client::makeGuzzle` has been made private (#582)
Use the `$guzzle` parameter of `Bugsnag\Client::__construct` instead

## 3.21.0 (2020-04-29)

### Enhancements
Expand Down
179 changes: 86 additions & 93 deletions src/Client.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,6 @@

class Client
{
/**
* The default endpoint.
*
* @var string
*/
const ENDPOINT = 'https://notify.bugsnag.com';
imjoehaines marked this conversation as resolved.
Show resolved Hide resolved

/**
* The config instance.
*
Expand Down Expand Up @@ -78,21 +71,34 @@ class Client
*
* If you don't pass in a key, we'll try to read it from the env variables.
*
* @param string|null $apiKey your bugsnag api key
* @param string|null $endpoint your bugsnag endpoint
* @param bool $default if we should register our default callbacks
* @param string|null $apiKey your bugsnag api key
* @param string|null $notifyEndpoint your bugsnag notify endpoint
* @param bool $default if we should register our default callbacks
*
* @return static
*/
public static function make($apiKey = null, $endpoint = null, $defaults = true)
{
// Retrieves environment variables
public static function make(
$apiKey = null,
$notifyEndpoint = null,
$defaults = true
) {
$env = new Env();

$config = new Configuration($apiKey ?: $env->get('BUGSNAG_API_KEY'));
$guzzle = static::makeGuzzle($endpoint ?: $env->get('BUGSNAG_ENDPOINT'));
if ($apiKey === null) {
$apiKey = $env->get('BUGSNAG_API_KEY');
}

$config = new Configuration($apiKey);

$client = new static($config, null, $guzzle);
if ($notifyEndpoint === null) {
$notifyEndpoint = $env->get('BUGSNAG_ENDPOINT');
}

if (is_string($notifyEndpoint)) {
$config->setNotifyEndpoint($notifyEndpoint);
}

$client = new static($config);

if ($defaults) {
$client->registerDefaultCallbacks();
Expand All @@ -102,23 +108,23 @@ public static function make($apiKey = null, $endpoint = null, $defaults = true)
}

/**
* Create a new client instance.
*
* @param \Bugsnag\Configuration $config
* @param \Bugsnag\Request\ResolverInterface|null $resolver
* @param \GuzzleHttp\ClientInterface|null $guzzle
* @param \Bugsnag\Shutdown\ShutdownStrategyInterface|null $shutdownStrategy
*
* @return void
* @param Configuration $config
* @param ResolverInterface|null $resolver
* @param ClientInterface|null $guzzle
* @param ShutdownStrategyInterface|null $shutdownStrategy
*/
public function __construct(Configuration $config, ResolverInterface $resolver = null, ClientInterface $guzzle = null, ShutdownStrategyInterface $shutdownStrategy = null)
{
public function __construct(
Configuration $config,
ResolverInterface $resolver = null,
ClientInterface $guzzle = null,
ShutdownStrategyInterface $shutdownStrategy = null
) {
$this->config = $config;
$this->resolver = $resolver ?: new BasicResolver();
$this->recorder = new Recorder();
$this->pipeline = new Pipeline();
$this->http = new HttpClient($config, $guzzle ?: static::makeGuzzle());
$this->sessionTracker = new SessionTracker($config);
$this->http = new HttpClient($config, $guzzle ?: self::makeGuzzle());
$this->sessionTracker = new SessionTracker($config, $this->http);

$this->registerMiddleware(new NotificationSkipper($config));
$this->registerMiddleware(new BreadcrumbData($this->recorder));
Expand All @@ -130,38 +136,17 @@ public function __construct(Configuration $config, ResolverInterface $resolver =
}

/**
* Make a new guzzle client instance.
*
* @param string|null $base
* @param array $options
*
* @return \GuzzleHttp\ClientInterface
* @return ClientInterface
*/
public static function makeGuzzle($base = null, array $options = [])
private static function makeGuzzle()
{
$key = method_exists(ClientInterface::class, 'request') ? 'base_uri' : 'base_url';

$options[$key] = $base ?: static::ENDPOINT;

imjoehaines marked this conversation as resolved.
Show resolved Hide resolved
if ($path = static::getCaBundlePath()) {
$options['verify'] = $path;
if (version_compare(PHP_VERSION, '5.6.0') >= 0) {
return new GuzzleClient();
}

return new GuzzleClient($options);
}

/**
* Get the ca bundle path if one exists.
*
* @return string|false
*/
protected static function getCaBundlePath()
{
if (version_compare(PHP_VERSION, '5.6.0') >= 0 || !class_exists(CaBundle::class)) {
return false;
}

return realpath(CaBundle::getSystemCaRootBundlePath());
return new GuzzleClient([
'verify' => realpath(CaBundle::getSystemCaRootBundlePath()),
]);
}

/**
Expand Down Expand Up @@ -323,22 +308,6 @@ public function notify(Report $report, callable $callback = null)
}
}

/**
* Notify Bugsnag of a deployment.
*
* @deprecated This function is being deprecated in favour of `build`.
*
* @param string|null $repository the repository from which you are deploying the code
* @param string|null $branch the source control branch from which you are deploying
* @param string|null $revision the source control revision you are currently deploying
*
* @return void
*/
public function deploy($repository = null, $branch = null, $revision = null)
{
$this->build($repository, $revision);
}

/**
* Notify Bugsnag of a build.
*
Expand Down Expand Up @@ -768,23 +737,33 @@ public function shouldIgnoreErrorCode($code)
}

/**
* Set session tracking state and pass in optional guzzle.
* Set notification delivery endpoint.
*
* @param bool $track whether to track sessions
* @param string $endpoint
*
* @return $this
*/
public function setAutoCaptureSessions($track)
public function setNotifyEndpoint($endpoint)
{
$this->config->setAutoCaptureSessions($track);
$this->config->setNotifyEndpoint($endpoint);

return $this;
}

/**
* Get notification delivery endpoint.
*
* @return string
*/
public function getNotifyEndpoint()
{
return $this->config->getNotifyEndpoint();
}

/**
* Set session delivery endpoint.
*
* @param string $endpoint the session endpoint
* @param string $endpoint
*
* @return $this
*/
Expand All @@ -796,27 +775,17 @@ public function setSessionEndpoint($endpoint)
}

/**
* Get the session client.
*
* @return \GuzzleHttp\ClientInterface
*/
public function getSessionClient()
{
return $this->config->getSessionClient();
}

/**
* Whether should be auto-capturing sessions.
* Get session delivery endpoint.
*
* @return bool
* @return string
*/
public function shouldCaptureSessions()
public function getSessionEndpoint()
{
return $this->config->shouldCaptureSessions();
return $this->config->getSessionEndpoint();
}

/**
* Sets the build endpoint.
* Set the build endpoint.
*
* @param string $endpoint the build endpoint
*
Expand All @@ -830,12 +799,36 @@ public function setBuildEndpoint($endpoint)
}

/**
* Returns the build endpoint.
* Get the build endpoint.
*
* @return string
*/
public function getBuildEndpoint()
{
return $this->config->getBuildEndpoint();
}

/**
* Set session tracking state.
*
* @param bool $track whether to track sessions
*
* @return $this
*/
public function setAutoCaptureSessions($track)
{
$this->config->setAutoCaptureSessions($track);

return $this;
}

/**
* Whether should be auto-capturing sessions.
*
* @return bool
*/
public function shouldCaptureSessions()
{
return $this->config->shouldCaptureSessions();
}
}
Loading