[Messenger][FrameworkBundle] Add option to exclude stack trace from ErrorDetailsStamp#53454
[Messenger][FrameworkBundle] Add option to exclude stack trace from ErrorDetailsStamp#53454HypeMC wants to merge 1 commit intosymfony:8.1from
ErrorDetailsStamp#53454Conversation
7c5a9e8 to
d2851fd
Compare
ErrorDetailsStampErrorDetailsStamp
| ->defaultNull() | ||
| ->info('Transport name to send failed messages to (after all retries have failed).') | ||
| ->end() | ||
| ->booleanNode('include_stack_trace_in_error') |
There was a problem hiding this comment.
I'm not too crazy about the name of the option, maybe someone has a better suggestion.
src/Symfony/Component/Messenger/EventListener/AddErrorDetailsStampListener.php
Outdated
Show resolved
Hide resolved
d2851fd to
c61849d
Compare
|
Hi, thanks for preparing the fix for this issue. I have a few concerns regarding the solution though. If I get it right, with this option the message will lose pretty important info about an error. From my understanding, the issue is not only in the big stack trace but also in the fact that the exception contains a previous exception and this previous exception has another previous exception. Eventually, after flattening the exception we could end up in having the stack trace processed from multiple exceptions, and for me this is what we should try to fix. Instead of flattening all previous exceptions, we should flatten only a current exception. This will allow us to keep the stack trace of the "latest" exception that will be useful to identify the error. |
|
@odombrovskyi-dev Hi, as mentioned in the description, the idea here is to allow the user to implement their own listener and log the exception however they see fit. For example, they can log the entire error into Logstash and add a custom stamp that serves as an identifier connecting the message to the log. I guess I could remove the stack trace and just leave the exception, but that might prove useless in some cases, as a certain exception might get thrown from different places. |
c61849d to
c08c143
Compare
c08c143 to
be566c4
Compare
be566c4 to
8023a58
Compare
…ErrorDetailsStamp`
8023a58 to
672d9be
Compare
|
We have introduced a decorated transport for amqp to prevent "Invalid AMQP data" |
|
I would be cleaner to just omit the entire ErrorDetailsStamp, not just the stack trace. When GuzzleException is added to ErrorDetails with large body/content it should also be omitted when size is gt 1024 bytes. |
|
Please have a look at #63288 instead (I don't like adding options :) ) |
…(nicolas-grekas) This PR was merged into the 6.4 branch. Discussion ---------- [Messenger] Optimize serialized size of ErrorDetailsStamp | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix #45944, fix #44943 | License | MIT Instead of #53454 As described in the linked issues, retrying errors can lead to huge message payloads. This PR implements 3 fixes for this concern: - As proposed in #45944, we just don't create a `FlattenException` when a `RecoverableExceptionInterface` is found: those are user initiated so that's not really an error anymore but intended behaviour. - We just remove the trace from generated `FlattenException` objects. This keeps only its "as string" version, which to me is enough to start a debugging session when needed. Ppl that need more can always use a logger to get the full detailed trace. - I updated the `ErrorDetailsStamp::equals()` method to not consider the message anymore but the file+line instead. This should provide more dedup opportunities. Commits ------- 15dd011 [Messenger] Optimize serialized size of ErrorDetailsStamp
Some transports, in my case Beanstalk, don't like the large payloads that can result from the entire stack trace being in the
ErrorDetailsStamp.The idea of this PR is to allow excluding the stack trace through an option. The user could then optionally add a custom listener to log the stack trace to alternative storage.