[Messenger] Allow retries to be logged as warnings #43160
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
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! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
4afd267 to
b8eec9d
Compare
|
Hey! I think @okwinza has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
|
Hi @jorissteyn and thanks for your PR! One question: is there a reason you can't just throw |
src/Symfony/Component/Messenger/Exception/LogRetryAsWarningInterface.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Messenger/Exception/RecoverableExceptionInterface.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Messenger/Tests/EventListener/SendFailedMessageForRetryListenerTest.php
Show resolved
Hide resolved
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.
b8eec9d to
8f757a6
Compare
Yes, our goal is to log warnings for messages which are eventually successfully handled. Currently messenger allows for three use-cases:
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? |
|
After rebase on latest 5.4, the |
|
Can you please share an example where this would be useful? |
|
@jorissteyn Any comments on Nicolas's feedback? |
This commit adds a new interface method on the `RetryStrategyInterface` allowing custom retry strategies to control the log severity of failed and retried messages. This is a spin-off from symfony#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 first The default logic for determining the log severity for failed messages has been (since symfony#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 attempt This PR implements the exact same behaviour but in a way that allows users to augment or override the default behaviour with their own specific needs. For example, having specific exceptions logged as `EMERGENCY` or to log them as `NOTICE` until or including the last attempt. 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
This commit adds a new interface method on the `RetryStrategyInterface` allowing custom retry strategies to control the log severity of failed and retried messages. This is a spin-off from symfony#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 first The default logic for determining the log severity for failed messages has been (since symfony#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 attempt This PR implements the exact same behaviour but in a way that allows users to augment or override the default behaviour with their own specific needs. For example, having specific exceptions logged as `EMERGENCY` or to log them as `NOTICE` until or including the last attempt. 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
|
@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.
Two abstract examples I listed in the new PR are:
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. |
This commit adds a new interface method on the `RetryStrategyInterface` allowing custom retry strategies to control the log severity of failed and retried messages. This is a spin-off from symfony#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 first The default logic for determining the log severity for failed messages has been (since symfony#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 attempt This PR implements the exact same behaviour but in a way that allows users to augment or override the default behaviour with their own specific needs. For example, having specific exceptions logged as `EMERGENCY` or to log them as `NOTICE` until or including the last attempt. 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
This commit adds a new interface method on the `RetryStrategyInterface` allowing custom retry strategies to control the log severity of failed and retried messages. This is a spin-off from symfony#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 first The default logic for determining the log severity for failed messages has been (since symfony#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 attempt This PR implements the exact same behaviour but in a way that allows users to augment or override the default behaviour with their own specific needs. For example, having specific exceptions logged as `EMERGENCY` or to log them as `NOTICE` until or including the last attempt. 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
This commit adds a new interface method on the `RetryStrategyInterface` allowing custom retry strategies to control the log severity of failed and retried messages. This is a spin-off from symfony#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 first The default logic for determining the log severity for failed messages has been (since symfony#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 attempt This PR implements the exact same behaviour but in a way that allows users to augment or override the default behaviour with their own specific needs. For example, having specific exceptions logged as `EMERGENCY` or to log them as `NOTICE` until or including the last attempt. 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
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.