Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Catch all exceptions thrown while sending an event #899

Merged
merged 6 commits into from
Oct 9, 2019

Conversation

HazAT
Copy link
Member

@HazAT HazAT commented Oct 9, 2019

This fixes an issue if Sentry replies with 429 error code that we no longer throw an exception.

PHP Fatal error:  Uncaught Http\Client\Common\Exception\ClientErrorException: TOO MANY REQUESTS in /Users/haza/Projects/demo-projects/php/demo-php/vendor/php-http/client-common/src/Plugin/ErrorPlugin.php:83
Stack trace:
#0 /Users/haza/Projects/demo-projects/php/demo-php/vendor/php-http/client-common/src/Plugin/ErrorPlugin.php(65): Http\Client\Common\Plugin\ErrorPlugin->transformResponseToException(Object(GuzzleHttp\Psr7\Request), Object(Zend\Diactoros\Response))
#1 [internal function]: Http\Client\Common\Plugin\ErrorPlugin->Http\Client\Common\Plugin\{closure}(Object(Zend\Diactoros\Response))
#2 /Users/haza/Projects/demo-projects/php/demo-php/vendor/php-http/curl-client/src/PromiseCore.php(215): call_user_func(Object(Closure), Object(Zend\Diactoros\Response))
#3 /Users/haza/Projects/demo-projects/php/demo-php/vendor/php-http/curl-client/src/MultiRunner.php(99): Http\Client\Curl\PromiseCore->fulfill()
#4 /Users/haza/Projects/demo-projects/php/demo-php/vendor/php-http/curl-client/src/CurlPromise.php(99): Http\Client\Curl\MultiRun in /Users/haza/Projects/demo-projects/php/demo-php/vendor/php-http/client-common/src/Plugin/ErrorPlugin.php on line 83

Fatal error: Uncaught Http\Client\Common\Exception\ClientErrorException: TOO MANY REQUESTS in /Users/haza/Projects/demo-projects/php/demo-php/vendor/php-http/client-common/src/Plugin/ErrorPlugin.php on line 83

Http\Client\Common\Exception\ClientErrorException: TOO MANY REQUESTS in /Users/haza/Projects/demo-projects/php/demo-php/vendor/php-http/client-common/src/Plugin/ErrorPlugin.php on line 83

Call Stack:
    0.9151    3181944   1. Sentry\ErrorHandler->handleException() /Users/haza/Projects/demo-projects/php/demo-php/vendor/sentry/sentry/src/ErrorHandler.php:0

@HazAT HazAT self-assigned this Oct 9, 2019
Makefile Outdated Show resolved Hide resolved
Co-Authored-By: Alessandro Lai <alessandro.lai85@gmail.com>
src/Transport/HttpTransport.php Outdated Show resolved Hide resolved
src/Transport/HttpTransport.php Outdated Show resolved Hide resolved
src/Transport/HttpTransport.php Outdated Show resolved Hide resolved
src/Transport/HttpTransport.php Outdated Show resolved Hide resolved
@ste93cry ste93cry changed the title Fix/http error master Catch all exceptions thrown while sending an event Oct 9, 2019
@ste93cry ste93cry added this to the 2.2 milestone Oct 9, 2019
@ste93cry ste93cry merged commit 61845ad into master Oct 9, 2019
@ste93cry ste93cry deleted the fix/http-error-master branch October 9, 2019 14:24
@helhum
Copy link

helhum commented Oct 11, 2019

Hey folks. From my perspective of being an author of a custom sentry integration, this change is a breaking change.

In the integration I have code like this:

try {
    $hub->captureEvent($payload);
} catch (\Throwable $e) {
    // Avoid hard failure in case connection to sentry failed
    if ($this->fallbackWriter) {
        $this->fallbackWriter->writeLog(
            new LogRecord(
                'Sentry.Writer',
                LogLevel::ERROR,
                'Failed to write to Sentry',
                ['exception' => $e],
                $record->getRequestId()
            )
        );
    }
}

It means that I rely on the exception being throw to be able to handle that case and use a fallback strategy to capture the event, to make sure it is not silently discarded.

With this change, there is no way to detect such failure, except of course, by implementing my own HttpTransport.

Any thoughts?

@ste93cry
Copy link
Collaborator

Technically speaking I think that we can consider this a BC indeed... However, please note that before 2.2.0 we were even more strict about catching and this PR is just going to fix a regression since wait(false) should never throw according to the documentation, but for some reason out of our control it did... 🙅

@helhum
Copy link

helhum commented Oct 11, 2019

@ste93cry You are right, thanks. I missed that my code actually never did what I expected, as requests have been sent on shutdown only before 2.2.0. So In fact, there is no change in behavior here and I'm now onto implementing my own HttpTransport to be able to have a fallback feature in my integration.

Thanks again for the quick response!

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants