Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Messenger] Allow fine-grained control in log severity of failures#44120
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
0e287ec to2704917Compare143756b to573b8b9CompareThis 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
573b8b9 toafb90f7Comparecarsonbot commentedNov 18, 2021
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 | ||
| */ | ||
| publicfunctionisUnrecoverable():bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sure this business logic should belong to thisHandlerFailedException class.
I suggest using the existinggetNestedExceptionOfClass instead.
| returnfalse; | ||
| } | ||
| if ($this->isRecoverable($throwable)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
theisRecoverable should be checked first.
In previous implementation if the exception contains bothrecoverable andunrecoverable the methodshouldRetry returnedtrue
| $shouldRetry =$retryStrategy &&$this->shouldRetry($throwable,$envelope,$retryStrategy); | ||
| $shouldRetry =$retryStrategy &&$retryStrategy->isRetryable($envelope,$throwable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this should be reverted.
You moved the logic about Recoverability fromSendFailedMessageForRetryListener to theMultiplierRetryStrategy but here, the retryretryStrategy is anRetryStrategyInterface. If people inject another instance, the behavior will not be preserved.
| /** | ||
| * @return string The \Psr\Log\LogLevel log severity | ||
| */ | ||
| publicfunctiongetLogSeverity(Envelope$message,\Throwable$throwable =null):string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
fabpot commentedMar 26, 2022
@jorissteyn Are you still interested in moving this PR forward? |
fabpot commentedJul 29, 2022
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