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

Log to a file #748

Closed
nicojs opened this issue May 13, 2018 · 8 comments
Closed

Log to a file #748

nicojs opened this issue May 13, 2018 · 8 comments
Assignees
Labels
🚀 Feature request New feature request

Comments

@nicojs
Copy link
Member

nicojs commented May 13, 2018

For ease of debugging issues, it would be useful to log all messages (log level trace) to a file. Comparable to what npm does: when an npm command fails, the user is informed that a log file can be found in the temp directory.

In order to do this, all stryker packages need to share log4js configuration. In order to do this, we need to move log4js to a shared peer dependency. The only one we have is the stryker-api, so I suggest to add the getLogger method there. We best use our own api and not rely in log4js for this. That way we can choose a different implementation rather easily. The stryker-api should not have a dependency on log4js, rather it would provide a way to set the log implementation. By default, it has a noop implementation and should log a warning (comparable to SLF4J for JVM).

This would also remove the necessity to call setGlobalLogLevel in every plugin to configure the log level on his instance of log4js.

@nicojs nicojs added comitted 🚀 Feature request New feature request labels May 13, 2018
@nicojs
Copy link
Member Author

nicojs commented May 13, 2018

Another way to solve it would be to inject a logger into each plugin. This is what karma does, but that would mean for us to add a dependency injection framework (or alter each interface). I'm not convinced we need that yet.

@ghost ghost assigned nicojs May 13, 2018
@ghost ghost added 🛠 In progress and removed comitted labels May 13, 2018
@nicojs
Copy link
Member Author

nicojs commented May 13, 2018

I've created the first draft of the logging api. @simondel @Archcry could you maybe have a look?

https://github.com/stryker-mutator/stryker/tree/748-logging-api/packages/stryker-api/src/logging

@nicojs
Copy link
Member Author

nicojs commented May 26, 2018

For the file logging: I'm thinking of keeping it simple. Add a config option for the file log level:

/**
   * The log level for logging to a file. If defined, stryker will output a log file called "stryker.log".
   * Default: undefined
   */
  fileLogLevel?: LogLevel;

  /**
   * The log level for logging to the console. Default: "info".
   */
  logLevel?: LogLevel;

@stryker-mutator/contributors any feedback is welcome.

@mthmulders
Copy link
Contributor

@nicojs why two different levels? I would expect (as you pictured in the original idea) that all logging is sent to a file, so that when something goes wrong, that file contains all information that has been collected thus far. The fileLogLevel and its comments suggests that there will be logging iff that variable is defined.

Also, just like SLF4J does, it might be useful to signal when logging is probably not set up correctly. In the case of SLF4J this can happen when there are multiple implementations on the classpath; in our case, it might be when the noop logger isn't overwritten by another one.

@nicojs
Copy link
Member Author

nicojs commented May 26, 2018

I would expect (as you pictured in the original idea) that all logging is sent to a file, so that when something goes wrong, that file contains all information that has been collected thus far

Yeah, I've thought about it. The problem is that I cannot guarantee it doesn't impact performance in a negative way. We still don't have performance tests. I've tested a first implementation on Stryker itself and the log file was 23MB. Do you think that this is a problem?

Also, just like SLF4J does, it might be useful to signal when logging is probably not set up correctly.

Great idea, I'll add it to the original issue

@mthmulders
Copy link
Contributor

I've tested a first implementation on Stryker itself and the log file was 23MB. Do you think that this is a problem?

Allocating 23 MB of disk space with the risk of throwing it away? Personally, it doesn't seem like a big deal to me, if it gave the benefit of very detailed log messages for troubleshooting.

@nicojs
Copy link
Member Author

nicojs commented May 29, 2018

Hmm ok, you've convinced me. It serves the use case of reporting issues. Whenever we have more use cases, we can change it again.

@simondel
Copy link
Member

simondel commented Jun 8, 2018

Let's go for an opt-in setting and place the log file in the current working directory.

simondel pushed a commit that referenced this issue Jul 17, 2018
This feature adds the ability to log to a "stryker.log" file using a separate log level.

You can configure it like this:

```js
// 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:

```ts
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
@ghost ghost removed the 🔎 Needs review label Jul 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚀 Feature request New feature request
Projects
None yet
Development

No branches or pull requests

3 participants