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] Pass sender details to SendMessageToTransportsEvent#40152
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
carsonbot commentedFeb 12, 2021
Hey! I had a quick look at this PR, I think it is alright. I think@ruudk has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
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.
TheSendMessageToTransportsEvent is about sending to one or multiple transports (hence plural transports).
TheSentStamp is past tense, meaning it's already sent. When you would pass that to the subscribers listening onSendMessageToTransportsEvent that would be untrue, as the message is not sent yet.
It's different to theReceivedStamp passed to the worker subscribers, in those cases, the message is actually received.
Therefore I think it would be better to do the following:
$senders =$this->sendersLocator->getSenders($envelope);if (null !==$this->eventDispatcher) {$event =newSendMessageToTransportsEvent($envelope,$senders);$this->eventDispatcher->dispatch($event);$envelope =$event->getEnvelope();}foreach ($sendersas$alias =>$sender) {$this->logger->info('Sending message {class} with {alias} sender using {sender}',$context + ['alias' =>$alias,'sender' =>\get_class($sender)]);$envelope =$sender->send($envelope->with(newSentStamp(\get_class($sender),\is_string($alias) ?$alias :null)));}
This way:
- you still only emit 1 event
- the event now contains all the senders
- the envelope passed to the subscribers is still the same
@ruudk |
What's the status of this PR?@Jeroeny Are you still interested in moving it forward? |
Yes I'd be willing to move this forward. The last time I hit kind of a dead-end in which approach to take. Would ruudk's suggestion be desired for example? |
I think@ruudk's suggestion makes a lot of sense. |
In their snippet, |
What would be the use case for having two entries with the same alias? |
I don't think there is, but I thought it was technically supported and could cause issues. Anyway, I've implemented the suggested approach 👍 . |
Thank you@Jeroeny. |
Uh oh!
There was an error while loading.Please reload this page.
This is a feature suggestion that will allow listeners for
SendMessageToTransportsEventto be able to read the sender and sender alias from a sent message.Use case: To create metrics for messages that go through the system (and especially where to and from). Listener(s) can be created to measure this. They would listen to
WorkerMessageHandledEvent,WorkerMessageFailedEventandSendMessageToTransportsEvent. The first two receive envelopes that contain aReceivedStamp, to determine where the message came from. However, the received envelopes for sent messages do not contain a stamp to read the sender. It would be great if this was possible to do with listener (instead of having to add middleware to each bus).Approach in this PR: stamp the envelope with
SentStampbefore dispatchingSendMessageToTransportsEvent.Alternative approach: pass the info to the event directly:
new SendMessageToTransportsEvent($envelope, get_class($sender), $alias). And keep the stamping afterwards.Edit: I just noticed that one event is dispatched regardless of the number of senders. In that case the event could be dispatched after all the senders have been called (and thus all the stamps set).