Conversation
The remote notifier config is fetched and cached for 10 minutes.
This will avoid sending errors to the service if the remote config response specifies that errors are disabled.
When set to false the defaults are used and no remote config fetch is attempted.
kyrylo
left a comment
kyrylo
left a comment
There was a problem hiding this comment.
We need to fetch the config every 10 minutes (in background). Currently, unless I am mistaken, we fetch it only once, on launch. Did I read the code correctly?
| protected $remoteConfigURL; | ||
| protected $httpClient; | ||
| protected $tempCache; | ||
| protected $tempCacheFilename = 'airbrake_cached_remote_config.json'; |
There was a problem hiding this comment.
Why do we need to cache remote config? Couldn't we keep the config in memory?
There was a problem hiding this comment.
I really wanted to do that as well. I searched a lot for a way to store data between or outside of requests without much luck. Everything that looked promising as a solution required PHP be compiled with an extra flag or installed with PECL. I couldn't really find anything I could depend on existing so I decided to use a temporary file to cache the remote config.
| return $this->errorConfig; | ||
| } | ||
|
|
||
| protected function newRemoteConfig($projectId) |
There was a problem hiding this comment.
Why does this have to be a function? Now we assign to $remoteConfig twice. I am not sure I am following this.
There was a problem hiding this comment.
my main motivation for having a newRemoteConfig method is that it made things easier to mock in tests.
We can remove the $remoteConfig assignment here though, sorry that was leftover from some local debugging. The body of this method should just be:
return new RemoteConfig($projectId);Since the $remoteConfig variable isn't prefixed with $this-> it will only be available in the method scope and not on the instance.
Co-authored-by: Kyrylo Silin <silin@kyrylo.org>
We don't need to set up or store the noticesURL during construction since we get this information at the time the error occurs with the buildNoticesURL function instead. This ensures we always get the freshest error host to use from the RemoteConfig or when that isn't available just fallback to the default error host of `api.airbrake.io`.
Hi @kyrylo thank you for the review and questions! I had another look at this an it turns out it was useless to set the As for the cache lifecycle here's how it works: When an error is being reported to Airbrake, we ask the remoteConfig for the endpoint to send it to. If the remote config isn't cached or is expired, we fetch and cache it for the next 10 minutes. If the cache is still fresh we read from it. You mention this only getting fetched once on launch but as I understand it through my testing PHP doesn't hold onto instances of objects across requests and it would take extra effort to set up a global instance of |
Co-authored-by: Kyrylo Silin <silin@kyrylo.org>
Co-authored-by: Kyrylo Silin <silin@kyrylo.org>
This makes it easy to update the version everywhere in only one place.
When we build an error we call errorHost() and errorNotifications(), without memoization this would result in two calls to the cache instead of just one.
2 participants
Adds support for fetching the remote notifier config when possible. The remote config feature creates a base for more flexible PHPBrake configuration and features at runtime. The remote config is cached for 10 minutes in a temp file in the system's temp directory. If fetching or caching fails or is unavailable for any reason, the default settings are used.
A few notes on
Airbrake\Notifierinitialization options and the remote config:remoteConfigoption to the initialization of theAirbrake\Notiferthat defaults totrue. If theAirbrake\Notifieris initialized with theremoteConfigset tofalse, the config will not be fetched from the remote and the default endpoint will be used.hostoption is specified withAirbrake\Notifierinitialization that will take precedence over the host fetched from the remote config.