[bug] Enable log options for Morgan that were missed in the 0.4.0 merge #999

mleanos · 2015-10-17T03:47:37Z

Adds the log options, and format to the Morgan middleware in the Express configuration.

These options are defined in the environment configurations.

The implementation derived from #254 by @lirantal, which somehow got overlooked when merging 0.4.0 into master.

Related to the discussion in #975, and taking over for @codydaig #982

mleanos · 2015-10-17T03:54:19Z

I'm aware of the coverage decrease. However, I just wanted to get this out there for review & work on writing tests in the meantime. I shall do that now :)

mleanos · 2015-10-17T05:09:40Z

I wrote two tests initially for this feature. However, I'm unsure if I went about this correctly. They just seem too simple. Although, the testability of a feature like this does seem limited.

~~It may not make sense to have the log options set in the Test env config. Does anyone have any feedback, or insight into how to enhance the test coverage here?~~ I removed the settings from the Test env config in my last commit.

This brings up an interesting idea I was pondering a while ago.. Would be worthwhile to add a feature to reinitialize the global configuration? In instances where the NODE_ENV is manually changed (most likely for test coverage only), it would be nice to test features against the updated configuration settings.

Any thought on this?

mleanos · 2015-10-17T06:53:44Z

I reworked the tests, and cleaned up the default settings for the log options & format.

I think this last commit is better, but still would love some feedback.

lirantal · 2015-10-17T17:11:41Z

@mleanos I did a similar "trick" with changing the NODE_ENV from testing to production through out the tests to make sure that the config works properly.

Otherwise LGTM.

mleanos · 2015-10-17T17:31:54Z

@lirantal I liked your strategy of changing the NODE_ENV manually. However, that won't work in this case. Once the logger is required, the config is initialized. Changing the node env will have no impact aftward.

What if I added a init method to the logger service? There I could add the require of the config. Is that a route worth going down?

lirantal · 2015-10-17T17:36:34Z

Don't think so because that's usually a bad practice to require within functions/if because node can't cache that and whatever other side effects it will have.
We can go with what we have for now until we come up with another strategy.

mleanos · 2015-10-17T17:40:39Z

SGTM. Thank you.

Any opinion on if we should add the log settings to the Test env config? This way we can test the Stream option as well.

codydaig · 2015-10-17T18:28:39Z

config/lib/logger.js

