Skip to content
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

[Messenger] Added ErrorDetailsStamp #32904

Merged
merged 1 commit into from Oct 2, 2020
Merged

[Messenger] Added ErrorDetailsStamp #32904

merged 1 commit into from Oct 2, 2020

Conversation

TimoBakx
Copy link
Member

@TimoBakx TimoBakx commented Aug 3, 2019

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.

@TimoBakx TimoBakx requested a review from sroze as a code owner August 3, 2019 14:32
@nicolas-grekas nicolas-grekas added this to the next milestone Aug 3, 2019
@TimoBakx TimoBakx changed the title [Messenger] Added FailedMessageErrorDetailsStamp [WIP] [Messenger] Added FailedMessageErrorDetailsStamp Nov 18, 2019
@TimoBakx TimoBakx changed the title [WIP] [Messenger] Added FailedMessageErrorDetailsStamp [WIP] [Messenger] Added ErrorDetailsStamp Nov 18, 2019
@TimoBakx TimoBakx changed the base branch from 4.4 to master November 23, 2019 15:30
@TimoBakx TimoBakx changed the title [WIP] [Messenger] Added ErrorDetailsStamp [Messenger] Added ErrorDetailsStamp Nov 24, 2019
src/Symfony/Component/Messenger/Stamp/RedeliveryStamp.php Outdated Show resolved Hide resolved
src/Symfony/Component/Messenger/Stamp/RedeliveryStamp.php Outdated Show resolved Hide resolved
@@ -75,6 +75,10 @@ public function testItSendsAndReceivesMessages()
$this->assertEmpty(iterator_to_array($receiver->get()));
}

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

Choose a reason for hiding this comment

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

how can people prevent the deprecation from happening?

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member

Tobion commented Feb 9, 2020

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
Copy link
Member Author

TimoBakx commented Feb 9, 2020

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.

Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

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 :)

@Jean85
Copy link
Contributor

Jean85 commented Jul 7, 2020

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
Copy link
Member Author

TimoBakx commented Jul 7, 2020

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
Copy link
Member

fabpot commented Sep 19, 2020

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

@TimoBakx
Copy link
Member Author

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

@TimoBakx
Copy link
Member Author

TimoBakx commented Sep 25, 2020

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

Edit: AppVeyor fails due to an unrelated test.

Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

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!

Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

This is ready!

@fabpot
Copy link
Member

fabpot commented Oct 2, 2020

Thank you @TimoBakx.

@fabpot fabpot merged commit 954009a into symfony:master Oct 2, 2020
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@fabpot fabpot mentioned this pull request Oct 5, 2020
jderusse added a commit that referenced this pull request Nov 3, 2020
…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
@TimoBakx TimoBakx deleted the messenger-add-failedmessageerrordetailsstamp branch November 15, 2020 10:55
@@ -122,6 +154,23 @@ protected function getReceiver(): ReceiverInterface

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link

AdrienBr commented Sep 6, 2021

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
Copy link
Member Author

TimoBakx commented Sep 6, 2021

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.

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

Successfully merging this pull request may close these issues.

None yet

8 participants