Skip to content

Capture messenger exceptions #326

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

Merged
merged 40 commits into from
Mar 23, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
e6c1faa
Add messenger to dev requirements
emarref Mar 18, 2020
348d629
Add subscriber to capture messages thrown in worker
emarref Mar 18, 2020
d24bb25
Add flushable client interface alias
emarref Mar 18, 2020
df4c3e5
Add listener service and configuration
emarref Mar 18, 2020
9c235a1
Remove messenger listener if messenger isn't used
emarref Mar 18, 2020
23f4d6b
Support messenger >4.3 when event was introduced
emarref Mar 18, 2020
a311892
Use worker_error timeout instead of handler_error
emarref Mar 18, 2020
9c2f4d2
Add worker_error to ConfigurationTest
emarref Mar 18, 2020
d969165
Remove messenger dev dependency
emarref Mar 18, 2020
d6f2db3
Flush client on message handled event
emarref Mar 18, 2020
e049ea0
Capture messenger soft fails by default
emarref Mar 18, 2020
f6a4a5f
Revert "Remove messenger dev dependency"
emarref Mar 18, 2020
479941a
Add changelog entry
emarref Mar 18, 2020
684409b
Make listener final for consistency
emarref Mar 18, 2020
6e86c23
Flush client on failed message
emarref Mar 18, 2020
a90f2e9
Add unit test for MessengerListener
emarref Mar 19, 2020
3f7e66f
Test HandlerFailedException is unwrapped
emarref Mar 19, 2020
c19a54d
Support for failed message event bc break
emarref Mar 19, 2020
a2e5391
User _SERVER in test
emarref Mar 19, 2020
bf834b2
Check for retry method
emarref Mar 19, 2020
ae09a50
Keep messenger version
emarref Mar 19, 2020
61d60ad
Move messenger config option up a level
emarref Mar 19, 2020
5b90e23
Add messenger config to root child
emarref Mar 19, 2020
d999e47
Get messenger option from the correct place
emarref Mar 19, 2020
88f5437
Call setForRetry only in builder method
emarref Mar 19, 2020
08938a0
Rename messenger soft fails parameter
emarref Mar 19, 2020
4956311
Default capture soft fails to true
emarref Mar 19, 2020
048dae9
Remove capture soft fails from parameters
emarref Mar 19, 2020
c0d061e
Expand messenger configuration
emarref Mar 19, 2020
41dbc12
Prophecy 1.7 compatibility
emarref Mar 19, 2020
c2e1306
Remove messenger from 3.4 install
emarref Mar 22, 2020
b1ddb7c
Skip tests when no messenger available
emarref Mar 22, 2020
ae91616
Dynamic configuration test for messenger
emarref Mar 22, 2020
fc29626
Run CS fixer
emarref Mar 22, 2020
e8d1dd5
Pass configuration as argument
emarref Mar 22, 2020
10036b0
PHPStan tidy
emarref Mar 22, 2020
eb3b6bd
Merge branch 'feature/capture-messenger-exceptions' of github.com:ema…
emarref Mar 22, 2020
fd8f386
Do not mark for a specific release yet
Jean85 Mar 23, 2020
d8124b6
Add thank you note
Jean85 Mar 23, 2020
ce3781e
Add docs to the readme
Jean85 Mar 23, 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
14 changes: 9 additions & 5 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,23 +14,27 @@ cache:
before_install:
- |
if [ "$SYMFONY_VERSION" != "" ]; then
sed -ri '/symfony\/monolog-bundle/! s/"symfony\/(.+)": "(.+)"/"symfony\/\1": "'$SYMFONY_VERSION'"/' composer.json;
sed -ri '/symfony\/(monolog-bundle|messenger)/! s/"symfony\/(.+)": "(.+)"/"symfony\/\1": "'$SYMFONY_VERSION'"/' composer.json;
fi;
- |
if [ "$SYMFONY_VERSION" == "3.4.*" ]; then
composer remove --dev symfony/messenger
fi;
- composer self-update
- composer global require hirak/prestissimo

install:
install:
- travis_retry travis_wait composer update --no-interaction --prefer-dist --prefer-stable

script: >-
vendor/bin/phpunit -v --coverage-clover=build/coverage-report.xml
vendor/bin/phpunit -v --coverage-clover=build/coverage-report.xml
&& bash <(curl -s https://codecov.io/bash) -f build/coverage-report.xml

jobs:
include:
- stage: Test
php: 7.1
env:
env:
- SYMFONY_VERSION: 3.4.*
- SYMFONY_DEPRECATIONS_HELPER: disabled
- php: 7.3
Expand All @@ -48,7 +52,7 @@ jobs:
name: PHPStan
script:
- composer phpstan
- script:
- script:
- composer cs-check
name: PHP-CS-Fixer
allow_failures:
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).

