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

Register channels to ResetLoggersWorkerSubscriber #403

Closed
wants to merge 1 commit into from

Conversation

l-vo
Copy link

@l-vo l-vo commented Apr 10, 2021

Channels registration for symfony/symfony#40761

@l-vo l-vo force-pushed the reset_loggers_on_messenger_workers branch from 08189af to cc8663a Compare April 10, 2021 20:44
@l-vo l-vo force-pushed the reset_loggers_on_messenger_workers branch from cc8663a to 3c93182 Compare April 10, 2021 20:45
fabpot added a commit to symfony/symfony that referenced this pull request Apr 13, 2021
This PR was merged into the 5.3-dev branch.

Discussion
----------

[MonologBridge] Reset loggers on workers

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       |
| License       | MIT
| Doc PR        |

This PR tries to solve some problems with buffered handlers (FingerCrossed) in workers.

Let's consider the default configuration (`stop_buffering: true`):
- When the threshold is crossed, all logs are flushed. Logs for the current message but also logs of previous messages in the buffer. Although buffer is limited `buffer_size`, it's a shame to keep logs of previous messages.
- When the threshold is crossed, buffering is disabled. So finger crossed configuration is not used anymore, all the logs are flushed as soon as they are written.

Then with (`stop_buffering: false`) (why isn't this the default configuration ?)
- It's a bit better since buffering isn't disabled when the threshold is crossed
- Like with `stop_buffering: true`, logs of previous messages are kept in memory

In a similar way of `DoctrineClearEntityManagerWorkerSubscriber`, this PR adds a `ResetLoggersWorkerSubscribber` to reset resettable loggers.

Integration in Monolog bundle: symfony/monolog-bundle#403

Commits
-------

1d2f7f1 [Messenger] Reset loggers on workers
@fabpot
Copy link
Member

fabpot commented Apr 13, 2021

The related PR in Symfony has been merged for 5.3.

@l-vo
Copy link
Author

l-vo commented Jun 16, 2021

Hello, is there a next release planned with this PR please ? Resetting loggers on workers has been announced for 5.3 (https://symfony.com/blog/new-in-symfony-5-3-logging-improvements) and this PR is needed to allow it to work.

@alxndrbauer
Copy link

Is there any particular reason why this is not merged yet?

@lyrixx
Copy link
Member

lyrixx commented Jul 30, 2021

I don't think this is good actually. The listener will always be registered. That means when Symfony handle an http request, if a message is sent in a sync transport, monolog will be flushed. So if something goes wrong after that, all log about routing, security etc will be lost 😐

I created another pr to fix this issue more "globally" symfony/symfony#41163

@lyrixx
Copy link
Member

lyrixx commented Oct 5, 2021

@l-vo Thanks for working on this topic.

As I have explain it in my previous comment, the alternative implementation provided by symfony/symfony#41163 (and subsequent PR) is better.

I'm also deprecating the integration in the bridge

I hope your understand the move here.

So, I'm sorry for closing your PR, but I think it's better like this.

Again, thanks for your first PR.

@lyrixx lyrixx closed this Oct 5, 2021
@l-vo
Copy link
Author

l-vo commented Oct 9, 2021

@lyrixx No problem, the reason is legit, thank you 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants