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 6.2 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

Thanks for the PR. This change looks like a new feature to me, or is there something that breaks without it?

Hello and thanks for the reply! 🙂

You're right, it's notbroken. I was also hesitating between a bug and a feature...
Basically, it makes the log less useful as the relationship between the message and the event is not visible, which makes me think it's amissing element rather than anew. What do you think?

That’s a new feature 😀

I have updated the pull request accordingly. 😉

Message content can be very huge. This can also overflow some third party service.

But I like the idea. I'm my application, I usually log raw object, then I add a custom monolog processor to keep only what I need. (I gave atalk about it)

We could do the same here

1. Log with the raw envelop
2. Add a default monolog processor that
   • Remove eveything and keep only the class.
   • Or keep only the 1024 first bytes of the content (arbitrary number)

So if people want more or less information in the log, they can decorate / replace the default processor in the DIC.

WDYT?

(I actually really like the idea. I think it could be generalized to the framework, when applicable)

Your idea goes further than my initial thoughts, but I really like it! 🙂

Instead of adding an additional risk of overflow (the message is already logged in multiple places), we would fix a minor lack and improve the component monitoring. Moreover, the possibility to replace/decorate the processor would be great.

I can try to work on this approach in the coming days if nobody else is on it.
In the meantime, what do you want to do with this pull request?

I can try to work on this approach in the coming days if nobody else is on it.

Please, wait for more approval from the core team, I'm not sure everything is OK with that

ping @symfony/mergers

@Nyholm removing this key is basically closing the pull request. 😅

Joke aside, what is your opinion on the processor approach proposed by@lyrixx? We could only keep the message class by default and let users decide if they want to add further data through decorators.

Because even if we decide to not add the message withinSendFailedMessageToFailureTransportListener::onMessageFailed logs, the message is already available (at least) in:

• \Symfony\Component\Messenger\TraceableMessageBus::dispatch
• \Symfony\Component\Messenger\Worker::ack
• \Symfony\Component\Messenger\EventListener\SendFailedMessageForRetryListener::onMessageFailed
• \Symfony\Component\Messenger\Middleware\HandleMessageMiddleware::handle
• \Symfony\Component\Messenger\Middleware\SendMessageMiddleware::handle

Hello@lyrixx and@Nyholm,

Did you have the opportunity to discuss the changes mentioned above? Thanks for your time.

Adding the raw message is a no go unfortunately. IMHO the custom processor is the way to go.

Thanks for your feedback. In that case, I'm closing this pull request.

But while I understand the reason why adding the message could cause problems (e.g. leak of sensitive data or logs overflow), I don't understand why it's a problem here and not in the other parts of the component. Am I missing something?

You are correct. Maybe we did miss something in previous prs.

I would be super happy if you want to help by removing those instances to.
I definitely understand if you don’t, but please tell us so someone else can work on that.

No problem, I'll submit a PR with the removal of other instances.

But I would love to have a workaround idea for the application I'm working on since we use the message details to monitor exactly what's going on. 😉

Should we subscribe to Messenger events and add our own log entries?

Should we subscribe to Messenger events and add our own log entries?

Yes,
You can also add your custom middleware. That will give you maximum control.