+      options.stream = fs.createWriteStream(process.cwd() + '/' + config.log.options.stream, { flags: 'a' });
+    } catch (err) {
+      // remove the stream option
+      delete options.stream;


Do we want to log an error to the console instead of just silently catching and handeling the error?

That's a good point. I think logging to the console would be appropriate here.

mleanos · 2015-10-17T21:50:39Z

I just committed quite a bit of changes. I added a bit more error handling, and modified some logic of how I was returning the log settings. For instance, before I was returning references to the env config settings. I think that was wrong. Now I'm making copies.

I also added a couple more tests, and added the log settings to the Test env config.

Further review would be greatly appreciated.

jloveland · 2015-10-18T02:40:15Z

config/env/test.js

+    // Can specify one of 'combined', 'common', 'dev', 'short', 'tiny'
+    format: process.env.LOG_FORMAT || 'combined',
+    // Stream defaults to process.stdout
+    // Uncomment to enable logging to a log on the file system


@mleanos what is this comment for? doesn't the option below log to the file system?

It was just a left over comment from when I copied the settings from the development config.

I can remove it, but it also does make sense to leave it?

It seems confusing. Let's move the comments in line 18 to https://github.com/mleanos/mean/blob/morgan-logger-config/config/env/test.js#L21 inside the options object and above the stream option.

@jloveland @lirantal I moved the comment, and re-worded it. Let me know if you'd like additional changes to it. Thanks.

mleanos · 2015-10-18T23:23:06Z

@codydaig I have rebased, and squashed. If there's no other feedback, or requested changes, then I think this is ready.

jloveland · 2015-10-19T00:38:09Z

config/lib/logger.js

+
+      // create the WriteStream to use for the logs
+      if (options.stream.length) {
+        options.stream = fs.createWriteStream(process.cwd() + '/' + config.log.options.stream, { flags: 'a' });


@mleanos, great job on this!

I think it would be more robust to provide an ability to specify the full path to the directory which we would like logs to be stored. For example, I like my logs to go to /var/log/meanjs/access.log. Your code assumes the root directory is the cwd.

@jloveland That's a good idea. I'll work on adding this.

I think moving the definition of the log location to be in the env config would be the way to go. This way the the logger config won't need to calculate the location of the logs.

jloveland · 2015-10-19T00:44:37Z

I think it would be nice add log rotation configs to the config.env.production.js and have that propagate to the config.lib.logger.js

// create a rotating write stream
var accessLogStream = FileStreamRotator.getStream({
  filename: logDirectory + '/access-%DATE%.log',
  frequency: 'daily',
  verbose: false
})

// setup the logger
app.use(morgan('combined', {stream: accessLogStream}))

lirantal · 2015-10-19T05:42:57Z

@jloveland sounds like a good diea
@mleanos can we add the logrotating functionality?

mleanos · 2015-10-19T06:49:11Z

@jloveland @lirantal The log rotating is a good idea.

I'll have to reconfigure the stream options, but I think it can be done to easily allow switching between both log rotating, and single file log (or even possibly allow for both options at the same time).

mleanos · 2015-10-19T07:03:18Z

@lirantal @jloveland I started looking at how I could change the Stream options in the env configs. Here's my initial idea... Any thoughts on this approach?

stream: {
  single: {
    active: true,
    directoryPath: process.cwd(),
    fileName: 'access.log'
  },
  rotating: {
    active: true,
    directoryPath: 'c:/windows/logs/meanjs',
    fileName: 'access-%DATE%.log',
    frequency: 'daily',
    verbose: false
  }
}

We could use these settings to build the options in the logger config that is passed along to Morgan.

mleanos · 2015-10-19T07:24:01Z

I guess the above won't work, since I can't pass both streams into Morgan at the same time :) I'll keep at it.

mleanos · 2015-10-19T11:27:44Z

@jloveland @lirantal I have implemented the Rotating Logs functionality. This still needs a little bit of work, and I need to write tests for the Rotating logs. But I wanted to get this out for feedback.

I had to re-work the Stream options to account for the Rotating Logs. How is this new implementation looking?

I've also refactored the tests to be more maintainable. There's one test in particular that I'm having an issue with. I have disabled this test for now, but would love to know if anyone has any insight into this. I'm getting a ENOTEMPTY error from the fs.rmdir call.

codydaig · 2015-10-19T14:18:33Z

@mleanos I think you have to empty the directory before you can remove the directory.

mleanos · 2015-10-19T16:34:47Z

@codydaig I'm using unlink before the fs.rmdir call to get rid of the file I added. Shouldn't that be sufficient?

codydaig · 2015-10-19T16:44:58Z

@mleanos I would assume so as well. Sorry, I didn't get a chance to look at the code before I made that comment, just going off the error message.

lirantal · 2015-10-19T19:34:09Z

Quite a lot of logic to add for the log rotation. Is there no package for that on npm that we can rely on and complicate the logic less?

mleanos · 2015-10-19T20:19:54Z

@lirantal What logic are you referring to?

Don't we want to offer the option of using a single file log, or a rotating log?

lirantal · 2015-10-19T20:20:55Z

Yes that's the point. I was wondering if there's some package to handle that so we don't need to start messing around with deleting files, creating directories, etc.

ilanbiala · 2015-10-19T20:51:14Z

package.json

@@ -34,6 +34,7 @@
    "crypto": "0.0.3",
    "express": "^4.13.1",
    "express-session": "^1.11.3",
+    "file-stream-rotator": "0.0.6",


mleanos · 2015-10-19T21:09:39Z

@lirantal I agree. I'm looking into possible packages to use. One problem is the unit testing though. If we want to write Unit Tests against the logging, then we will need to remove the test logs after the tests are complete. I'm not seeing a solution to that particular issue, unless a package offers this out of the box.

jloveland · 2015-10-20T02:33:27Z

I tested these features and they LGTM!

mleanos · 2015-10-20T03:39:21Z

@lirantal I ended up implementing mock-fs into the tests here; using this we don't have to worry about cleaning up files/directories after the tests.

Thank you for testing with me @jloveland It's much appreciated :)

We still have to make sure the provided log directory is created, if it doesn't exist. There's no way around it, but I don't think it will be an issue.
https://github.com/meanjs/mean/pull/999/files#diff-c31b71d5f97621da6a2b4c696f198f49R63
https://github.com/meanjs/mean/pull/999/files#diff-c31b71d5f97621da6a2b4c696f198f49R84

@codydaig @ilanbiala @lirantal @jloveland Let me know what you think of these changes, and if we're satisfied I can squash.

codydaig · 2015-10-20T04:07:54Z

LGTM

ilanbiala · 2015-10-20T16:54:23Z

package.json

@@ -112,6 +113,7 @@
    "karma-ng-html2js-preprocessor": "^0.1.2",
    "karma-phantomjs-launcher": "~0.2.0",
    "load-grunt-tasks": "^3.2.0",
+    "mock-fs": "^3.4.0",


~, we are moving away from ^.

ilanbiala · 2015-10-20T17:01:56Z

@mleanos pretty much LGTM, just looking at tests and I'm wondering if we can add a bit more coverage for those edge cases? Ideally we hit every line, though I'm not sure how to test console.log being executed.

https://coveralls.io/builds/3890174/source?filename=..%2Fconfig%2Flib%2Flogger.js

codydaig · 2015-10-20T17:13:52Z

@ilanbiala @mleanos Something along these lines should work. I hope I remember the exact syntax correctly.

var spy = sinon.spy(console, 'log');

spy.firstCall.calledWith();
spy.secondCall.calledWith(chalk.yellow('Warning: An invalid format was provided. The logger will use the default format of "' + format + '"'));
spy.ThirdCall.calledWith();

console.log.restore();

mleanos · 2015-10-20T19:46:44Z

@codydaig Thank you for the suggestion. I'll try to implement this. We have a few other areas of the app where this would be useful.

@lirantal

Adds the log options, and format to the Morgan middleware in the Express configuration. These options are defined in the environment configurations. The implementation derived from meanjs#254 by @lirantal, which somehow got overlooked when merging 0.4.0 into master. Added tests for the Logger configuration. Added the log settings to the Test env config. Added environment variables for the log settings in the Test & Production env configs. Moved the Morgan Express middleware outside of the NODE_ENV === 'development' check. Morgan should be used in all environments, and use the settings set in each env config. Changed the wording of the Stream option comments in the env configs. Added Rotating Logs functionality, and refactored the log Stream options. Added a new npm package, FileStreamRotator, for use with Morgan's rotating logs functionality. Also, refactored the log configuration tests to be more maintainable. Added more tests, and refactored test suite to use mock-fs.

mleanos · 2015-10-21T04:10:04Z

@codydaig @ilanbiala I looked into using Sinon, and it was quite handy. It turns out that using Sinon's sandbox feature, it's possible to mute all console logs for the duration of a test/test-suite. I think this will really be useful in a few different areas of our application.

I'd like to look at incorporating the muting of the console logs into our test coverage, but more on a global test coverage scope. Let's not worry about adding the extra coverage for the lines that log to console in this PR. I think it would be better to address it separately.

Part of my reasoning to wait on using Sinon, is because currently we're checking if NODE_ENV === 'test' in order to log to the console or not. If I implemented the muting in just these tests, then we would end up logging to the console when the Morgan logs are implemented via other test suites.

@ilanbiala I addressed the mock-fs dependency comment you left.

I have rebased & squashed since this should be ready now.

ilanbiala · 2015-10-21T15:09:41Z

Did another once over, LGTM.

codydaig · 2015-10-23T22:23:14Z

LGTM

[bug] Enable log options for Morgan that were missed in the 0.4.0 merge

codydaig mentioned this pull request Oct 17, 2015

[bug] Logger wasn't using the logger config option #982

Closed

ilanbiala added the enhance:feature label Oct 17, 2015

ilanbiala added this to the 0.4.2 milestone Oct 17, 2015

ilanbiala assigned codydaig Oct 17, 2015

mleanos force-pushed the morgan-logger-config branch from bae1ad4 to 1f4cc3b Compare October 17, 2015 07:51

codydaig reviewed Oct 17, 2015
View reviewed changes

mleanos force-pushed the morgan-logger-config branch from 1f4cc3b to b3d5af4 Compare October 17, 2015 21:36

jloveland reviewed Oct 18, 2015
View reviewed changes

mleanos force-pushed the morgan-logger-config branch from b3d5af4 to 5b83f3f Compare October 18, 2015 23:14

jloveland reviewed Oct 19, 2015
View reviewed changes

mleanos force-pushed the morgan-logger-config branch from 5b83f3f to cf81e22 Compare October 19, 2015 11:20

ilanbiala reviewed Oct 19, 2015
View reviewed changes

ilanbiala reviewed Oct 20, 2015
View reviewed changes

mleanos force-pushed the morgan-logger-config branch from cd95f35 to 8cd2291 Compare October 21, 2015 04:04

codydaig added a commit that referenced this pull request Oct 23, 2015

Merge pull request #999 from mleanos/morgan-logger-config

a6b3f14

[bug] Enable log options for Morgan that were missed in the 0.4.0 merge

codydaig merged commit a6b3f14 into meanjs:master Oct 23, 2015

codydaig mentioned this pull request Oct 23, 2015

Added logging of time, username, and origin on failed signin. #975

Closed

mleanos deleted the morgan-logger-config branch October 24, 2015 02:40

[bug] Enable log options for Morgan that were missed in the 0.4.0 merge #999

[bug] Enable log options for Morgan that were missed in the 0.4.0 merge #999

Conversation

mleanos commented Oct 17, 2015

mleanos commented Oct 17, 2015

mleanos commented Oct 17, 2015

mleanos commented Oct 17, 2015

lirantal commented Oct 17, 2015

mleanos commented Oct 17, 2015

lirantal commented Oct 17, 2015

mleanos commented Oct 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mleanos commented Oct 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mleanos commented Oct 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jloveland commented Oct 19, 2015

lirantal commented Oct 19, 2015

mleanos commented Oct 19, 2015

mleanos commented Oct 19, 2015

mleanos commented Oct 19, 2015

mleanos commented Oct 19, 2015

codydaig commented Oct 19, 2015

mleanos commented Oct 19, 2015

codydaig commented Oct 19, 2015

lirantal commented Oct 19, 2015

mleanos commented Oct 19, 2015

lirantal commented Oct 19, 2015

Choose a reason for hiding this comment

mleanos commented Oct 19, 2015

jloveland commented Oct 20, 2015

mleanos commented Oct 20, 2015

codydaig commented Oct 20, 2015

Choose a reason for hiding this comment

ilanbiala commented Oct 20, 2015

codydaig commented Oct 20, 2015

mleanos commented Oct 20, 2015

mleanos commented Oct 21, 2015

ilanbiala commented Oct 21, 2015

codydaig commented Oct 23, 2015