[Messenger] Added ErrorDetailsStamp #32904

TimoBakx · 2019-08-03T14:32:37Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	yes
Deprecations?	no
Tests pass?	yes
Fixed tickets	#32311
License	MIT
Doc PR	No doc changes are needed

#SymfonyHackday

This PR is part of the work started in #32341. That PR has a workaround for showing exceptions added to a previous retry. This PR stores error messages in a separate stamp, so they're more easily accessed.

I also added the exceptionClass as a separate string (independant of FlattenException), so that information is always available, even if the trace is not (due to FlattenException not being available).

Duplicated exceptions (compared to the last one) are not stored separately.

Questions:

Should we limit the total amount of exceptions (remove the oldest when adding a new one)?
- Yes, but in a new PR
The current implementation adds this stamp in the Worker instead of the listeners to prevent duplicate code (due to the immutability of the envelope in the event). Is this the proper way to do this?
- No, create a new listener and a way to add stamps to the envelope inside the event.
When adding details of a HandlerFailedException, should we add a stamp per wrapped Throwable? There can be multiple errors wrapped by a single HandlerFailedException.
- Yes, but in a later PR

Todo:

only add new information if it differs from the previous exception
add deprecations
fall back to old stamp data if no new stamp is available
rebase and retarget to master
update CHANGELOG.md
check for docs PR

BC Breaks:
When this PR is merged, RedeliveryStamps will no longer be populated with exception data. Any implementations that use RedeliveryStamp::getExceptionMessage() or RedeliveryStamp::getFlattenedException() will receive an empty string or null respectively for stamps added after this update. They should rely on ErrorDetailsStamp instead.

New stuff:

New stamp ErrorDetailsStamp.
New event subscriber AddErrorDetailsStampListener.
New method AbstractWorkerMessageEvent::addStamps.

src/Symfony/Component/Messenger/Stamp/RedeliveryStamp.php

src/Symfony/Component/Messenger/Event/AbstractWorkerMessageEvent.php

src/Symfony/Component/Messenger/Stamp/RedeliveryStamp.php

Tobion · 2020-02-09T12:24:31Z

src/Symfony/Component/Messenger/Tests/Transport/AmqpExt/AmqpExtIntegrationTest.php

@@ -75,6 +75,10 @@ public function testItSendsAndReceivesMessages()
        $this->assertEmpty(iterator_to_array($receiver->get()));
    }

