Capture messenger exceptions #326

emarref · 2020-03-18T21:47:55Z

Errors that occur when handling Messenger messages asynchronously (e.g. background queue) are not captured or flushed to Sentry. This pull request addresses two issues;

Firstly, exceptions thrown during the execution of the Worker class are caught and logged, and therefore never captured by Sentry.

Secondly, captured messages are never flushed to Sentry unless the worker is configured to run with a limit (i.e. --time-limit=3600 or --limit=10).

When a handler fails when executed via the Worker, Messenger dispatches an event. This pull request listens for this WorkerMessageFailedEvent, and captures the exception, then flushes immediately.

Addresses #259 and #811.

Jean85

Thank you a lot for this PR! I surely will have to work a bit to make the CI work. Also, can you add a changelog entry?

I would love to also have an end2end test for this... But I'm not sure it's feasible; I would leave it to the end.

composer.json

src/EventListener/MessengerListener.php

src/DependencyInjection/Configuration.php

Jean85

One last small nitpick remains (plus fixing the CI), great job!

src/DependencyInjection/SentryExtension.php

Co-Authored-By: Alessandro Lai <[email protected]>

…rref/sentry-symfony into feature/capture-messenger-exceptions

Jean85

Thanks you so much for your contribution! I've added some non-code commits, now it's ready to merge!

rogamoore · 2020-03-24T20:29:31Z

Thanks for this PR! :)
Is there an estimate when this will be released?

Jean85 · 2020-03-24T20:39:06Z

I'm waiting for #322 to be merged before preparing the 3.5 release.

B-Galati · 2020-05-05T05:06:49Z

src/EventListener/MessengerListener.php

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


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

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

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.

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

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.

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.

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.

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?

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.

Jean85 · 2020-07-02T10:45:24Z

@emarref is the capture_soft_fails: false working for you? I just enabled it but I'm still receiving errors...

emarref · 2020-07-02T23:36:08Z

Yeah I've just confirmed it's working as expected for me. symfony/symfony:4.4.8, sentry/sentry-symfony:3.5.1, sentry/sdk:2.1.0.

Jean85 · 2020-07-03T09:18:33Z

Ok great. I discovered that the issue is in the fact that willRetry stays false when you use the message:failed:retry command, even if it will be retried later.

Jean85 · 2020-07-03T09:42:36Z

Oh crap, issue found: willRetry is populated in AFTER this listener runs, here: https://github.com/symfony/symfony/blob/5c056c18fc64d187c232ef0f0f0f7a2cc6da4d04/src/Symfony/Component/Messenger/EventListener/SendFailedMessageForRetryListener.php#L54-L58

$ bin/console debug:eve Symfony\\Component\\Messenger\\Event\\WorkerMessageFailedEvent

Registered Listeners for "Symfony\Component\Messenger\Event\WorkerMessageFailedEvent" Event
===========================================================================================

 ------- ---------------------------------------------------------------------------------------------------------- ---------- 
  Order   Callable                                                                                                   Priority  
 ------- ---------------------------------------------------------------------------------------------------------- ---------- 
  #1      Sentry\SentryBundle\EventListener\MessengerListener::onWorkerMessageFailed()                               128       
  #2      Symfony\Component\Messenger\EventListener\SendFailedMessageForRetryListener::onMessageFailed()             100       
  #3      Symfony\Component\Messenger\EventListener\SendFailedMessageToFailureTransportListener::onMessageFailed()   -100      
 ------- ---------------------------------------------------------------------------------------------------------- ----------

@emarref what do you get if you call bin/console debug:eve Symfony\\Component\\Messenger\\Event\\WorkerMessageFailedEvent? Did you customize the priority?

Jean85 · 2020-07-03T11:18:45Z

I've opened #353 to dig further, it seems something changed between 4.3 and 4.4.

emarref · 2020-07-03T23:37:22Z

Did you customize the priority?

No I haven't.

what do you get if you call bin/console debug:eve Symfony\Component\Messenger\Event\WorkerMessageFailedEvent?

"Symfony\Component\Messenger\Event\WorkerMessageFailedEvent" event
------------------------------------------------------------------

 ------- ---------------------------------------------------------------------------------------------------------- ---------- 
  Order   Callable                                                                                                   Priority  
 ------- ---------------------------------------------------------------------------------------------------------- ---------- 
  #1      Sentry\SentryBundle\EventListener\MessengerListener::onWorkerMessageFailed()                               128       
  #2      Symfony\Component\Messenger\EventListener\SendFailedMessageForRetryListener::onMessageFailed()             100       
  #3      Symfony\Bridge\Doctrine\Messenger\DoctrineClearEntityManagerWorkerSubscriber::onWorkerMessageFailed()      0         
  #4      Symfony\Component\Messenger\EventListener\SendFailedMessageToFailureTransportListener::onMessageFailed()   -100      
 ------- ---------------------------------------------------------------------------------------------------------- ----------

I am definitely seeing the error thrown in my handler appearing multiple times in Sentry for a single run. But the listeners don't seem to be in the right order as you've shown. I will take a look when I get more time over the next few days.

Thanks for opening the PR.

emarref added 8 commits March 19, 2020 09:55

Add messenger to dev requirements

e6c1faa

Add subscriber to capture messages thrown in worker

