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

feature: merge std(err && out) logs and tidy logic of stdio #822

Merged
merged 12 commits into from
Nov 17, 2014
Merged

feature: merge std(err && out) logs and tidy logic of stdio #822

merged 12 commits into from
Nov 17, 2014

Conversation

Tjatse
Copy link
Collaborator

@Tjatse Tjatse commented Nov 17, 2014

Final chapters:

  • Merge std(err && out) logs, fixes Add an option to combine stderr and stdout into one log file #807
  • Tidy logic of stdio (waterfall)
  • Disconnect child_process graceful/peaceful, with some node vers, the child_process does not disconnect immediately even if it is dumped, maybe it is the GC strategy, so currently we should close the spawned process at once since the _reloadLogs event is being triggered, it may fixes the following problems:
    • Log lag
    • Log gets lost
    • Useless EventEmitter.on
    • Too many stdios are piping stream to a specific file at once (most of them are useless)
    • ...
  • Fix some small bugs

Test cases of merge std(err && out) logs coverage

start(cluster/fork)
restart
reload
graceful(Restart/Reload...)
reloadLogs
flush
describe(desc/info)
logs
jlist(prettylist)
startJSON

@Tjatse
Copy link
Collaborator Author

Tjatse commented Nov 17, 2014

The test can not go over

  • v0.11.13
  • v0.10.32
  • v0.10.30

Error comes out from ./test/bash/gracefulReload2.sh, but on my Mac Pro, it passed.

Weird, seems the test stuff is easy broke.

@soyuka
Copy link
Collaborator

soyuka commented Nov 17, 2014

Travis sometimes has issues with pm2 test suite. Let me fork this on my workspace and let you know.

@Tjatse
Copy link
Collaborator Author

Tjatse commented Nov 17, 2014

Sweet!!! appreciate!!!

@soyuka
Copy link
Collaborator

soyuka commented Nov 17, 2014

10.31 is ok
11.14 is ok

Debian 7

@Tjatse
Copy link
Collaborator Author

Tjatse commented Nov 17, 2014

@soyuka meantime, I've found a bug of ForkMode...

@Tjatse
Copy link
Collaborator Author

Tjatse commented Nov 17, 2014

@soyuka I've reverted the previous commit and push a new one, you can read that one more thing I have done:

cspr.removeAllListeners();
cspr.disconnect();
startLogging(cb);

The problem is We should disconnect the previous CHILD_PROCESS, then spawn a new one, this actually makes a graceful reloadLogs

@soyuka
Copy link
Collaborator

soyuka commented Nov 17, 2014

Yes indeed, this is a nice improvement and would also prevent the famous "too many open files" unix error.
This is a good refactor, I just hope it doesn't break any features :).

@Unitech thoughts?

@Tjatse
Copy link
Collaborator Author

Tjatse commented Nov 17, 2014

Just dont merge this PR..bugs have been dug. :)

@soyuka
Copy link
Collaborator

soyuka commented Nov 17, 2014

No problem, let me know when you think it's good to go and I'll test it locally ;)

@Unitech
Copy link
Owner

Unitech commented Nov 17, 2014

I understand that the test suit is quite big and slow on all these Node.JS versions. I made a first POC to parallelize tests, but didn't went further (https://github.com/Unitech/PM2/blob/master/test/fast-test.sh#L48).

By the way the refactor looks like a good initiative and improvement to me, thanks for helping to make this tool better 👍

@Tjatse
Copy link
Collaborator Author

Tjatse commented Nov 17, 2014

@Unitech Thats very kind of you.

@Unitech @soyuka It's solved successfully, the test cases are passed by avoiding retries from Owner/Collaborator.

@Tjatse Tjatse changed the title feature: merge std(err && out) logs feature: merge std(err && out) logs and tidy logic of stdio Nov 17, 2014
Unitech added a commit that referenced this pull request Nov 17, 2014
feature: merge std(err && out) logs and tidy logic of stdio
@Unitech Unitech merged commit b60df25 into Unitech:master Nov 17, 2014
@Unitech
Copy link
Owner

Unitech commented Nov 17, 2014

I merged the PR into development, sometime tests are not passing, we should investigate more

@Tjatse
Copy link
Collaborator Author

Tjatse commented Nov 18, 2014

That's okay, @Unitech @soyuka @jshkurti I've deeply looked into the test bashes, and found that rm [FILE] always be called immediately after $ pm2 kill/delete, this actually have no effects, thats why you guys can find that so much logs are left behind (under fixtures && bash dir) after npm test have been done, pm2 kill doesn't peaceful stop the Stream Pipe, it will be destroyed until there have no WritableStream in Pipes.

So, when you run

$ pm2 delete all
rm *.log

rm do nothing sometimes.

How to solve these?

  • sleep [SECOND(s)] before rm, in my test cases test/bash/log-entire.sh L38-40, it's sleep 1, e.g.:

    $ pm2 delete all
    sleep 1
    rm *.log
    
  • Another way it's to randomize the log name, but not a fixed one, we current using out-rel.log or mergeout.log too much, these will effect the next test if it's not removed cleanly.

@Tjatse
Copy link
Collaborator Author

Tjatse commented Nov 18, 2014

I've restarted the failed 2 Travis Jobs, passed now. It seems have no relationship with this PR. x.x

@Unitech
Copy link
Owner

Unitech commented Nov 18, 2014

Tests pass better now! I refactored some stuff and also removed the .disconnect() in the fork mode file because processes need to be able to communicate with PM2 via IPC.

Else your PR is nice, I haven't thought about making a third log file that would merge the error and output so we can inspect logs in contexts.

I was thinking about doing something like pm2-logrotate that would be integrated into PM2, so we don't need to install and configure an external tool to get a production environment up and running.
It would avoid us to get gigantic logs file (:

@Tjatse
Copy link
Collaborator Author

Tjatse commented Nov 19, 2014

It is fine. I worked on node-forever for a while, and know exactly what him want, agreed with you, a merged log is needed sometime - to trace error context. And I've updated the Adv-ReadMe to let users known how to produce logs. Of course, if you guys have another brilliant idea to figure this out, it's glad to accept.=

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.

Add an option to combine stderr and stdout into one log file
3 participants