Skip to content
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

[Messenger] Custom method argument $onHandledCallback for worker with command messenger:consume #32560

Closed
B-Galati opened this issue Jul 16, 2019 · 7 comments · Fixed by #34217

Comments

@B-Galati
Copy link
Contributor

B-Galati commented Jul 16, 2019

Description
At the moment it does not look possible to have a custom $onHandledCallback on the Worker used by messenger:consume command.

See ->

My use case is that I would like to reset monolog buffered handler at this very moment to send my error logs in batch to Sentry.

I thought about using WorkerMessageFailedEvent but then I would miss important logging messages such as :

  • $this->logger->error('Error thrown while handling message {class}. Dispatching for retry #{retryCount} using {delay} ms delay. Error: "{error}"', $context + ['retryCount' => $retryCount, 'delay' => $delay, 'error' => $throwable->getMessage(), 'exception' => $throwable]);
  • $this->logger->critical('Error thrown while handling message {class}. Removing from transport after {retryCount} retries. Error: "{error}"', $context + ['retryCount' => $retryCount, 'error' => $throwable->getMessage(), 'exception' => $throwable]);

Possible solution
Perhaps by replacing the callable with an interface that could be autowired in the command, not sure.

@lyrixx
Copy link
Member

lyrixx commented Jul 16, 2019

IMHO, the Symfony should be reset after each message. I do that in my custom worker and I Opened an issue in swarrot to do the same.

The status of the message should not be taken in consideration.

@B-Galati
Copy link
Contributor Author

@weaverryan one idea would be to dispatch an event to replace the callback, WDYT?

@Tobion
Copy link
Member

Tobion commented Jul 18, 2019

I thought about using WorkerMessageFailedEvent but then I would miss important logging messages

Why would you miss them? They would get sent with the next batch of messages, wouldn't they?

@B-Galati
Copy link
Contributor Author

@Tobion You're right, but they would be sent with unrelated logs records like:

---- Nothing before because it's been flushed ----
ERROR related to my message
Handling a new message unrelated to my error

@B-Galati
Copy link
Contributor Author

trying to address that issue #32614

@Tobion
Copy link
Member

Tobion commented Jul 19, 2019

they would be sent with unrelated logs records

If you sent messages in batches there are always messages in one batch that might not belong together. But what is the problem you have? Logs are time based not group based.

@B-Galati
Copy link
Contributor Author

they would be sent with unrelated logs records

If you sent messages in batches there are always messages in one batch that might not belong together. But what is the problem you have? Logs are time based not group based.

I would like to send all logs that are related to the same HTTP request or handled message in one batch. For instance I am using this monolog config. The problem I have is that I cannot achieve this behavior right now with messenger worker while it's perfectly fine with HTTP request.

Another solution that works would be to reset the kernel after each message the worker handled.

monolog:
  handlers:
    sentry_buffer:
      type: fingers_crossed
      action_level: warning
      handler: buffer
      channels: ["!event", "!security"]
    buffer:
      type: buffer
      handler: sentry
    sentry:
      type: service
      id: 'sentry_handler'

@fabpot fabpot closed this as completed Nov 4, 2019
nicolas-grekas added a commit that referenced this issue Nov 5, 2019
This PR was merged into the 4.4 branch.

Discussion
----------

[Messenger] use events consistently in worker

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Fix #32560, #32614, #33843
| License       | MIT
| Doc PR        |

The worker had the three ways to handle events
1. $onHandledCallback in `run(array $options = [], callable $onHandledCallback = null)`
2. events dispatched using the event dispatcher
3. hardcoded things inside the worker

This PR refactores the messenger worker to only use event dispatching. So instead of a hardcoded `$onHandledCallback` and worker decorators, we use event listeners and we don't need a `WorkerInterface` at all. The behavior of all the options like `--memory-limit` etc remains the same.

I introduced two new events
- `WorkerStartedEvent`
- `WorkerRunningEvent`

Together with the existing `WorkerStoppedEvent` it's very symmetrical and solves the referenced issues.

Commits
-------

201f159 [Messenger] use events consistently in worker
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants