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

Make handler flush on reset #102

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

DrLuke
Copy link

@DrLuke DrLuke commented Feb 24, 2022

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    CloudWatch implements ResettableInterface (via AbstractHandler), but does nothing on resetting. That means that long running processes like the message consumer don't flush logs between handling messages.

  • What is the current behavior? (You can also link to an open issue here)
    Calling reset() on logger doesn't do anything

  • What is the new behavior (if this is a feature change)?
    Calling reset() on logger flushes messages

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
    No

  • Other information:

This handler implements `ResettableInterface`, but does nothing on resetting. That results in logs not getting flushed on long running processes like the message handler.
With this flushing should happen on reset.
@JerrycheeMM
Copy link

JerrycheeMM commented Apr 20, 2022

Hi DrLuke, may I know how does the reset() being triggered ? I wrote a scheduler script to reset the log every 5 minutes but it seems like its not working. I realized that it is a different set of buffer depending on where the Log::info() is being triggered.

@DrLuke
Copy link
Author

DrLuke commented Apr 25, 2022

@bradp-roweit
Copy link

This affects compatability with Laravel Octane too. Octane has a class Laravel\Octane\Listeners\FlushMonologState, which calls the reset() method.

The change in this PR is the same solution we came up with when running into the issue.

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.

3 participants