Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

[hotfix] Logger Development config default #1019

Merged
merged 1 commit into from
Oct 27, 2015

Conversation

mleanos
Copy link
Member

@mleanos mleanos commented Oct 27, 2015

Reverts the default Logger setting to use the stdout by default, rather than the stream option for the Development env config.

I realized this might trip some users up, since when the Logger config was implemented (8cd2291) I had overlooked the fact that users probably wouldn't want the Stream option turned on by default.

Reverts the default Logger setting to use the stdout by default, rather
than the stream option.
@codydaig
Copy link
Member

LGTM

@ilanbiala
Copy link
Member

@mleanos why would users not want to stream into a file?

@codydaig
Copy link
Member

@ilanbiala I use PM2 across my servers for node applications, and PM2 automatically takes the stdout and saves it to a rotating file that I can easily tail multiple processes at the same time with, so I personally like stdout being the primary.

@ilanbiala
Copy link
Member

@mleanos @codydaig so PM2 has support for it, but then we should we include it at all? Why not remove it and just use PM2 instead of Forever in a new PR?

@mleanos
Copy link
Member Author

mleanos commented Oct 27, 2015

@ilanbiala I think it makes more sense to use stdout for Development, since it was the default being used before the Logger was implemented. I was afraid it would throw some users off. For Production, it makes sense to use the Stream option by default.

@ilanbiala
Copy link
Member

@mleanos who wouldn't use PM2 or forever in production though? My point is why not just let them handle it (rotating file logging) completely?

@mleanos
Copy link
Member Author

mleanos commented Oct 27, 2015

@ilanbiala This doesn't really affect the use of PM2 or forever. Although, PM2 has a way to rotate logs, our current implementation is using Morgan for logging. This change only affects the Morgan express middleware that we're using to log.

The Development env config will use stdout by default, but the setting can be changed to use the Stream option. As for Production, it seemed appropriate to set Morgan to save the logs to a file by default. Is that what you're questioning?

I don't see how PM2, or forever come into play here. I can see a loose relation to the former, but for our current implementation it's not really relevant.

@ilanbiala
Copy link
Member

@codydaig said that PM2 can use output from stdout and rotate it through logs, so we can just always have Morgan log through stdout, no?

@mleanos
Copy link
Member Author

mleanos commented Oct 27, 2015

@ilanbiala We're not implementing PM2 in this project. So the Morgan Stream option provides a way to log to a file. This allows out-of-the-box filesystem logging capabilities for our users.

@ilanbiala
Copy link
Member

@mleanos Forever seems to have lost much of its support, and @codydaig and @lirantal have advocated for PM2, so I'm suggesting we switch it and avoid us doing more work than we need to by supporting an extra feature when we can use a library that does that and still get better production tools.

@mleanos
Copy link
Member Author

mleanos commented Oct 27, 2015

@ilanbiala That seems like a great suggestion. Let's get something into the 0.5.0 pipeline for PM2 integration.

However, I think we can merge this simple change in, to support our current implementation of Morgan.

@ilanbiala ilanbiala added this to the 0.4.2 milestone Oct 27, 2015
@ilanbiala ilanbiala self-assigned this Oct 27, 2015
@ilanbiala
Copy link
Member

@mleanos have you used PM2 before? @codydaig maybe you could open up a PR just so we have it on record for 0.5.0, even if it's just adding PM2 to the package.json?

ilanbiala added a commit that referenced this pull request Oct 27, 2015
Change logger development config to use stdout rather than stream
@ilanbiala ilanbiala merged commit 1ddd364 into meanjs:master Oct 27, 2015
@mleanos
Copy link
Member Author

mleanos commented Oct 27, 2015

@ilanbiala No I haven't used it. But I've been looking at it, and like what I see.

Thanks for merging!

@mleanos mleanos deleted the logger-default-stdout branch October 27, 2015 21:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants