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

Open
wants to merge 1 commit into
base: master
from

Conversation

@TimoBakx
Copy link
Contributor

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 Aug 3, 2019
@TimoBakx TimoBakx force-pushed the TimoBakx:messenger-add-failedmessageerrordetailsstamp branch 2 times, most recently from 06ab7ff to f8d0130 Aug 3, 2019
@nicolas-grekas nicolas-grekas added this to the next milestone Aug 3, 2019
@TimoBakx TimoBakx force-pushed the TimoBakx:messenger-add-failedmessageerrordetailsstamp branch 2 times, most recently from 9afd59b to 98af29b Aug 5, 2019
@TimoBakx TimoBakx changed the title [Messenger] Added FailedMessageErrorDetailsStamp [WIP] [Messenger] Added FailedMessageErrorDetailsStamp Nov 18, 2019
@TimoBakx TimoBakx force-pushed the TimoBakx:messenger-add-failedmessageerrordetailsstamp branch 8 times, most recently from 1af0c50 to 72b2f76 Nov 18, 2019
@TimoBakx TimoBakx changed the title [WIP] [Messenger] Added FailedMessageErrorDetailsStamp [WIP] [Messenger] Added ErrorDetailsStamp Nov 18, 2019
@TimoBakx TimoBakx force-pushed the TimoBakx:messenger-add-failedmessageerrordetailsstamp branch 7 times, most recently from 5cabdc0 to 5bd97d3 Nov 18, 2019
@TimoBakx TimoBakx force-pushed the TimoBakx:messenger-add-failedmessageerrordetailsstamp branch 7 times, most recently from 301734a to 1942394 Nov 23, 2019
@TimoBakx TimoBakx changed the base branch from 4.4 to master Nov 23, 2019
@TimoBakx TimoBakx force-pushed the TimoBakx:messenger-add-failedmessageerrordetailsstamp branch 6 times, most recently from 56141b6 to a7b7ff5 Nov 23, 2019
@TimoBakx TimoBakx changed the title [WIP] [Messenger] Added ErrorDetailsStamp [Messenger] Added ErrorDetailsStamp Nov 24, 2019
@TimoBakx TimoBakx force-pushed the TimoBakx:messenger-add-failedmessageerrordetailsstamp branch from a7b7ff5 to 4824d2d Nov 24, 2019
*/
public function addStamps(StampInterface ...$stamps): void
{
$this->envelope = $this->envelope->with(...$stamps);

This comment has been minimized.

Copy link
@Tobion

Tobion Feb 9, 2020

Member

This means all the events extending from AbstractWorkerMessageEvent can change the envelope. I think it's fine but the code handling events need to fetch the new envelope with getEnvelope before using the old envelope.
See

private function handleMessage(Envelope $envelope, ReceiverInterface $receiver, string $transportName): void
{
$event = new WorkerMessageReceivedEvent($envelope, $transportName);
$this->dispatchEvent($event);
if (!$event->shouldHandle()) {
return;
}
try {
$envelope = $this->bus->dispatch($envelope->with(new ReceivedStamp($transportName), new ConsumedByWorkerStamp()));

This comment has been minimized.

Copy link
@Tobion

Tobion Feb 9, 2020

Member

The same applies to handling WorkerMessageHandledEvent and WorkerMessageFailedEvent

This comment has been minimized.

Copy link
@TimoBakx

TimoBakx Feb 9, 2020

Author Contributor

This is a good point. Should I make the changes for this or should we open this up for a more general discussion?

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

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

This comment has been minimized.

Copy link
@Tobion

Tobion Feb 9, 2020

Member

how can people prevent the deprecation from happening?

This comment has been minimized.

Copy link
@TimoBakx

TimoBakx Feb 9, 2020

Author Contributor

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

This comment has been minimized.

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

This comment has been minimized.

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

@TimoBakx TimoBakx force-pushed the TimoBakx:messenger-add-failedmessageerrordetailsstamp branch 6 times, most recently from 069488b to 30e1db6 Feb 9, 2020
@TimoBakx TimoBakx force-pushed the TimoBakx:messenger-add-failedmessageerrordetailsstamp branch from 30e1db6 to 5770163 Feb 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.