feat(logging): Allow log to a file #954

nicojs · 2018-07-06T12:50:37Z

This feature adds the ability to log to a "stryker.log" file using a separate log level.

You can configure it like this:

// stryker.conf.js
module.exports = function(config) {
  config.set({
    fileLogLevel: 'trace' // defaults to 'off'
  });
}

Note: The console logger can still be configured as expected using logLevel: 'info'.

This also adds logging to the stryker api. Plugin creators can now opt into stryker logging using the new logging api:

import { getLogger, Logger } from 'stryker-api/logging';
const logger: Logger = getLogger('myLoggerCategory');

They no longer have to 'role their own config'. Stryker will take care of the logging appearing in the expected places (console and file).

Fixes #748

nicojs · 2018-07-07T01:45:32Z

Finally got a green build! I even ran into a race condition with the log4js logging service. I still want to make the code in the LogConfigurator a bit cleaner though. Otherwise it can be reviewed.

…was not working, instead hope for the best

nicojs · 2018-07-07T11:23:24Z

I've removed the retrying mechanism for getting a free port to log to as it was not working properly. Instead, removed the preference for port 5000 to decrease the likelihood of hitting an EADDRINUSE error.

This PR pins all _dependencies_ and _devDependencies_ on minor versions. I also consolidated all commonly used dependencies to most recent (supported) versions. Namely: * log4js@~1.1.0 (we're upgrading to 2.x with #954) * semver@~5.5.0 * tslib@~1.9.3 Fixes #967

simondel

Could you take a look at my comments? We need to make sure that we update the peer dependencies of packages ourselves to we drop support for older api versions with this change.

simondel · 2018-07-12T13:20:45Z

integrationTest/test/jasmine-jasmine/verify/verify.ts

+    const strykerLog = await fs.readFile('./stryker.log', 'utf8');
+    expect(strykerLog).contains('INFO InputFileResolver Found 2 of 10 file(s) to be mutated');
+    expect(strykerLog).matches(/Stryker Done in \d+ seconds/);
+    // expect(strykerLog).not.contains('ERROR'); TODO


Why is this in a todo?

We get an error after 10 test runs because of this issue: jasmine/jasmine-npm#134. I'll add it as a comment.

simondel · 2018-07-12T13:22:50Z

packages/stryker-api/src/logging/LoggerFactoryMethod.ts

+ * @returns {Logger} instance of logger for the category
+ * @static
+ */
+export default interface LoggerFactoryMethod {


I don't understand why this exists and what it does

I changed the description to:

* Represents a factory to get loggers by category name. * This interface is used to describe the shape of a logger factory method.

Is this better?

simondel · 2018-07-12T13:24:24Z

packages/stryker-api/logging.ts

+export { default as LoggerFactory } from './src/logging/LoggerFactory';
+export { default as Logger } from './src/logging/Logger';
+export { default as LoggerFactoryMethod } from './src/logging/LoggerFactoryMethod';
+export { default as getLogger } from './src/logging/getLogger';


Do we really need getLogger in a separate file or can we also just place the content here?

It is how our entire API is build up right now. Everything in separate files. getLogger is also used from LoggerFactoryMethod and others. You want me to move the entire logging API to this file?

Let's leave it like this for now.

simondel · 2018-07-12T13:26:06Z

packages/stryker-html-reporter/src/HtmlReporter.ts

@@ -1,4 +1,4 @@
-import * as log4js from 'log4js';
+import * as logging from 'stryker-api/logging';


what else do we use from the logging here?

Nope, changing it to a named import.

simondel · 2018-07-12T13:26:21Z

packages/stryker-html-reporter/test/helpers/log4jsMock.ts

@@ -1,4 +1,4 @@
-import * as log4js from 'log4js';
+import * as log4js from 'stryker-api/logging';


This is a log4jsMock file

Good point. I'll change it.

simondel · 2018-07-12T13:45:29Z

packages/stryker/src/logging/LogConfigurator.ts

+  /**
+   * Configure the logging for the server. Includes the master configuration.
+   * This method can only be called ONCE, as it starts the log4js server to listen for log events.
+   * It returns the logging client context that should be used to doe the `forWorker` calls.


What do you mean? There seem to be some errors in the last part of this sentence.

Rephrased it as: It returns the logging client context that should be used to configure the child processes.. I've also renamed the forWorker method to configureChildProcess. Is it clearer now?

simondel · 2018-07-12T13:45:50Z

packages/stryker/src/logging/LogConfigurator.ts

+
+  /**
+   * Configure the logging for the server. Includes the master configuration.
+   * This method can only be called ONCE, as it starts the log4js server to listen for log events.


Can it only be called once or should it only be called once?

simondel · 2018-07-12T13:47:11Z

packages/stryker/src/logging/logUtils.ts

+}
+
+function getLevel(level: LogLevel): log4js.Level {
+  // Needs an any cast here, wrongly typed, see https://github.com/log4js-node/log4js-node/pull/745


Isn't this fixed now?

Awesome! My PR got merged. I've removed this entire method 👍

simondel · 2018-07-12T13:48:21Z

packages/stryker/src/logging/LogConfigurator.ts

+
+  private static createMasterAppenders(consoleLogLevel: LogLevel, fileLogLevel: LogLevel): AppendersConfiguration {
+
+    const multiAppender = { type: require.resolve('./MultiAppender'), appenders: ['filteredConsole'] };


Why require the type like this? What are you trying to do?

That's the log4js way of loading custom appenders. See https://log4js-node.github.io/log4js-node/appenders.html#other-appenders. I'll add the link to the code as well so it's less magical.

simondel · 2018-07-12T13:50:11Z

packages/stryker/src/logging/MultiAppender.ts

+  }
+}
+
+export function configure(config: { appenders: string[] }, _: any, findAppender: (name: string) => RuntimeAppender ): RuntimeAppender {


Where is this used?

It is used by log4js. See https://log4js-node.github.io/log4js-node/writing-appenders.html. I'll add a jsdoc explaining it.

* Add js docs * Add references to log4js documentation * Rename all wrong `log4js` references in plugins to `logging`

nicojs · 2018-07-12T19:30:12Z

Thanks for the great review @simondel . I've implemented the changes, could you take a look?

nicojs · 2018-07-14T07:04:25Z

Just one question left @simondel. I'll try to update the peer dependency version later today as well. (But might be tomorrow)

…f next release

simondel · 2018-07-16T05:55:37Z

Looks good to me! Just one question, do we want to upgrade to log4js 3.0 in this PR or separately?

Refactor code so additional arguments are passed as constructor parameters. Also refactor the integration tests to be more flat and consice.

nicojs · 2018-07-17T06:13:21Z

@simondel I've refactored the code to remove the IsolatedRunnerOptions interface. I also add the dependency to log4js 3.0.0. Could you merge this PR before merging others today? (don't want to keep resolving package.json merge conflicts)

simondel · 2018-07-17T07:00:08Z

Merged 🎉

nicojs added 16 commits May 13, 2018 22:35

feat(Logging api): add logging api

01a9b36

feat(logging-api): first usage of logging api

187855e

Merge branch 'master' into 748-logging-api

a6feb8c

Update log4js 2.7

8e50b4f

Merge branch 'master' into 748-logging-api

09eb2f2

feat: add file log level option to stryker config

c50d69d

chore: add stryker.log to gitignore

98c2bfb

docs(stryker): Document new file log level option

c002d5c

feat(file logging): add logging server

7d2f3df

refactor: remove unused import

6590624

test(logging): Add unit test for multi appender

691e3a7

refactor(stryker): Add semicolons

042117e

test(logging): Add unit tests for the log configurator

188bada

Merge branch 'master' into 748-logging-api

d5d483e

fix(karma-runner): remove logging logic from karma

4fdc250

build(dev depedencies): remove unused types for old log4js

1729794

ghost assigned nicojs Jul 6, 2018

ghost added the 🔎 Needs review label Jul 6, 2018

nicojs added 3 commits July 6, 2018 14:56

chore(karma-runner): Add express.js types back

c14a520

test(stryker-api): fix failing test

0a30c08

test(logging): Remove accidental .only

5a92cc3

nicojs requested a review from sanderkoenders July 6, 2018 15:38

nicojs added 4 commits July 7, 2018 00:25

fix(logging): Remove color from file layout

be5477f

test(logging): Add logging integration test

3339086

refactor(ConfigReader): don't use process.exit

10bed57

fix(taskkill): make killing of tasks more relient on windows

a820577

nicojs force-pushed the 748-logging-api branch from f0204c1 to 701b36a Compare July 7, 2018 00:25

test(api): Improve error logging on integration test

ee1efda

nicojs force-pushed the 748-logging-api branch from 701b36a to ee1efda Compare July 7, 2018 00:27

test(karma-runner): Fix failing test in karma runner after merge

33aa4d9

nicojs added 2 commits July 7, 2018 03:16

fix(logging): Make logging server more robust against race conditions

c2882b9

test(stryker): Higher timeout for integration tests

f59d0c2

refactor: Remove code responsible for retrying different ports as it …

9d6a4f6

…was not working, instead hope for the best

This was referenced Jul 11, 2018

build(deps): update log4js requirement to ^2.10.0 #973

Closed

fix(Dependencies): Pin all deps on minor version #974

Merged

Remove side effects from stryker plugins #667

Closed

simondel requested changes Jul 12, 2018

View reviewed changes

nicojs added 4 commits July 12, 2018 17:25

Merge branch 'master' into 748-logging-api

82db388

feat(jest-runner): Use new logging API

145e583

build(karma-runner): Add express types back again

d10b523

refactor(logging): Implement review comments

cbe94cc

* Add js docs * Add references to log4js documentation * Rename all wrong `log4js` references in plugins to `logging`

nicojs added 2 commits July 14, 2018 14:23

Merge branch 'master' into 748-logging-api

18859d6

fix(stryker-api): Update stryker-api peer dependency in preparation o…

0761b79

…f next release

simondel approved these changes Jul 16, 2018

View reviewed changes

nicojs added 4 commits July 17, 2018 08:00

refactor(isolated-runner): Remove IsolatedRunnerOptions

07cc2b1

Refactor code so additional arguments are passed as constructor parameters. Also refactor the integration tests to be more flat and consice.

Merge branch 'master' into 748-logging-api

bafa8e8

refactor: add semicolon

256f8ab

fix(log4js): update to log4js 3

6f64f1d

simondel merged commit c2f6b82 into master Jul 17, 2018

ghost removed the 🔎 Needs review label Jul 17, 2018

simondel deleted the 748-logging-api branch July 17, 2018 07:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(logging): Allow log to a file #954

feat(logging): Allow log to a file #954

nicojs commented Jul 6, 2018 •

edited

Loading

nicojs commented Jul 7, 2018

nicojs commented Jul 7, 2018 •

edited

Loading

simondel left a comment

simondel Jul 12, 2018

nicojs Jul 12, 2018

simondel Jul 12, 2018

nicojs Jul 12, 2018

simondel Jul 12, 2018

nicojs Jul 12, 2018

simondel Jul 14, 2018

simondel Jul 12, 2018

nicojs Jul 12, 2018

simondel Jul 12, 2018

nicojs Jul 12, 2018

simondel Jul 12, 2018

nicojs Jul 12, 2018

simondel Jul 12, 2018

simondel Jul 12, 2018

nicojs Jul 12, 2018

simondel Jul 12, 2018

nicojs Jul 12, 2018

simondel Jul 12, 2018

nicojs Jul 12, 2018

nicojs commented Jul 12, 2018

nicojs commented Jul 14, 2018

simondel commented Jul 16, 2018

nicojs commented Jul 17, 2018

simondel commented Jul 17, 2018

		@@ -1,4 +1,4 @@
		import * as log4js from 'log4js';
		import * as logging from 'stryker-api/logging';

		@@ -1,4 +1,4 @@
		import * as log4js from 'log4js';
		import * as log4js from 'stryker-api/logging';


		private static createMasterAppenders(consoleLogLevel: LogLevel, fileLogLevel: LogLevel): AppendersConfiguration {

		const multiAppender = { type: require.resolve('./MultiAppender'), appenders: ['filteredConsole'] };

feat(logging): Allow log to a file #954

feat(logging): Allow log to a file #954

Conversation

nicojs commented Jul 6, 2018 • edited Loading

nicojs commented Jul 7, 2018

nicojs commented Jul 7, 2018 • edited Loading

simondel left a comment

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

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

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

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

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

Choose a reason for hiding this comment

nicojs commented Jul 12, 2018

nicojs commented Jul 14, 2018

simondel commented Jul 16, 2018

nicojs commented Jul 17, 2018

simondel commented Jul 17, 2018

nicojs commented Jul 6, 2018 •

edited

Loading

nicojs commented Jul 7, 2018 •

edited

Loading