NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Messenger] Allow retries to be logged as warnings#43160

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

jorissteyn wants to merge1 commit intosymfony:5.4fromjorissteyn:feature/messenger-retry-log-severity

Closed

[Messenger] Allow retries to be logged as warnings#43160

jorissteyn wants to merge1 commit intosymfony:5.4fromjorissteyn:feature/messenger-retry-log-severity

Conversation

Copy link

jorissteyn commentedSep 24, 2021•
edited
Loading

This commit introduces a marker interface for exceptions to allow retries to be logged as warnings instead of the default error severity.

This behaviour is already implemented for recoverable exceptions, but those are retried ad infinitum regardless of the retry strategy. With the new marker interface, it's possible to log exceptions as warnings while still using the retry strategy.

The RecoverableExceptionInterface now extends this interface to achieve the existing behaviour using the new more flexible approach.

| Q             | A| ------------- | ---| Branch?       | 5.4| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Tickets       | Related to problem described in comments of https://github.com/symfony/messenger/pull/15| License       | MIT| Doc PR        | https://github.com/symfony/symfony-docs/pull/15844

jorissteyn requested a review fromsroze as acode owner

September 24, 2021 09:14

carsonbot added Status: Needs Review Feature Messenger labels

Sep 24, 2021

carsonbot added this to the5.4 milestone

Sep 24, 2021

Copy link