348d629

Add flushable client interface alias

d24bb25

Add listener service and configuration

df4c3e5

Remove messenger listener if messenger isn't used

9c235a1

Support messenger >4.3 when event was introduced

23f4d6b

Use worker_error timeout instead of handler_error

a311892

Add worker_error to ConfigurationTest

9c2f4d2

Jean85 suggested changes Mar 18, 2020

View reviewed changes

composer.json Show resolved Hide resolved

src/EventListener/MessengerListener.php Outdated Show resolved Hide resolved

src/EventListener/MessengerListener.php Show resolved Hide resolved

Jean85 added this to the 3.5 milestone Mar 18, 2020

Jean85 added the Type: Improvement label Mar 18, 2020

emarref added 4 commits March 19, 2020 10:59

Remove messenger dev dependency

d969165

Flush client on message handled event

d6f2db3

Capture messenger soft fails by default

e049ea0

Revert "Remove messenger dev dependency"

f6a4a5f

emarref requested a review from Jean85 March 18, 2020 22:27

emarref added 8 commits March 19, 2020 11:28

Add changelog entry

479941a

Make listener final for consistency

684409b

Flush client on failed message

6e86c23

Add unit test for MessengerListener

a90f2e9

Test HandlerFailedException is unwrapped

3f7e66f

Support for failed message event bc break

c19a54d

User _SERVER in test

a2e5391

Check for retry method

bf834b2

Jean85 suggested changes Mar 19, 2020

View reviewed changes

src/DependencyInjection/Configuration.php Outdated Show resolved Hide resolved

emarref added 5 commits March 20, 2020 09:35

Keep messenger version

ae09a50

Move messenger config option up a level

61d60ad

Add messenger config to root child

5b90e23

Get messenger option from the correct place

d999e47

Call setForRetry only in builder method

88f5437

emarref added 3 commits March 20, 2020 10:12

Remove capture soft fails from parameters

048dae9

Expand messenger configuration

c0d061e

Prophecy 1.7 compatibility

41dbc12

emarref requested a review from Jean85 March 19, 2020 21:50

Jean85 suggested changes Mar 20, 2020

View reviewed changes

src/DependencyInjection/SentryExtension.php Outdated Show resolved Hide resolved

emarref and others added 7 commits March 23, 2020 11:34

Remove messenger from 3.4 install

c2e1306

Skip tests when no messenger available

b1ddb7c

Dynamic configuration test for messenger

ae91616

Run CS fixer

fc29626

Pass configuration as argument

e8d1dd5

Co-Authored-By: Alessandro Lai <[email protected]>

PHPStan tidy

10036b0

Merge branch 'feature/capture-messenger-exceptions' of github.com:ema…

eb3b6bd

…rref/sentry-symfony into feature/capture-messenger-exceptions

emarref requested a review from Jean85 March 22, 2020 23:33

Jean85 added 3 commits March 23, 2020 16:36

Do not mark for a specific release yet

fd8f386

Add thank you note

d8124b6

Add docs to the readme

ce3781e

Jean85 approved these changes Mar 23, 2020

View reviewed changes

Jean85 merged commit f587b90 into getsentry:master Mar 23, 2020

Jean85 mentioned this pull request Apr 14, 2020

New release #330

Closed

B-Galati reviewed May 5, 2020

View reviewed changes

Jean85 mentioned this pull request May 6, 2020

HandlerFailedException could wrap multiple exceptions #338

Closed

Jean85 mentioned this pull request Jul 3, 2020

Messenger: capture_soft_fails:false doesn't work #353

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Capture messenger exceptions #326

Capture messenger exceptions #326

emarref commented Mar 18, 2020

Jean85 left a comment

Jean85 left a comment

Jean85 left a comment

rogamoore commented Mar 24, 2020

Jean85 commented Mar 24, 2020

B-Galati May 5, 2020

Jean85 May 5, 2020

emarref May 5, 2020

B-Galati May 6, 2020

Jean85 May 6, 2020

B-Galati May 6, 2020 •

edited

Loading

emarref May 7, 2020

Jean85 May 7, 2020

emarref May 7, 2020

Jean85 commented Jul 2, 2020

emarref commented Jul 2, 2020

Jean85 commented Jul 3, 2020

Jean85 commented Jul 3, 2020 •

edited

Loading

Jean85 commented Jul 3, 2020

emarref commented Jul 3, 2020 •

edited

Loading

Capture messenger exceptions #326

Capture messenger exceptions #326

Conversation

emarref commented Mar 18, 2020

Jean85 left a comment

Choose a reason for hiding this comment

Jean85 left a comment

Choose a reason for hiding this comment

Jean85 left a comment

Choose a reason for hiding this comment

rogamoore commented Mar 24, 2020

Jean85 commented Mar 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

B-Galati May 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jean85 commented Jul 2, 2020

emarref commented Jul 2, 2020

Jean85 commented Jul 3, 2020

Jean85 commented Jul 3, 2020 • edited Loading

Jean85 commented Jul 3, 2020

emarref commented Jul 3, 2020 • edited Loading

B-Galati May 6, 2020 •

edited

Loading

Jean85 commented Jul 3, 2020 •

edited

Loading

emarref commented Jul 3, 2020 •

edited

Loading