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] Log when worker should stop and whenSIGTERM 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 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
7697d63 to7dfa027CompareUh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Messenger/EventListener/StopWorkerOnSigtermSignalListener.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
ruudk commentedAug 26, 2021
@OskarStark Applied your feedback. |
src/Symfony/Component/Messenger/EventListener/StopWorkerOnSigtermSignalListener.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
ruudk commentedAug 27, 2021
okwinza commentedAug 31, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I think this PR could really benefit from#42335 once that is merged to provide additional context about the worker in the logs. |
ruudk commentedAug 31, 2021
@okwinza That's a nice improvement for a future PR indeed :) Can we get this merged? |
OskarStark commentedAug 31, 2021
Could you add a testcase for this new feature? Thank you |
okwinza commentedAug 31, 2021
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. |
ruudk commentedAug 31, 2021
@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
lyrixx commentedSep 23, 2021
Could you rebase? Thanks |
ruudk commentedSep 27, 2021
@lyrixx Done :) |
ruudk commentedOct 7, 2021
@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? |
lyrixx left a comment
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
fabpot commentedOct 7, 2021
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 imhocc `@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
SIGTERMwas received and the worker is about to stop.