-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Messenger] Log when worker should stop and when SIGTERM
is received
#42723
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
7697d63
to
7dfa027
Compare
src/Symfony/Component/Messenger/EventListener/StopWorkerOnSigtermSignalListener.php
Show resolved
Hide resolved
@OskarStark Applied your feedback. |
src/Symfony/Component/Messenger/EventListener/StopWorkerOnSigtermSignalListener.php
Show resolved
Hide resolved
I think this PR could really benefit from #42335 once that is merged to provide additional context about the worker in the logs. |
@okwinza That's a nice improvement for a future PR indeed :) Can we get this merged? |
Could you add a testcase for this new feature? Thank you |
I would suggest merging #42335 first and then rebasing and refactoring this PR on top of it since this PR loses a significant part of it's value without the worker context data IMO. |
@okwinza I'm sorry but why would it loose value? This adds value so that it's known that the signal is received and that your signaling works. If it's so important that you know which worker was signaled, then you can already use separate logs. In my case, we run in Kubernetes and all workers are separated and identifiable. If your PR is merged, please improve this further. Let's not creating blocks that are not needed. @OskarStark I added a test case for stopping the worker. I couldn't add a test for the |
…kwinza) This PR was merged into the 5.4 branch. Discussion ---------- [Messenger] Add `WorkerMetadata` to `Worker` class. | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | Fixes #37736 | License | MIT | Doc PR | - At the moment, there is no clean way to access the values of `transportNames` or recently introduced `queueNames` that the worker was configured with, although such data might be quite useful for logging/monitoring or other tasks. This PR attempts to fix that by adding a new and extensible way to provide additional information about a particular `Worker` object. So far, the following PRs could benefit from this change: - #43133 - #42723 **Use case example:** ---- - As I developer - When a message was consumed from transport with name `async`. - And the worker state is `idle`. - Then I want to reset services. **Before this PR**, the only solution not relying on using Reflection API would look like this: ```php private $servicesResetter; private $receiversName; private $actualReceiverName = null; public function __construct(ServicesResetter $servicesResetter, array $receiversName) { $this->servicesResetter = $servicesResetter; $this->receiversName = $receiversName; } public function saveReceiverName(AbstractWorkerMessageEvent $event): void { $this->actualReceiverName = $event->getReceiverName(); } public function resetServices(WorkerRunningEvent $event): void { if (!$event->isWorkerIdle() && \in_array($this->actualReceiverName, $this->receiversName, true)) { $this->servicesResetter->reset(); } $this->actualReceiverName = null; } public static function getSubscribedEvents(): array { return [ WorkerMessageHandledEvent::class => ['saveReceiverName'], WorkerMessageFailedEvent::class => ['saveReceiverName'], WorkerRunningEvent::class => ['resetServices'], ]; } ``` **With this PR**, one could simply use this to retrieve the transport name. ```php $event->getWorker()->getWorkerMetadata()->getTransportName() === $this->transportName; ``` So the whole solution would look like this: ```php private $servicesResetter; private $receiversName; public function __construct(ServicesResetter $servicesResetter, array $receiversName) { $this->servicesResetter = $servicesResetter; $this->receiversName = $receiversName; } public function resetServices(WorkerRunningEvent $event): void { $actualTransportName = $event->getWorker()->getWorkerMetadata()->getTransportName(); if (!$event->isWorkerIdle() || !in_array($actualTransportName, $this->receiversName, true)) { return; } $this->servicesResetter->reset(); } public static function getSubscribedEvents(): array { return [ WorkerRunningEvent::class => ['resetServices'], ]; } ``` Commits ------- 583f85b [Messenger] Add WorkerMetadata to Worker class
Could you rebase? Thanks |
@lyrixx Done :) |
@lyrixx The PR keeps getting out of date when the changelog changes. Updated the PR again to make it mergeable. Is there anything left that prevents a merge? |
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.
👍🏼 Thanks
Thank you @ruudk. |
…omie) This PR was merged into the 5.4 branch. Discussion ---------- [Messenger] Add worker metadata inside logs | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | no | New feature? | . | Deprecations? | no | Tickets | Improve #42723 | License | MIT | Doc PR | . Small PR related to one of my comment #42723 (comment) on the ticket PR: having this contextual info is valuable imho cc `@ruudk` `@okwinza` `@fabpot` Commits ------- 649cb10 [Messenger] Add worker metadata inside logs
These two small log lines can help debugging crashing workers. Sometimes it's hard to tell if the worker crashed or is shutting down. Also, when working on gracefully shutdown problems, it's helpful to know when a
SIGTERM
was received and the worker is about to stop.