-
Notifications
You must be signed in to change notification settings - Fork 38
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.
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
Add error bundling to help deal with high traffic #93
Conversation
src/Raygun4php/RaygunClient.php
Outdated
$maxBundleSize = min(100, $settings["maxBundleSize"]); | ||
|
||
if($this->bundleErrors) { | ||
$this->path = '/bulk/entries'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The route's now changed to /entries/bulk
, would you mind updating this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
src/Raygun4php/RaygunClient.php
Outdated
)); | ||
|
||
// Flush any leftover messages stored in the session which weren't bundled | ||
if(isset($_SESSION) && isset($_SESSION["raygun_error_bundle"])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If they haven't called session_start()
at this point what effect is there?
- remove session_start - pass settings through from client to bundler
…aygun4php into add-error-bundling
$this->bundler->setBundle($bundle); | ||
|
||
if(count($bundle) === 1) { | ||
$this->Send($bundle[0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are setting the bundle above, then won't sending the error at this point mean duplicate errors being sent to the API? One with the setBundle
and then again when we call the Send
method here?
$this->Send($bundle[0]); | ||
} | ||
else { | ||
$this->SendBundle($bundle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SendBundle
doesn't accept any parameters. Also if we are retrieving it from storage couldn't we just add it to the current bundle and wait for it queued up by subsequent errors?
Perhaps this was intended to send the errors immediately after retrieving them again.
|
||
// Save to disk to disk if possible | ||
if($this->writeToDisk && file_exists($storageFile) && is_writable($storageFile)) { | ||
return file_put_contents($storageFile, "{$message}\n", FILE_APPEND); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if instead of writing to the file each time and then looking up the bundles each time a new request is made we could instead use the register_shutdown_function
to just send the remaining errors in the bundle at the end.
@samuel-holt are you interested in reviving this pull request? |
@robbieaverill Not particularly, I would be interested in adding this bundling functionality but with a different approach, I think this PR is stale and should be deleted |
See #112 for future tracking |
Updates