From 7f6c83cd441b5ed0d7f88281a5b73d97a39bd2a7 Mon Sep 17 00:00:00 2001 From: Steve Hall Date: Wed, 6 May 2020 17:41:43 +0100 Subject: [PATCH 1/5] Alter MessengerListener to use current hub with methods for capturing exception and accessing client. This allows the scope of the current hub to be updated in a prior event listener. --- phpstan-baseline.neon | 5 ++++ src/DependencyInjection/SentryExtension.php | 2 +- src/EventListener/MessengerListener.php | 25 +++++++++-------- src/Resources/config/services.xml | 1 - test/EventListener/MessengerListenerTest.php | 28 ++++++++++++-------- 5 files changed, 35 insertions(+), 26 deletions(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index cebf27f4..c228271c 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -275,6 +275,11 @@ parameters: count: 1 path: test/EventListener/MessengerListenerTest.php + - + message: "#^Property Sentry\\\\SentryBundle\\\\Test\\\\EventListener\\\\MessengerListenerTest\\:\\:\\$currentHub has no typehint specified\\.$#" + count: 1 + path: test/EventListener/MessengerListenerTest.php + - message: "#^Class Symfony\\\\Component\\\\Messenger\\\\Event\\\\WorkerMessageHandledEvent constructor invoked with 3 parameters, 2 required\\.$#" count: 1 diff --git a/src/DependencyInjection/SentryExtension.php b/src/DependencyInjection/SentryExtension.php index da52e01d..a6377019 100644 --- a/src/DependencyInjection/SentryExtension.php +++ b/src/DependencyInjection/SentryExtension.php @@ -174,7 +174,7 @@ private function configureMessengerListener(ContainerBuilder $container, array $ return; } - $container->getDefinition(MessengerListener::class)->setArgument(1, $processedConfiguration['capture_soft_fails']); + $container->getDefinition(MessengerListener::class)->setArgument(0, $processedConfiguration['capture_soft_fails']); } /** diff --git a/src/EventListener/MessengerListener.php b/src/EventListener/MessengerListener.php index bc3d6e68..d6d5a03e 100644 --- a/src/EventListener/MessengerListener.php +++ b/src/EventListener/MessengerListener.php @@ -2,30 +2,23 @@ namespace Sentry\SentryBundle\EventListener; -use Sentry\FlushableClientInterface; +use Sentry\SentrySdk; 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 + * @param bool $captureSoftFails */ - public function __construct(FlushableClientInterface $client, bool $captureSoftFails = true) + public function __construct(bool $captureSoftFails = true) { - $this->client = $client; $this->captureSoftFails = $captureSoftFails; } @@ -46,8 +39,11 @@ public function onWorkerMessageFailed(WorkerMessageFailedEvent $event): void $error = $error->getPrevious(); } - $this->client->captureException($error); - $this->client->flush(); + $hub = SentrySdk::getCurrentHub(); + $hub->captureException($error); + if (method_exists($hub->getClient(), 'flush')) { + $hub->getClient()->flush(); + } } /** @@ -57,6 +53,9 @@ 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(); + $hub = SentrySdk::getCurrentHub(); + if (method_exists($hub->getClient(), 'flush')) { + $hub->getClient()->flush(); + } } } diff --git a/src/Resources/config/services.xml b/src/Resources/config/services.xml index 46324cb0..bc3e4b24 100644 --- a/src/Resources/config/services.xml +++ b/src/Resources/config/services.xml @@ -53,7 +53,6 @@ - diff --git a/test/EventListener/MessengerListenerTest.php b/test/EventListener/MessengerListenerTest.php index 5e85f663..1102e257 100644 --- a/test/EventListener/MessengerListenerTest.php +++ b/test/EventListener/MessengerListenerTest.php @@ -5,6 +5,8 @@ use Sentry\FlushableClientInterface; use Sentry\SentryBundle\EventListener\MessengerListener; use Sentry\SentryBundle\Test\BaseTestCase; +use Sentry\SentrySdk; +use Sentry\State\HubInterface; use Symfony\Component\Messenger\Envelope; use Symfony\Component\Messenger\Event\WorkerMessageFailedEvent; use Symfony\Component\Messenger\Event\WorkerMessageHandledEvent; @@ -14,12 +16,16 @@ class MessengerListenerTest extends BaseTestCase { private $client; + private $currentHub; protected function setUp(): void { parent::setUp(); $this->client = $this->prophesize(FlushableClientInterface::class); + $this->currentHub = $this->prophesize(HubInterface::class); + $this->currentHub->getClient()->willReturn($this->client); + SentrySdk::setCurrentHub($this->currentHub->reveal()); } public function testSoftFailsAreRecorded(): void @@ -30,10 +36,10 @@ public function testSoftFailsAreRecorded(): void $error = new \RuntimeException(); - $this->client->captureException($error)->shouldBeCalled(); + $this->currentHub->captureException($error)->shouldBeCalled(); $this->client->flush()->shouldBeCalled(); - $listener = new MessengerListener($this->client->reveal(), true); + $listener = new MessengerListener(true); $message = (object) ['foo' => 'bar']; $envelope = Envelope::wrap($message); $event = $this->getMessageFailedEvent($envelope, 'receiver', $error, true); @@ -49,10 +55,10 @@ public function testHardFailsAreRecorded(): void $error = new \RuntimeException(); - $this->client->captureException($error)->shouldBeCalled(); + $this->currentHub->captureException($error)->shouldBeCalled(); $this->client->flush()->shouldBeCalled(); - $listener = new MessengerListener($this->client->reveal(), true); + $listener = new MessengerListener(true); $message = (object) ['foo' => 'bar']; $envelope = Envelope::wrap($message); $event = $this->getMessageFailedEvent($envelope, 'receiver', $error, false); @@ -68,10 +74,10 @@ public function testSoftFailsAreNotRecorded(): void $error = new \RuntimeException(); - $this->client->captureException($error)->shouldNotBeCalled(); + $this->currentHub->captureException($error)->shouldNotBeCalled(); $this->client->flush()->shouldNotBeCalled(); - $listener = new MessengerListener($this->client->reveal(), false); + $listener = new MessengerListener(false); $message = (object) ['foo' => 'bar']; $envelope = Envelope::wrap($message); $event = $this->getMessageFailedEvent($envelope, 'receiver', $error, true); @@ -87,10 +93,10 @@ public function testHardFailsAreRecordedWithCaptureSoftDisabled(): void $error = new \RuntimeException(); - $this->client->captureException($error)->shouldBeCalled(); + $this->currentHub->captureException($error)->shouldBeCalled(); $this->client->flush()->shouldBeCalled(); - $listener = new MessengerListener($this->client->reveal(), false); + $listener = new MessengerListener(false); $message = (object) ['foo' => 'bar']; $envelope = Envelope::wrap($message); $event = $this->getMessageFailedEvent($envelope, 'receiver', $error, false); @@ -111,10 +117,10 @@ public function testHandlerFailedExceptionIsUnwrapped(): void $event = $this->getMessageFailedEvent($envelope, 'receiver', $wrappedError, false); - $this->client->captureException($error)->shouldBeCalled(); + $this->currentHub->captureException($error)->shouldBeCalled(); $this->client->flush()->shouldBeCalled(); - $listener = new MessengerListener($this->client->reveal()); + $listener = new MessengerListener(); $listener->onWorkerMessageFailed($event); } @@ -125,7 +131,7 @@ public function testClientIsFlushedWhenMessageHandled(): void } $this->client->flush()->shouldBeCalled(); - $listener = new MessengerListener($this->client->reveal()); + $listener = new MessengerListener(); $message = (object) ['foo' => 'bar']; $envelope = Envelope::wrap($message); From 7a10000d02c59f4609da45cd956920d9cba21474 Mon Sep 17 00:00:00 2001 From: Steve Hall Date: Thu, 7 May 2020 11:11:57 +0100 Subject: [PATCH 2/5] Inject the hub rather than rely upon singleton instance. --- phpstan-baseline.neon | 2 +- src/DependencyInjection/SentryExtension.php | 2 +- src/EventListener/MessengerListener.php | 28 +++++++++++------- src/Resources/config/services.xml | 1 + test/EventListener/MessengerListenerTest.php | 30 +++++++++----------- 5 files changed, 35 insertions(+), 28 deletions(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index c228271c..6248aa51 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -276,7 +276,7 @@ parameters: path: test/EventListener/MessengerListenerTest.php - - message: "#^Property Sentry\\\\SentryBundle\\\\Test\\\\EventListener\\\\MessengerListenerTest\\:\\:\\$currentHub has no typehint specified\\.$#" + message: "#^Property Sentry\\\\SentryBundle\\\\Test\\\\EventListener\\\\MessengerListenerTest\\:\\:\\$hub has no typehint specified\\.$#" count: 1 path: test/EventListener/MessengerListenerTest.php diff --git a/src/DependencyInjection/SentryExtension.php b/src/DependencyInjection/SentryExtension.php index a6377019..da52e01d 100644 --- a/src/DependencyInjection/SentryExtension.php +++ b/src/DependencyInjection/SentryExtension.php @@ -174,7 +174,7 @@ private function configureMessengerListener(ContainerBuilder $container, array $ return; } - $container->getDefinition(MessengerListener::class)->setArgument(0, $processedConfiguration['capture_soft_fails']); + $container->getDefinition(MessengerListener::class)->setArgument(1, $processedConfiguration['capture_soft_fails']); } /** diff --git a/src/EventListener/MessengerListener.php b/src/EventListener/MessengerListener.php index d6d5a03e..4bf01fa4 100644 --- a/src/EventListener/MessengerListener.php +++ b/src/EventListener/MessengerListener.php @@ -2,23 +2,31 @@ namespace Sentry\SentryBundle\EventListener; -use Sentry\SentrySdk; +use Sentry\FlushableClientInterface; +use Sentry\State\HubInterface; use Symfony\Component\Messenger\Event\WorkerMessageFailedEvent; use Symfony\Component\Messenger\Event\WorkerMessageHandledEvent; use Symfony\Component\Messenger\Exception\HandlerFailedException; final class MessengerListener { + /** + * @var HubInterface + */ + private $hub; + /** * @var bool */ private $captureSoftFails; /** - * @param bool $captureSoftFails + * @param HubInterface $hub + * @param bool $captureSoftFails */ - public function __construct(bool $captureSoftFails = true) + public function __construct(HubInterface $hub, bool $captureSoftFails = true) { + $this->hub = $hub; $this->captureSoftFails = $captureSoftFails; } @@ -39,10 +47,10 @@ public function onWorkerMessageFailed(WorkerMessageFailedEvent $event): void $error = $error->getPrevious(); } - $hub = SentrySdk::getCurrentHub(); - $hub->captureException($error); - if (method_exists($hub->getClient(), 'flush')) { - $hub->getClient()->flush(); + $this->hub->captureException($error); + $client = $this->hub->getClient(); + if ($client instanceof FlushableClientInterface) { + $client->flush(); } } @@ -53,9 +61,9 @@ 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. - $hub = SentrySdk::getCurrentHub(); - if (method_exists($hub->getClient(), 'flush')) { - $hub->getClient()->flush(); + $client = $this->hub->getClient(); + if ($client instanceof FlushableClientInterface) { + $client->flush(); } } } diff --git a/src/Resources/config/services.xml b/src/Resources/config/services.xml index bc3e4b24..0da1c1cf 100644 --- a/src/Resources/config/services.xml +++ b/src/Resources/config/services.xml @@ -53,6 +53,7 @@ + diff --git a/test/EventListener/MessengerListenerTest.php b/test/EventListener/MessengerListenerTest.php index 1102e257..a19ca9f0 100644 --- a/test/EventListener/MessengerListenerTest.php +++ b/test/EventListener/MessengerListenerTest.php @@ -5,7 +5,6 @@ use Sentry\FlushableClientInterface; use Sentry\SentryBundle\EventListener\MessengerListener; use Sentry\SentryBundle\Test\BaseTestCase; -use Sentry\SentrySdk; use Sentry\State\HubInterface; use Symfony\Component\Messenger\Envelope; use Symfony\Component\Messenger\Event\WorkerMessageFailedEvent; @@ -16,16 +15,15 @@ class MessengerListenerTest extends BaseTestCase { private $client; - private $currentHub; + private $hub; protected function setUp(): void { parent::setUp(); $this->client = $this->prophesize(FlushableClientInterface::class); - $this->currentHub = $this->prophesize(HubInterface::class); - $this->currentHub->getClient()->willReturn($this->client); - SentrySdk::setCurrentHub($this->currentHub->reveal()); + $this->hub = $this->prophesize(HubInterface::class); + $this->hub->getClient()->willReturn($this->client); } public function testSoftFailsAreRecorded(): void @@ -36,10 +34,10 @@ public function testSoftFailsAreRecorded(): void $error = new \RuntimeException(); - $this->currentHub->captureException($error)->shouldBeCalled(); + $this->hub->captureException($error)->shouldBeCalled(); $this->client->flush()->shouldBeCalled(); - $listener = new MessengerListener(true); + $listener = new MessengerListener($this->hub->reveal(), true); $message = (object) ['foo' => 'bar']; $envelope = Envelope::wrap($message); $event = $this->getMessageFailedEvent($envelope, 'receiver', $error, true); @@ -55,10 +53,10 @@ public function testHardFailsAreRecorded(): void $error = new \RuntimeException(); - $this->currentHub->captureException($error)->shouldBeCalled(); + $this->hub->captureException($error)->shouldBeCalled(); $this->client->flush()->shouldBeCalled(); - $listener = new MessengerListener(true); + $listener = new MessengerListener($this->hub->reveal(), true); $message = (object) ['foo' => 'bar']; $envelope = Envelope::wrap($message); $event = $this->getMessageFailedEvent($envelope, 'receiver', $error, false); @@ -74,10 +72,10 @@ public function testSoftFailsAreNotRecorded(): void $error = new \RuntimeException(); - $this->currentHub->captureException($error)->shouldNotBeCalled(); + $this->hub->captureException($error)->shouldNotBeCalled(); $this->client->flush()->shouldNotBeCalled(); - $listener = new MessengerListener(false); + $listener = new MessengerListener($this->hub->reveal(), false); $message = (object) ['foo' => 'bar']; $envelope = Envelope::wrap($message); $event = $this->getMessageFailedEvent($envelope, 'receiver', $error, true); @@ -93,10 +91,10 @@ public function testHardFailsAreRecordedWithCaptureSoftDisabled(): void $error = new \RuntimeException(); - $this->currentHub->captureException($error)->shouldBeCalled(); + $this->hub->captureException($error)->shouldBeCalled(); $this->client->flush()->shouldBeCalled(); - $listener = new MessengerListener(false); + $listener = new MessengerListener($this->hub->reveal(), false); $message = (object) ['foo' => 'bar']; $envelope = Envelope::wrap($message); $event = $this->getMessageFailedEvent($envelope, 'receiver', $error, false); @@ -117,10 +115,10 @@ public function testHandlerFailedExceptionIsUnwrapped(): void $event = $this->getMessageFailedEvent($envelope, 'receiver', $wrappedError, false); - $this->currentHub->captureException($error)->shouldBeCalled(); + $this->hub->captureException($error)->shouldBeCalled(); $this->client->flush()->shouldBeCalled(); - $listener = new MessengerListener(); + $listener = new MessengerListener($this->hub->reveal()); $listener->onWorkerMessageFailed($event); } @@ -131,7 +129,7 @@ public function testClientIsFlushedWhenMessageHandled(): void } $this->client->flush()->shouldBeCalled(); - $listener = new MessengerListener(); + $listener = new MessengerListener($this->hub->reveal()); $message = (object) ['foo' => 'bar']; $envelope = Envelope::wrap($message); From 6ab1d8868c6dfc337a66c0cd6305ddad977f84d0 Mon Sep 17 00:00:00 2001 From: Steve Hall Date: Thu, 7 May 2020 11:33:53 +0100 Subject: [PATCH 3/5] Correctly typehint and remove phpstan exclusions for the MessengerListenerTest properties. --- phpstan-baseline.neon | 10 ---------- test/EventListener/MessengerListenerTest.php | 2 ++ 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 6248aa51..00067d1e 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -270,16 +270,6 @@ parameters: count: 1 path: test/EventListener/ConsoleListenerTest.php - - - message: "#^Property Sentry\\\\SentryBundle\\\\Test\\\\EventListener\\\\MessengerListenerTest\\:\\:\\$client has no typehint specified\\.$#" - count: 1 - path: test/EventListener/MessengerListenerTest.php - - - - message: "#^Property Sentry\\\\SentryBundle\\\\Test\\\\EventListener\\\\MessengerListenerTest\\:\\:\\$hub has no typehint specified\\.$#" - count: 1 - path: test/EventListener/MessengerListenerTest.php - - message: "#^Class Symfony\\\\Component\\\\Messenger\\\\Event\\\\WorkerMessageHandledEvent constructor invoked with 3 parameters, 2 required\\.$#" count: 1 diff --git a/test/EventListener/MessengerListenerTest.php b/test/EventListener/MessengerListenerTest.php index a19ca9f0..781d076a 100644 --- a/test/EventListener/MessengerListenerTest.php +++ b/test/EventListener/MessengerListenerTest.php @@ -14,7 +14,9 @@ class MessengerListenerTest extends BaseTestCase { + /** @var \Prophecy\Prophecy\ObjectProphecy|FlushableClientInterface */ private $client; + /** @var \Prophecy\Prophecy\ObjectProphecy|HubInterface */ private $hub; protected function setUp(): void From d0579249d26c6d3447eced5d86a17eed0cdd4204 Mon Sep 17 00:00:00 2001 From: Steve Hall Date: Thu, 7 May 2020 13:14:04 +0100 Subject: [PATCH 4/5] Merge changes from #340. --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index dcde3c7b..16fa20f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 multiple events if multiple exceptions are generated in a Messenger Worker context (#340, thanks to @emarref) - ... ## 3.5.0 (2020-05-04) From 77625a88e0de0e49bdbb3dba3af2cff11f2b9986 Mon Sep 17 00:00:00 2001 From: Steve Hall Date: Thu, 7 May 2020 13:22:35 +0100 Subject: [PATCH 5/5] Fix up new test. --- test/EventListener/MessengerListenerTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/EventListener/MessengerListenerTest.php b/test/EventListener/MessengerListenerTest.php index 9526923d..69b14f44 100644 --- a/test/EventListener/MessengerListenerTest.php +++ b/test/EventListener/MessengerListenerTest.php @@ -60,7 +60,7 @@ public function testNonMessengerErrorsAreRecorded(): void $error = new \RuntimeException(); - $this->client->captureException($error)->shouldBeCalled(); + $this->hub->captureException($error)->shouldBeCalled(); $this->client->flush()->shouldBeCalled(); $listener = new MessengerListener($this->hub->reveal(), true);