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

Logs should be flushed on process exit #19

Closed
adisbladis opened this issue Jun 7, 2017 · 9 comments
Closed

Logs should be flushed on process exit #19

adisbladis opened this issue Jun 7, 2017 · 9 comments

Comments

@adisbladis
Copy link

@adisbladis adisbladis commented Jun 7, 2017

Currently logs are emitted every 2000ms but not on process exit.

This causes processes that are very short lived (say a bug that crashes the server on startup) to not show anything.

@mostlyjason
Copy link

@mostlyjason mostlyjason commented Jun 8, 2017

Thanks for reporting! Hopefully @Shwetajain148 can address this one

@Shwetajain148
Copy link

@Shwetajain148 Shwetajain148 commented Jul 14, 2017

HI @adisbladis, As you said that short-lived processes that may crash the server or stop a running application (as per your case) doesn't show anything on process exit. My assumption is that if the application crashes then the logging will not be completed (it seems the desired behavior).

Could you please provide more information about your setup and environment so that I can be able to reproduce it at my end and then can work further to resolve it?

@mostlyjason
Copy link

@mostlyjason mostlyjason commented Jul 14, 2017

I think what he is suggesting is to add some kind of an event listener for process exit. When the event is triggered, we could flush the logs that are in the queue and then continue exiting.

@adisbladis
Copy link
Author

@adisbladis adisbladis commented Jul 14, 2017

Exactly what @mostlyjason said.
I don't mean that node comes crashing down

All you need to do to reproduce is this:
Logger.whatever("Hello world"); process.exit(1)

@Shwetajain148
Copy link

@Shwetajain148 Shwetajain148 commented Jul 25, 2017

Hi @adisbladis, I looked at this issue and tried to handle the exit events like process.exit(); or SIGINT i.e. pressing Ctrl+C to terminate the running process but in each case, I found that as soon as the process.exit event gets fired, no more logging takes place. As we are using ajax requests to send logs to Loggly which is an asynchronous operation, exit events will never wait for ajax requests to complete first. Please refer the below link that states
Listener functions must only perform synchronous operations. The Node.js process will exit immediately after calling the 'exit' event listeners causing any additional work still queued in the event loop to be abandoned.

Ref: https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_event_exit

I dug around and came up with two ideas to implement your feature requests.

  1. The first approach that I can follow is to check the library status i.e. when the library is done sending all logs and then I can return a boolean flag true and based on this result anyone can call process.exit() explicitly. This will ensure the complete logging from the user end. Below is the sample code snippet:
var logglyLibStatus = checkLibrarySatus();
if (logglyLibStatus) {
  process.exit(1);
}
  1. The second approach that I can think of is to create a variable like shouldProcessExit which will accept the user input either in true or false at run time. If the user sets the value to true it means he wants to exit the application after successful logging. So I will keep on checking the value of shouldProcessExit at a specific interval and when the library will find its value to true, the library will exit the application otherwise it will keep on running.

I am thinking of to implement one of above approaches.

cc: @mostlyjason

@mostlyjason
Copy link

@mostlyjason mostlyjason commented Jul 25, 2017

I don't think we should ask for user input since many scripts run headless. I like option 1. However, don't stall the exit based on a boolean. Make the best effort to send logs and then exit.

@FDIM
Copy link

@FDIM FDIM commented Aug 15, 2017

Why not dump remaining entries to a file (possibly user specified) and provide a helper method to flush it upon next startup?

@mostlyjason
Copy link

@mostlyjason mostlyjason commented Sep 22, 2017

@FDIM sorry I just saw this now. We implemented it this way because that was the proposed solution at the time on winstonjs/winston#228. It seems like this problem is bigger than just the Loggly plugin, it seems to affect winston as a whole and file logging. I hope we can make use of the solution the winston team decides on once it finds a better solution. In the meantime calling a helper function could be a convenient workaround.

@mostlyjason
Copy link

@mostlyjason mostlyjason commented Sep 22, 2017

Closing this for now feel free to reopen if you have additional discussion

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

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.