[Messenger] Allow fine-grained control in log severity of failures #44120
Conversation
0e287ec to
2704917
Compare
143756b to
573b8b9
Compare
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
573b8b9 to
afb90f7
Compare
|
Hey! I think @nikophil has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
| * | ||
| * If ALL nested Exceptions are an instance of UnrecoverableExceptionInterface the failure is unrecoverable | ||
| */ | ||
| public function isUnrecoverable(): bool |
There was a problem hiding this comment.
I'm not sure this business logic should belong to this HandlerFailedException class.
I suggest using the existing getNestedExceptionOfClass instead.
| return false; | ||
| } | ||
|
|
||
| if ($this->isRecoverable($throwable)) { |
There was a problem hiding this comment.
the isRecoverable should be checked first.
In previous implementation if the exception contains both recoverable and unrecoverable the method shouldRetry returned true
| $shouldRetry = $retryStrategy && $this->shouldRetry($throwable, $envelope, $retryStrategy); | ||
| $shouldRetry = $retryStrategy && $retryStrategy->isRetryable($envelope, $throwable); |
There was a problem hiding this comment.
this should be reverted.
You moved the logic about Recoverability from SendFailedMessageForRetryListener to the MultiplierRetryStrategy but here, the retry retryStrategy is an RetryStrategyInterface. If people inject another instance, the behavior will not be preserved.
| /** | ||
| * @return string The \Psr\Log\LogLevel log severity | ||
| */ | ||
| public function getLogSeverity(Envelope $message, \Throwable $throwable = null): string; |
There was a problem hiding this comment.
that's a BC break you must provide migration path in lower versions
And, I've not convinced this method should be part of this interface
|
@jorissteyn Are you still interested in moving this PR forward? |
|
Closing as stalled. |
This PR adds a new interface method on the
RetryStrategyInterfaceallowing custom retry strategies to control the log severity of failed and retried messages.This is a spin-off from #43160
The default logic for determining the log severity for failed messages has been (since #43492):
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
EMERGENCYor to log them asNOTICEuntil 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
HandlerFailedExceptionso it can be used by retry strategy objectsadd 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