carsonbot commentedSep 24, 2021

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (seehttps://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://symfony.com/releases)
Features and deprecations must be submitted against the 5.4 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

This was referencedSep 24, 2021

[Messenger] Allow retries to be logged as warningssymfony/messenger#16

Closed

[Messenger] Document new log severity marker interfacesymfony/symfony-docs#15844

Closed

Take in consideration max_retries setting when you throw a RecoverableMessageHandlingExceptionsymfony/messenger#15

Closed

jorissteyn force-pushed thefeature/messenger-retry-log-severity branch 2 times, most recently from4afd267 tob8eec9dCompare

September 24, 2021 09:22

Copy link

carsonbot commentedSep 25, 2021

Hey!

I think@okwinza has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

Copy link

Contributor

okwinza commentedSep 25, 2021

Hi@jorissteyn and thanks for your PR!

One question: is there a reason you can't just throwRecoverableExceptionInterface to achieve the desired behavior?

okwinza suggested changes

Sep 25, 2021

View reviewed changes

src/Symfony/Component/Messenger/CHANGELOG.md OutdatedShow resolvedHide resolved

src/Symfony/Component/Messenger/Exception/LogRetryAsWarningInterface.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Messenger/Exception/RecoverableExceptionInterface.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Messenger/Tests/EventListener/SendFailedMessageForRetryListenerTest.phpShow resolvedHide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Sep 25, 2021

[Messenger] Allow retries to be logged as warnings

8f757a6

This commit introduces a marker interface for exceptions to allow retries to be logged as warnings instead of the default error severity.This behaviour is already implemented for recoverable exceptions, but those are retried ad infinitum regardless of the retry strategy. With the new marker interface, it's possible to log exceptions as warnings while still using the retry strategy.The RecoverableExceptionInterface now extends this interface to achieve the existing behaviour using the new more flexible approach.

jorissteyn force-pushed thefeature/messenger-retry-log-severity branch fromb8eec9d to8f757a6Compare

September 27, 2021 06:28

Copy link

Author

jorissteyn commentedSep 27, 2021

Hi@jorissteyn and thanks for your PR!
One question: is there a reason you can't just throwRecoverableExceptionInterface to achieve the desired behavior?

Yes, our goal is to log warnings for messages which are eventually successfully handled. Currently messenger allows for three use-cases:

unrecoverable, logged as critical
recoverable, always logged as warning, but might never be successfully handled
other exceptions: logged as error, but might succeed on retry

The new interface allows creating "retryable" exceptions, logged as warnings (like recoverable) until they fail on their last retry.

I've considered simply proposing "other exceptions" to be logged as warnings until the last retry, but that would be a BC break, and for "unexpected" exceptions, it makes sense to log them as errors even if they are retried. So expclicitly marking an exception as "log as warning" fills the gap. Messenger could also provide a "RetryableException" to be more in line with how recoverable works.

Does this make sense?

Copy link

Author

jorissteyn commentedSep 27, 2021

After rebase on latest 5.4, the PHPUnit / Tests (7.2) task is failing. I can't correlate the error with a change in this PR, also, runs fine on my machine :)

Copy link

Member

nicolas-grekas commentedSep 27, 2021

Can you please share an example where this would be useful?
I'm not convinced by the naming, which looks too coupled to a specific use case.
What about adding a newgetLogLevel(\Exception $e) method toRetryStrategyInterface instead of a marker interface?

Copy link

Member

fabpot commentedOct 29, 2021

@jorissteyn Any comments on Nicolas's feedback?

fabpot modified the milestones:5.4,6.1

Nov 3, 2021

jorissteyn pushed a commit to jorissteyn/symfony that referenced this pull request

Nov 17, 2021

Allow fine-grained control in log severity of failures

0e287ec

This commit adds a new interface method on the `RetryStrategyInterface`allowing custom retry strategies to control the log severity of failedand retried messages.This is a spin-off fromsymfony#43160| Q             | A| ------------- | ---| Branch?       | 6.0| Bug fix?      | no| BC break? | yes - custom retry strategies require including the new trait or implementing the new method| New feature?  | yes - changelog not yet updated| Deprecations? | no| Tickets       | -| License       | MIT| Doc PR        | not yet, collecting feedback firstThe default logic for determining the log severity for failed messageshas been (sincesymfony#43492): - recoverable exceptions are logged as warning - unrecoverable exceptions are logged as critical - neutral exceptions are logged as warning after a non-final attempt - neutral exceptions are logged as critical after a final attemptThis PR implements the exact same behaviour but in a way that allowsusers to augment or override the default behaviour with their ownspecific needs. For example, having specific exceptions logged as`EMERGENCY` or to log them as `NOTICE` until or including the lastattempt.The refactoring done to make this possible is: - move the logic determining if an exception is   recoverable/unrecoverable/neutral to the `HandlerFailedException`   so it can be used by retry strategy objects - add a new interface method on the `RetryStrategyInterface` (BC   break!) - implement the existing behaviour using the new mechanism in a   trait, this way, custom retry strategies can easily re-use the   default behaviour

jorissteyn mentioned this pull request

Nov 17, 2021

[Messenger] Allow fine-grained control in log severity of failures#44120

Closed

jorissteyn pushed a commit to jorissteyn/symfony that referenced this pull request

Nov 17, 2021

Allow fine-grained control in log severity of failures

2704917

This commit adds a new interface method on the `RetryStrategyInterface`allowing custom retry strategies to control the log severity of failedand retried messages.This is a spin-off fromsymfony#43160| Q             | A| ------------- | ---| Branch?       | 6.0| Bug fix?      | no| BC break? | yes - custom retry strategies require including the new trait or implementing the new method| New feature?  | yes - changelog not yet updated| Deprecations? | no| Tickets       | -| License       | MIT| Doc PR        | not yet, collecting feedback firstThe default logic for determining the log severity for failed messageshas been (sincesymfony#43492): - recoverable exceptions are logged as warning - unrecoverable exceptions are logged as critical - neutral exceptions are logged as warning after a non-final attempt - neutral exceptions are logged as critical after a final attemptThis PR implements the exact same behaviour but in a way that allowsusers to augment or override the default behaviour with their ownspecific needs. For example, having specific exceptions logged as`EMERGENCY` or to log them as `NOTICE` until or including the lastattempt.The refactoring done to make this possible is: - move the logic determining if an exception is   recoverable/unrecoverable/neutral to the `HandlerFailedException`   so it can be used by retry strategy objects - add a new interface method on the `RetryStrategyInterface` (BC   break!) - implement the existing behaviour using the new mechanism in a   trait, this way, custom retry strategies can easily re-use the   default behaviour

Copy link

Author

jorissteyn commentedNov 17, 2021

@fabpot I was working under the assumption the default behaviour (logging as error by default) was desirable to leave unchanged. The patch merged in#43492 (simply changing the default) makes a lot of sense and obsoletes the changes proposed in this thread.

I have however encountered projects that require more control in how different domain-specific exceptions are logged and@nicolas-grekas suggestion would accomplish that. I've taken a stab at it in#44120 and believe it's actually a nice and flexible solution while at the same time making the default behaviour much clearer code-wise.

Can you please share an example where this would be useful?

Two abstract examples I listed in the new PR are:

Having specific exceptions logged as EMERGENCY
Log exceptions as NOTICE until or including the last attempt

Please don't hesitate to discuss this further if you're not fully convinced, I could elaborate on concrete use-cases I believe would be possible if we go forward with#44120.

jorissteyn closed this

Nov 17, 2021

jorissteyn pushed a commit to jorissteyn/symfony that referenced this pull request

Nov 18, 2021

Allow fine-grained control in log severity of failures

143756b

This commit adds a new interface method on the `RetryStrategyInterface`allowing custom retry strategies to control the log severity of failedand retried messages.This is a spin-off fromsymfony#43160| Q             | A| ------------- | ---| Branch?       | 6.0| Bug fix?      | no| BC break? | yes - custom retry strategies require including the new trait or implementing the new method| New feature?  | yes - changelog not yet updated| Deprecations? | no| Tickets       | -| License       | MIT| Doc PR        | not yet, collecting feedback firstThe default logic for determining the log severity for failed messageshas been (sincesymfony#43492): - recoverable exceptions are logged as warning - unrecoverable exceptions are logged as critical - neutral exceptions are logged as warning after a non-final attempt - neutral exceptions are logged as critical after a final attemptThis PR implements the exact same behaviour but in a way that allowsusers to augment or override the default behaviour with their ownspecific needs. For example, having specific exceptions logged as`EMERGENCY` or to log them as `NOTICE` until or including the lastattempt.The refactoring done to make this possible is: - move the logic determining if an exception is   recoverable/unrecoverable/neutral to the `HandlerFailedException`   so it can be used by retry strategy objects - add a new interface method on the `RetryStrategyInterface` (BC   break!) - implement the existing behaviour using the new mechanism in a   trait, this way, custom retry strategies can easily re-use the   default behaviour

jorissteyn pushed a commit to jorissteyn/symfony that referenced this pull request

Nov 18, 2021

Allow fine-grained control in log severity of failures

573b8b9

This commit adds a new interface method on the `RetryStrategyInterface`allowing custom retry strategies to control the log severity of failedand retried messages.This is a spin-off fromsymfony#43160| Q             | A| ------------- | ---| Branch?       | 6.0| Bug fix?      | no| BC break? | yes - custom retry strategies require including the new trait or implementing the new method| New feature?  | yes - changelog not yet updated| Deprecations? | no| Tickets       | -| License       | MIT| Doc PR        | not yet, collecting feedback firstThe default logic for determining the log severity for failed messageshas been (sincesymfony#43492): - recoverable exceptions are logged as warning - unrecoverable exceptions are logged as critical - neutral exceptions are logged as warning after a non-final attempt - neutral exceptions are logged as critical after a final attemptThis PR implements the exact same behaviour but in a way that allowsusers to augment or override the default behaviour with their ownspecific needs. For example, having specific exceptions logged as`EMERGENCY` or to log them as `NOTICE` until or including the lastattempt.The refactoring done to make this possible is: - move the logic determining if an exception is   recoverable/unrecoverable/neutral to the `HandlerFailedException`   so it can be used by retry strategy objects - add a new interface method on the `RetryStrategyInterface` (BC   break!) - implement the existing behaviour using the new mechanism in a   trait, this way, custom retry strategies can easily re-use the   default behaviour

jorissteyn pushed a commit to jorissteyn/symfony that referenced this pull request

Nov 18, 2021

[Messenger] Allow fine-grained control in log severity of failures

afb90f7

This commit adds a new interface method on the `RetryStrategyInterface`allowing custom retry strategies to control the log severity of failedand retried messages.This is a spin-off fromsymfony#43160| Q             | A| ------------- | ---| Branch?       | 6.0| Bug fix?      | no| BC break? | yes - custom retry strategies require including the new trait or implementing the new method| New feature?  | yes - changelog not yet updated| Deprecations? | no| Tickets       | -| License       | MIT| Doc PR        | not yet, collecting feedback firstThe default logic for determining the log severity for failed messageshas been (sincesymfony#43492): - recoverable exceptions are logged as warning - unrecoverable exceptions are logged as critical - neutral exceptions are logged as warning after a non-final attempt - neutral exceptions are logged as critical after a final attemptThis PR implements the exact same behaviour but in a way that allowsusers to augment or override the default behaviour with their ownspecific needs. For example, having specific exceptions logged as`EMERGENCY` or to log them as `NOTICE` until or including the lastattempt.The refactoring done to make this possible is: - move the logic determining if an exception is   recoverable/unrecoverable/neutral to the `HandlerFailedException`   so it can be used by retry strategy objects - add a new interface method on the `RetryStrategyInterface` (BC   break!) - implement the existing behaviour using the new mechanism in a   trait, this way, custom retry strategies can easily re-use the   default behaviour