## Unreleased
- Capture and flush messages in a Messenger Worker context (#326, thanks to @emarref)
- ...

## 3.4.4 (2020-03-16)
Expand Down
5 changes: 4 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,9 @@ Keep in mind that leaving the `dsn` value empty (or undeclared) in other environ
```yaml
sentry:
dsn: "https://public:[email protected]/1"
messenger:
enabled: true # flushes Sentry messages at the end of each message handling
capture_soft_fails: true # captures exceptions marked for retry too
options:
environment: '%kernel.environment%'
release: '%env(VERSION)%' #your app version
Expand Down Expand Up @@ -165,7 +168,7 @@ The 3.0 version of the bundle uses the newest version (2.x) of the underlying Se

The Sentry 2.0 SDK uses the Unified API, hence it uses the concept of `Scope`s to hold information about the current
state of the app, and attach it to any event that is reported. This bundle has three listeners (`RequestListener`,
`SubRequestListener` and `ConsoleListener`) that adds some easy default information.
`SubRequestListener` and `ConsoleListener`) that adds some easy default information. Since 3.5, a fourth listener has been added to handle the case of Messanger Workers: `MessengerListener`.

Those listeners normally are executed with a priority of `1` to allow easier customization with custom listener, that by
default run with a lower priority of `0`.
Expand Down
3 changes: 2 additions & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,13 @@
"jangregor/phpstan-prophecy": "^0.3.0",
"monolog/monolog": "^1.11||^2.0",
"php-http/mock-client": "^1.0",
"phpstan/phpstan-shim": "^0.11",
"phpstan/phpstan-phpunit": "^0.11",
"phpstan/phpstan-shim": "^0.11",
"phpunit/phpunit": "^7.5||^8.5",
"symfony/browser-kit": "^3.4||^4.0||^5.0",
"symfony/expression-language": "^3.4||^4.0||^5.0",
"symfony/framework-bundle": "^3.4||^4.0||^5.0",
"symfony/messenger": "^4.3||^5.0",
"symfony/monolog-bundle": "^3.4",
"symfony/phpunit-bridge": "^5.0",
"symfony/yaml": "^3.4||^4.0||^5.0"
Expand Down
7 changes: 5 additions & 2 deletions phpstan.neon
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ parameters:
- "/Call to function method_exists.. with 'Symfony.+' and 'getThrowable' will always evaluate to false./"
- '/Class PHPUnit_Framework_TestCase not found/'
- '/Symfony\\Component\\HttpKernel\\Event\\(GetResponse|FilterController)Event not found.$/'
-
-
message: '/Symfony\\Bundle\\FrameworkBundle\\Client/'
path: test/End2End/End2EndTest.php
-
-
message: '/^Call to an undefined method Symfony\\Component\\Config\\Definition\\Builder\\TreeBuilder::root...$/'
path: src/DependencyInjection/Configuration.php
-
Expand All @@ -23,6 +23,9 @@ parameters:
-
message: '/Sentry\\SentryBundle\\EventListener\\SubRequestListenerRequestEvent( not found)?.$/'
path: src/EventListener/SubRequestListener.php
-
message: '/Class Symfony\\Component\\Messenger\\Event\\WorkerMessage[a-zA-Z]*Event constructor invoked with [0-9]+ parameters, [0-9]+ required\.$/'
path: test/EventListener/MessengerListenerTest.php

includes:
- vendor/jangregor/phpstan-prophecy/src/extension.neon
Expand Down
15 changes: 14 additions & 1 deletion src/DependencyInjection/Configuration.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
use Symfony\Component\Config\Definition\Builder\ArrayNodeDefinition;
use Symfony\Component\Config\Definition\Builder\TreeBuilder;
use Symfony\Component\Config\Definition\ConfigurationInterface;
use Symfony\Component\Messenger\MessageBusInterface;

/**
* This is the class that validates and merges configuration from your app/config files
Expand Down Expand Up @@ -40,7 +41,6 @@ public function getConfigTreeBuilder(): TreeBuilder
$rootNode->children()
->booleanNode('register_error_listener')
->defaultTrue();

// Options array (to be passed to Sentry\Options constructor) -- please keep alphabetical order!
$optionsNode = $rootNode->children()
->arrayNode('options')
Expand Down Expand Up @@ -141,6 +141,8 @@ public function getConfigTreeBuilder(): TreeBuilder
->defaultValue(128);
$listenerPriorities->scalarNode('console_error')
->defaultValue(128);
$listenerPriorities->scalarNode('worker_error')
->defaultValue(128);

// Monolog handler configuration
$monologConfiguration = $rootNode->children()
Expand All @@ -160,6 +162,17 @@ public function getConfigTreeBuilder(): TreeBuilder
$errorHandler->booleanNode('bubble')
->defaultTrue();

// Messenger configuration
$messengerConfiguration = $rootNode->children()
->arrayNode('messenger')
->addDefaultsIfNotSet()
->children();

$messengerConfiguration->booleanNode('enabled')
->defaultValue(interface_exists(MessageBusInterface::class));
$messengerConfiguration->booleanNode('capture_soft_fails')
->defaultTrue();

return $treeBuilder;
}

Expand Down
13 changes: 13 additions & 0 deletions src/DependencyInjection/SentryExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
use Sentry\Options;
use Sentry\SentryBundle\ErrorTypesParser;
use Sentry\SentryBundle\EventListener\ErrorListener;
use Sentry\SentryBundle\EventListener\MessengerListener;
use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException;
use Symfony\Component\Config\FileLocator;
use Symfony\Component\DependencyInjection\ContainerBuilder;
Expand Down Expand Up @@ -49,6 +50,7 @@ public function load(array $configs, ContainerBuilder $container): void
}

$this->configureErrorListener($container, $processedConfiguration);
$this->configureMessengerListener($container, $processedConfiguration['messenger']);
$this->configureMonologHandler($container, $processedConfiguration['monolog']);
}

Expand Down Expand Up @@ -160,6 +162,17 @@ private function configureErrorListener(ContainerBuilder $container, array $proc
$this->tagExceptionListener($container);
}

private function configureMessengerListener(ContainerBuilder $container, array $processedConfiguration): void
{
if (! $processedConfiguration['enabled']) {
$container->removeDefinition(MessengerListener::class);

return;
}

$container->getDefinition(MessengerListener::class)->setArgument(1, $processedConfiguration['capture_soft_fails']);
}

/**
* BC layer for Symfony < 4.3
*/
Expand Down
62 changes: 62 additions & 0 deletions src/EventListener/MessengerListener.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
<?php

namespace Sentry\SentryBundle\EventListener;

use Sentry\FlushableClientInterface;
use Symfony\Component\Messenger\Event\WorkerMessageFailedEvent;
use Symfony\Component\Messenger\Event\WorkerMessageHandledEvent;
use Symfony\Component\Messenger\Exception\HandlerFailedException;

final class MessengerListener
{
/**
* @var FlushableClientInterface
*/
private $client;

/**
* @var bool
*/
private $captureSoftFails;

/**
* @param FlushableClientInterface $client
* @param bool $captureSoftFails
*/
public function __construct(FlushableClientInterface $client, bool $captureSoftFails = true)
{
$this->client = $client;
$this->captureSoftFails = $captureSoftFails;
}

/**
* @param WorkerMessageFailedEvent $event
*/
public function onWorkerMessageFailed(WorkerMessageFailedEvent $event): void
{
if (! $this->captureSoftFails && $event->willRetry()) {
// Don't capture soft fails. I.e. those that will be scheduled for retry.
return;
}

$error = $event->getThrowable();

if ($error instanceof HandlerFailedException && null !== $error->getPrevious()) {
// Unwrap the messenger exception to get the original error
$error = $error->getPrevious();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HandlerFailedException handles a collection of exceptions so getting only the previous may lose some information along the way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the use case there? What's the situation where the handler will report multiple exceptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/symfony/messenger/blob/master/Middleware/HandleMessageMiddleware.php#L56-L69

Looks like when a command has multiple handlers, they will all run. Any throwable is caught and the next handler is continued.

So here, we should iterate over $error->getNestedExceptions() and capture them all.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or just capture HandlerFailedException? Not sure about the pros and cons.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since HandlerFailedException is not naturally comprehensible to Sentry, capturing it isn't ideal, it shouldn't work out of the box: the event will not show enough information on the specific handlers nor the specific errors.

Just to be sure that I understood this correctly: multiple handlers will run together only if they are all sync and not async, right? If that's the case, at least it should be a non-so-common use case, so we don't have to rush the patch.

Opening #338 to track this issue.

Copy link

@B-Galati B-Galati May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be sure that I understood this correctly: multiple handlers will run together only if they are all sync and not async, right?

No, I don't think so by reading the code and from what I understand about Messenger. Sync or Async does not matter. The only thing that matter is if there are multiple handlers for a Message or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sync or Async does not matter. The only thing that matter is if there are multiple handlers for a Message or not.

Correct. Each handler for a command will be run either to completion or to an exception. Subsequent handlers will be executed and previous exceptions aggregated and added to the HandlerFailedException. So even if one handler succeeds, another handler may fail and raise the exception.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, maybe I did not explain myself completely. For sync and async I was referring to the transport: if it's sync, the message is handled in the same request/process, and so it requires special handling on Sentry's side. If it's async, it should go through a queue (Rabbit? SQS?) so it will be executed inside a different process, and it's isolated as far as Sentry is concerned.

Does this still hold?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah if a command has multiple handlers, sync or async won’t matter. The handle middleware will execute them all in the same process, catching and aggregating errors as they’re thrown, to be later raised in the handler failed exception.

}

$this->client->captureException($error);
$this->client->flush();
}

/**
* @param WorkerMessageHandledEvent $event
*/
public function onWorkerMessageHandled(WorkerMessageHandledEvent $event): void
{
// Flush normally happens at shutdown... which only happens in the worker if it is run with a lifecycle limit
// such as --time=X or --limit=Y. Flush immediately in a background worker.
$this->client->flush();
}
}
8 changes: 8 additions & 0 deletions src/Resources/config/services.xml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
<factory service="Sentry\ClientBuilderInterface" method="getClient" />
</service>

<service id="Sentry\FlushableClientInterface" alias="Sentry\ClientInterface" public="false"/>

<service id="Sentry\State\HubInterface" class="Sentry\State\Hub" public="false">
<factory class="Sentry\SentryBundle\SentryBundle" method="getCurrentHub" />
<call method="bindClient">
Expand Down Expand Up @@ -50,6 +52,12 @@
<tag name="kernel.event_listener" event="kernel.finish_request" method="onKernelFinishRequest" priority="%sentry.listener_priorities.sub_request%" />
</service>

<service id="Sentry\SentryBundle\EventListener\MessengerListener" class="Sentry\SentryBundle\EventListener\MessengerListener" public="false">
<argument type="service" id="Sentry\FlushableClientInterface" />
<tag name="kernel.event_listener" event="Symfony\Component\Messenger\Event\WorkerMessageFailedEvent" method="onWorkerMessageFailed" priority="%sentry.listener_priorities.worker_error%" />
<tag name="kernel.event_listener" event="Symfony\Component\Messenger\Event\WorkerMessageHandledEvent" method="onWorkerMessageHandled" priority="%sentry.listener_priorities.worker_error%" />
</service>

<service id="Sentry\SentryBundle\Command\SentryTestCommand" class="Sentry\SentryBundle\Command\SentryTestCommand" public="false">
<tag name="console.command" />
</service>
Expand Down
6 changes: 6 additions & 0 deletions test/DependencyInjection/ConfigurationTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
use Sentry\SentryBundle\Test\BaseTestCase;
use Symfony\Component\Config\Definition\Exception\InvalidConfigurationException;
use Symfony\Component\Config\Definition\Processor;
use Symfony\Component\Messenger\MessageBusInterface;

class ConfigurationTest extends BaseTestCase
{
Expand Down Expand Up @@ -50,6 +51,7 @@ public function testConfigurationDefaults(): void
'console' => 1,
'request_error' => 128,
'console_error' => 128,
'worker_error' => 128,
],
'options' => [
'class_serializers' => [],
Expand All @@ -72,6 +74,10 @@ public function testConfigurationDefaults(): void
'bubble' => true,
],
],
'messenger' => [
'enabled' => interface_exists(MessageBusInterface::class),
'capture_soft_fails' => true,
],
];

$this->assertEquals($expectedDefaults, $processed);
Expand Down
12 changes: 12 additions & 0 deletions test/DependencyInjection/SentryExtensionTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use Sentry\Options;
use Sentry\SentryBundle\DependencyInjection\SentryExtension;
use Sentry\SentryBundle\EventListener\ErrorListener;
use Sentry\SentryBundle\EventListener\MessengerListener;
use Sentry\SentryBundle\Test\BaseTestCase;
use Sentry\SentrySdk;
use Sentry\Severity;
Expand Down Expand Up @@ -423,6 +424,17 @@ public function testMonologHandlerIsNotRegistered(): void
$this->assertFalse($container->has(self::MONOLOG_HANDLER_TEST_PUBLIC_ALIAS));
}

public function testMessengerHandlerIsNotRegistered(): void
{
$container = $this->getContainer([
'messenger' => [
'enabled' => false,
],
]);

$this->assertFalse($container->has(MessengerListener::class));
}

private function getContainer(array $configuration = []): Container
{
$containerBuilder = new ContainerBuilder();
Expand Down
Loading