Skip to content

[MonologBridge] Reset loggers on workers #40761

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 13, 2021
Merged

[MonologBridge] Reset loggers on workers #40761

merged 1 commit into from
Apr 13, 2021

Conversation

l-vo
Copy link
Contributor

@l-vo l-vo commented Apr 10, 2021 •
edited
Loading

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

@l-vo l-vo force-pushed the messenger_reset_loggers branch 2 times, most recently from 34ad0d8 to 3845558 Compare April 10, 2021 14:59
@l-vo l-vo changed the title WIP: Reset loggers on workers Reset loggers on workers Apr 10, 2021
@l-vo l-vo changed the title Reset loggers on workers [Messenger] Reset loggers on workers Apr 10, 2021
@l-vo l-vo force-pushed the messenger_reset_loggers branch from 3845558 to 895e5af Compare April 10, 2021 15:26
@l-vo l-vo closed this Apr 10, 2021
@l-vo l-vo reopened this Apr 10, 2021
@l-vo l-vo mentioned this pull request Apr 10, 2021
Closed
jderusse
jderusse reviewed Apr 10, 2021
View reviewed changes
Copy link
Member

@jderusse jderusse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great to provide the "glue" in monolog-bundle

@carsonbot carsonbot changed the title [Messenger] Reset loggers on workers [MonologBridge] Reset loggers on workers Apr 10, 2021
@jderusse jderusse added this to the 5.x milestone Apr 10, 2021
@l-vo l-vo force-pushed the messenger_reset_loggers branch from 895e5af to adfa302 Compare April 10, 2021 20:57
@l-vo
Copy link
Contributor Author

l-vo commented Apr 10, 2021

Would be great to provide the "glue" in monolog-bundle

just pushed it: symfony/monolog-bundle#403

@l-vo l-vo force-pushed the messenger_reset_loggers branch from adfa302 to 005686c Compare April 11, 2021 13:43
fabpot
fabpot approved these changes Apr 13, 2021
View reviewed changes
src/Symfony/Bridge/Monolog/CHANGELOG.md Outdated
@@ -1,6 +1,10 @@
CHANGELOG
=========

5.3.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5.3 (see https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry)

src/Symfony/Bridge/Monolog/CHANGELOG.md Outdated
@@ -1,6 +1,10 @@
CHANGELOG
=========

5.3.0
-----
* Added `ResetLoggersWorkerSubscriber` to reset buffered logs in messenger workers
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing empty line above

Added -> Add

@fabpot
Copy link
Member

fabpot commented Apr 13, 2021

Thank you @l-vo.

@fabpot fabpot force-pushed the messenger_reset_loggers branch from 005686c to 1d2f7f1 Compare April 13, 2021 06:43
@fabpot fabpot mentioned this pull request Apr 13, 2021
Closed
@fabpot fabpot merged commit ff0cf61 into symfony:5.x Apr 13, 2021
@l-vo l-vo deleted the messenger_reset_loggers branch April 17, 2021 21:34
@fabpot fabpot mentioned this pull request Apr 18, 2021
Merged
This was referenced Apr 18, 2021
Closed
Closed
@gggeek
Copy link

gggeek commented May 11, 2021

Sorry for chiming in so late, just giving my 2c of advice: this is a nice feature to have, that is in fact useful for any long-running sf console script, be it a daemon or a huge batch script doing import/export of data and taking many hours to run.

As such, it could probably use a better name than 'SomethingWorker', to make it more evident to developers that it is not tied exclusively to the Messenger component.
It could also benefit of being given more visibility in the docs (if not in the code itself, at least in the symfony docs)

@l-vo
Copy link
Contributor Author

l-vo commented May 11, 2021 •
edited
Loading

@gggeek It would be useful for any long-running process that uses a loop BUT this feature relies on a worker event. It's why it is named somethingWorker 🙂

@gggeek
Copy link

gggeek commented May 11, 2021

@l-vo doh. I missed the namespace of the event, my bad. Wouldn't it be better then to have the worker emit instead a resetLogs event?

@l-vo
Copy link
Contributor Author

l-vo commented May 11, 2021

@gggeek No it wouldn't. The reason is decoupling. The emitted event should not be aware of what the listeners are going to do.

@gggeek
Copy link

gggeek commented May 14, 2021

But then the monolog bridge is forced to know about this event - as well as any other event required by daemon-like scripts. It does not seem very different...

@derrabus derrabus mentioned this pull request May 20, 2021
Merged
nicolas-grekas added a commit that referenced this pull request May 23, 2021
@nicolas-grekas
feature #41301 [MonologBridge] Remove deprecated code (derrabus)
2dc99db 
This PR was merged into the 6.0 branch.

Discussion
----------

[MonologBridge] Remove deprecated code

| Q             | A
| ------------- | ---
| Branch?       | 6.0
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | #40761
| License       | MIT
| Doc PR        | N/A

Commits
-------

53ce26e [MonologBridge] Remove deprecated code
@lyrixx lyrixx mentioned this pull request Oct 1, 2021
Closed
@Seldaek Seldaek mentioned this pull request May 3, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jderusse jderusse jderusse left review comments

@fabpot fabpot fabpot approved these changes

Assignees
No one assigned
Labels
Feature MonologBridge Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
5 participants
@l-vo @fabpot @gggeek @jderusse @carsonbot