+    /**
+     * @group legacy
+     * ^ for now, deprecation errors are thrown during serialization.


how can people prevent the deprecation from happening?

Currently, the deprecation is being triggered by a fallback. This fallback is used in case people have messages in the queue while updating their codebase to a version that includes this PR.
Newer messages that are retried after this PR is downloaded will no longer trigger this deprecation.

Tobion · 2020-02-09T12:41:57Z

Also I think the error details stamp should be added by each retry (not just when failing at the end).
So when each retry has the same error, it will not duplicate the error details due to the new equals check. But when each retry has a different problem it would be nice to see that actually. (We should also limit this to say 3 different errors so the message does not grow too big again).

When we have the retry errors, the failed command should be able to list all the errors (not just diplay the last one).

With this feature added, the PR would actually give a nice DX improvement. Currently it's just a small refactoring without much gain for users.

TimoBakx · 2020-02-09T15:24:47Z

Hi @Tobion, thank you for your review.

The idea for this PR is that it paves the way for a few follow-up PRs that include more changes and DX improvement (like a limit on the amount of error stamps). In order to keep this PR as small as possible (to make reviewing easier), @weaverryan and I decided to break the total work up in several separate PRs.

I'll look over your suggestions and rebase and make changes this week.

weaverryan

Sorry for the MASSIVE delay on reviewing Timo. It's really nice work - I've left some comments.

But, to make sure that we're fully thinking of the current status versus the improved status if this PR is merged, can you make the direct case for why we should merge this? What I mean is, I think (once we fully got #32341 done) that this PR makes getting the error information off of the Envelope easier because it's not attached to the RedeliveryStamp. So instead of looking through all the RedeliveryStamps to find the most recent one with exception information, you can now look directly at the ErrorDetailsStamp: any of these will always contain the error details. In other words, it's kind of a cleanup / convenience. But I think (and this where I need your help), it doesn't solve any huge thing. If I'm right, with some many changes originally in this area, that wasn't obvious to me until now.

Thanks :)

src/Symfony/Component/Messenger/Event/AbstractWorkerMessageEvent.php

src/Symfony/Component/Messenger/Command/AbstractFailedMessagesCommand.php

src/Symfony/Component/Messenger/Command/FailedMessagesShowCommand.php

src/Symfony/Component/Messenger/Stamp/ErrorDetailsStamp.php

Jean85 · 2020-07-07T09:57:00Z

This is a really nice improvement! Any way to push this forward? It would be helpful to port this into 5.x as a BC layer, and be able to start relying on it.

TimoBakx · 2020-07-07T10:19:56Z

Hi @weaverryan & @Jean85, thanks for the review and kind words.
I'm sorry I haven't had much time to work on this after covid (work suddenly got a bit stressful, causing me to want to spend less time programming in my free time).

I hope I have time soon to rebase and do some more work on this.

@weaverryan The main reason for this PR is to decouple the retries (how many times have we done this and when should we stop trying?) from the errors (what went wrong?). We saw some issues where the errors caused problems with the envelope getting too large (especially when the retry number was high). Decoupling this can help us in the future by for example limiting the maximum amount of error stamps (by removing the oldest) without losing information about retries.

Right now, this PR also includes ignoring duplicated errors (in case the same error as the last one occurs), since it wouldn't add any new information.

Perhaps it would also be easier to hook into the flow of things when these stamps are separated.

fabpot · 2020-09-19T07:21:06Z

@TimoBakx Do you think you will have to time to work on this PR in the near future?

TimoBakx · 2020-09-19T07:56:28Z

@fabpot Yes, I should have time in the weekends of weeks 39, 40 and 41.

TimoBakx · 2020-09-25T11:49:59Z

@Tobion, @weaverryan I have rebased the code with the recent master branch and included your suggestions.

Edit: AppVeyor fails due to an unrelated test.

weaverryan

I left minor comments that we do need to handle, but this looks great!

As Timo said, it's just a nice, decoupling of the "retry" information from the "failure information". I love it!

src/Symfony/Component/Messenger/Event/AbstractWorkerMessageEvent.php

src/Symfony/Component/Messenger/Event/WorkerMessageFailedEvent.php

src/Symfony/Component/Messenger/CHANGELOG.md

src/Symfony/Component/Messenger/Command/AbstractFailedMessagesCommand.php

weaverryan

This is ready!

fabpot · 2020-10-02T05:56:46Z

Thank you @TimoBakx.

…from the Messenger component as event subscriber (Jeroen Noten) This PR was merged into the 5.2-dev branch. Discussion ---------- [FrameworkBundle] Register AddErrorDetailsStampListener from the Messenger component as event subscriber | Q | A | ------------- | --- | Branch? | 5.x | Bug fix? | yes | New feature? | no | Deprecations? | no | License | MIT This is a fix for a bug in version 5.2-BETA3. In #32904, adding the error details to a failed message in the Messenger component was moved to a separate listener. However, this listener is not registered in the FrameworkBundle, resulting in no error details stored at all (when using the Symfony skeleton). This PR adds that missing registration. Commits ------- deda2ac [FrameworkBundle] Register AddErrorDetailsStampListener from the Messenger component as event subscriber

Tobion · 2021-04-28T13:28:06Z

src/Symfony/Component/Messenger/Command/AbstractFailedMessagesCommand.php

@@ -122,6 +154,23 @@ protected function getReceiver(): ReceiverInterface

    protected function getLastRedeliveryStampWithException(Envelope $envelope): ?RedeliveryStamp
    {
+        if (null === \func_get_args()[1]) {


this method only has one argument. isn't that accessing a non-existing argument which will result in a php warning?

As discussed on Slack, I'll add a new PR in which I replace this check (that causes a warning if \func_get_args()[1] is not set) with a check on \func_num_args.

Tobion · 2021-04-28T13:31:42Z

src/Symfony/Component/Messenger/Stamp/RedeliveryStamp.php

@@ -27,9 +27,33 @@ final class RedeliveryStamp implements StampInterface
    public function __construct(int $retryCount, string $exceptionMessage = null, FlattenException $flattenException = null, \DateTimeInterface $redeliveredAt = null)


the arguments itself have not been deprecated and the order needs to be changed ($redeliveredAt before exceptionMessage and flattenException )

As discussed on Slack, I'll create a new PR to:

Remove the $exceptionMessage parameter

Remove the $flattenException parameter

Add checks on amount of parameters given and type-checks for those parameters to trigger the deprecation warnings.

AdrienBr · 2021-09-06T09:33:12Z

Hi @TimoBakx and thank you for this improvement.

How are we supposed to deal with these deprecations when an exception is thrown while handling a message ?

User Deprecated: Since symfony/messenger 5.2: Using the "getFlattenException()" method of the "Symfony\Component\Messenger\Stamp\RedeliveryStamp" class is deprecated, use the "Symfony\Component\Messenger\Stamp\ErrorDetailsStamp" class instead.

User Deprecated: Since symfony/messenger 5.2: Using the "getExceptionMessage()" method of the "Symfony\Component\Messenger\Stamp\RedeliveryStamp" class is deprecated, use the "Symfony\Component\Messenger\Stamp\ErrorDetailsStamp" class instead.

I'm using amqp transport.

Edit : I use Doctrine Transport for failure. I suspect Doctrine is trying to Normalize the whole envelope of the message. The Normalization process is calling every methods of the objects (Stamps here).

Maybe this issue can be ignored.

Thanks

TimoBakx · 2021-09-06T10:32:05Z

Hi @AdrienBr, thanks for reaching out.

These deprecations warnings can be thrown by internal code and are caused by a backwards compatibility for Symfony 5.x. We kept the older method so that older code that was still relying to the older stamps would still work correctly.

In Symfony 6.0, the older stamps should not be used in this way anymore and the deprecation warning will disappear.

So for now, if you get these deprecation warnings in your project, you can safely ignore them, as long as they're not caused by your own code.

TimoBakx requested a review from sroze as a code owner August 3, 2019 14:32

carsonbot added Status: Needs Review Messenger Feature BC Break labels Aug 3, 2019

TimoBakx mentioned this pull request Aug 3, 2019

[Messenger] Show exceptions after multiple retries #32341

Merged

nicolas-grekas added this to the next milestone Aug 3, 2019

TimoBakx mentioned this pull request Sep 17, 2019

[Messenger] Fix exception message of failed message is dropped on retry #33600

Merged

weaverryan mentioned this pull request Oct 23, 2019

Revert "[Messenger] Fix exception message of failed message is dropped #34082

Merged

TimoBakx changed the title ~~[Messenger] Added FailedMessageErrorDetailsStamp~~ [WIP] [Messenger] Added FailedMessageErrorDetailsStamp Nov 18, 2019

TimoBakx changed the title ~~[WIP] [Messenger] Added FailedMessageErrorDetailsStamp~~ [WIP] [Messenger] Added ErrorDetailsStamp Nov 18, 2019

TimoBakx changed the base branch from 4.4 to master November 23, 2019 15:30

TimoBakx changed the title ~~[WIP] [Messenger] Added ErrorDetailsStamp~~ [Messenger] Added ErrorDetailsStamp Nov 24, 2019

Tobion suggested changes Feb 9, 2020

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Feb 9, 2020

weaverryan reviewed Mar 26, 2020

View reviewed changes

TimoBakx requested a review from Tobion September 25, 2020 11:50

carsonbot removed the Status: Needs Work label Sep 25, 2020

TimoBakx requested a review from weaverryan September 25, 2020 11:50

carsonbot added the Status: Needs Review label Sep 25, 2020

weaverryan approved these changes Oct 1, 2020

View reviewed changes

carsonbot removed the Status: Needs Review label Oct 1, 2020

carsonbot added the Status: Reviewed label Oct 1, 2020

weaverryan mentioned this pull request Oct 1, 2020

[Messenger] dispatch event when a message is retried #36152

Merged

[Messenger] Added FailedMessageErrorDetailsStamp

cd27b86

weaverryan approved these changes Oct 1, 2020

View reviewed changes

fabpot approved these changes Oct 2, 2020

View reviewed changes

fabpot mentioned this pull request Oct 2, 2020

[Messenger] Added ErrorDetailsStamp symfony/symfony-docs#14319

Closed

fabpot merged commit 954009a into symfony:master Oct 2, 2020

nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020

fabpot mentioned this pull request Oct 5, 2020

Release v5.2.0-BETA1 #38417

Merged

jeroennoten mentioned this pull request Nov 1, 2020

[FrameworkBundle] Register AddErrorDetailsStampListener from the Messenger component as event subscriber #38941

Merged

TimoBakx deleted the messenger-add-failedmessageerrordetailsstamp branch November 15, 2020 10:55

Jean85 mentioned this pull request Feb 19, 2021

Fix deprecations facile-it/mongodb-messenger-transport#5

Merged

Tobion reviewed Apr 28, 2021

View reviewed changes

Jean85 mentioned this pull request Jul 7, 2021

RedeliveryStamp loses exception message in SendFailedMessageForRetryListener #42017

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Messenger] Added ErrorDetailsStamp #32904

[Messenger] Added ErrorDetailsStamp #32904

TimoBakx commented Aug 3, 2019 •

edited

Loading

Tobion Feb 9, 2020

TimoBakx Feb 9, 2020

Tobion commented Feb 9, 2020 •

edited

Loading

TimoBakx commented Feb 9, 2020

weaverryan left a comment

Jean85 commented Jul 7, 2020

TimoBakx commented Jul 7, 2020 •

edited

Loading

fabpot commented Sep 19, 2020

TimoBakx commented Sep 19, 2020

TimoBakx commented Sep 25, 2020 •

edited

Loading

weaverryan left a comment

weaverryan left a comment

fabpot commented Oct 2, 2020

Tobion Apr 28, 2021

TimoBakx Apr 29, 2021

Tobion Apr 28, 2021

TimoBakx Apr 29, 2021

AdrienBr commented Sep 6, 2021 •

edited

Loading

TimoBakx commented Sep 6, 2021

		@@ -27,9 +27,33 @@ final class RedeliveryStamp implements StampInterface
		public function __construct(int $retryCount, string $exceptionMessage = null, FlattenException $flattenException = null, \DateTimeInterface $redeliveredAt = null)

[Messenger] Added ErrorDetailsStamp #32904

[Messenger] Added ErrorDetailsStamp #32904

Conversation

TimoBakx commented Aug 3, 2019 • edited Loading

Tobion Feb 9, 2020

Choose a reason for hiding this comment

TimoBakx Feb 9, 2020

Choose a reason for hiding this comment

Tobion commented Feb 9, 2020 • edited Loading

TimoBakx commented Feb 9, 2020

weaverryan left a comment

Choose a reason for hiding this comment

Jean85 commented Jul 7, 2020

TimoBakx commented Jul 7, 2020 • edited Loading

fabpot commented Sep 19, 2020

TimoBakx commented Sep 19, 2020

TimoBakx commented Sep 25, 2020 • edited Loading

weaverryan left a comment

Choose a reason for hiding this comment

weaverryan left a comment

Choose a reason for hiding this comment

fabpot commented Oct 2, 2020

Tobion Apr 28, 2021

Choose a reason for hiding this comment

TimoBakx Apr 29, 2021

Choose a reason for hiding this comment

Tobion Apr 28, 2021

Choose a reason for hiding this comment

TimoBakx Apr 29, 2021

Choose a reason for hiding this comment

AdrienBr commented Sep 6, 2021 • edited Loading

TimoBakx commented Sep 6, 2021

TimoBakx commented Aug 3, 2019 •

edited

Loading

Tobion commented Feb 9, 2020 •

edited

Loading

TimoBakx commented Jul 7, 2020 •

edited

Loading

TimoBakx commented Sep 25, 2020 •

edited

Loading

AdrienBr commented Sep 6, 2021 •

edited

Loading