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

npm loglevel should be left as default #528

Closed
jasonkarns opened this issue Sep 18, 2017 · 11 comments
Closed

npm loglevel should be left as default #528

jasonkarns opened this issue Sep 18, 2017 · 11 comments

Comments

@jasonkarns
Copy link
Member

There are a long list of arguments why docker shouldn't be changing npm's default and a lot of vocal support for removing the verbose setting. Now that it's been over a year since this was last discussed, I would like to reopen the discussion to consider returning to npm's default.

  • conflicting with npm's own default setting is jarring and violates principle of least surprise
  • warn already prints (typically) enough info during a failure case. the scenarios where info is actually necessary to debug a failure are vanishingly slim. indeed, failures themselves are the exception, so info is necessary in a fraction of a fraction of cases
  • the env var can't be overridden with .npmrc or npm config set because env vars take precedence. thus the only way to get back to npm's default is to set our own env var
    -the vast majority of people are now forced to manually undo a setting for the tiny percentage that want it to be different than npm's default

And most importantly, contrary to the documented rationale for this setting:

  • it's possible to get the full debug log during a failure without being overly verbose to STDOUT

At the very least, this setting should not be set via environment variable, but rather via the builtin config file. $PREFIX/etc/npmrc is specifically for distributions to customize defaults to their liking. Using this would allow users to force it back to npm's default using either user or project .npmrc, instead of forcing users to override using env var or cli flag.

@LaurentGoderre
Copy link
Member

You could view this is an issue with npm itself giving precedence to the env var as opposed to the explicit file. However, since the multi stage has been added to npm, I'm inclined to think that the settings should probably be added as desired to builder images instead of having on by default for the base image.

@SimenB
Copy link
Member

SimenB commented Sep 19, 2017

I don't think it's an issue if we stop setting it in this image. I personally just use yarn anyways, and I've never missed more verbose logging there.

That said, people expect more verbose stuff on CI where they can't access the logfile if something goes wrong, see e.g. travis-ci/travis-ci#7991

You could view this is an issue with npm itself giving precedence to the env var as opposed to the explicit file.

I disagree, it makes sense that NPM_CONFIG_LEVEL=debug npm install overwrites a setting from a file on disk.

@LaurentGoderre
Copy link
Member

I had to disable the verbose on my CI because the logs exceed the maximum allowed log of Travis.

@LaurentGoderre
Copy link
Member

Beside I don't think it's completely unreasonable to require CI to use a slightly customized image.

@chorrell
Copy link
Contributor

I also agree: let's revert back to the npm default

@Starefossen
Copy link
Member

You don't have to modify the image, just set the NPM_CONFIG_LEVEL environment variable, all CI servers support setting environment variables out of the box.

@chorrell
Copy link
Contributor

That's fine, and it's documented in the README: https://github.com/nodejs/docker-node#verbosity

But the issue here is why override the npm defaults when users can set NPM_CONFIG_LOGLEVEL info themselves if they need more verbose logging (e.g., in CI)?

I'd prefer if we just reverted to the default npm logging and change the docs to reflect that. I think this issue will keep coming up and I don't think the the rationale for setting the log level to warn is strong enough.

@LaurentGoderre
Copy link
Member

LaurentGoderre commented Sep 20, 2017

@chorrell, @Starefossen commented to me that you even wouldn't need a custom image for CI.

@jasonkarns
Copy link
Member Author

jasonkarns commented Sep 20, 2017

@Starefossen right, so the question is what value is there in the base image setting the loglevel such that (what seems like) a vast majority of people must then manually override? When it's just as easy for the minority of people to set it to info if they need to? The default should match the most common use case; which IME (and seemingly others according to the old thread and this one; not to mention npm's rationale for it being their default, too) is overwhelmingly in favor of warn.

(At my current client, there are hundreds of Jenkins CI config files using this base image container and virtually every single one of them is setting loglevel back to npm's default. That's a clear indication that the image default is wrong if 99% of users need to unset it.)

@Starefossen
Copy link
Member

Starefossen commented Sep 20, 2017

npm assumes that you are not running inside a container, it writes all log outputs to the npm-debug.log file which is unaccessible for most when running in a containerized environment, for many having a detailed enough log sent to stdout/stderr is their only means of debugging; especially in a CI environment.

@jasonkarns I would kindly ask you to provide us the necessary data for concluding with majority and minority in this case. We are fully aware that a small number of users have asked about the debug level, but the rest of our millions of Node.js Docker users [1] have not weighed in on this discussion. This does not tell us anything about who is the minority and who is the majority in this case though.

[1] node is the 11th most downloaded Docker Image on the official Docker Registry: https://hub.docker.com/explore/

@jasonkarns
Copy link
Member Author

Indeed. The only two numbers I can gauge on are:

  • actual use at my client, which shows hundreds of CI configurations, across hundreds of apps and libraries (and different teams with different opinions), ~90% of which force loglevel back to warn, the other ~10% of which force it to silent. (so either preferring npm's default, or preferring it even quieter than npm's default. virtually none of which actually prefer the docker image default)
  • the number of people who have commented on the original issue (Excessive npm log output in 4.x #57) and this one seem overwhelmingly in favor of using npm's default. Of course, vocal support is usually in favor of change (the status quo rarely triggers noise).

Question: Is this base image supposed to be catering to CIs at the expense of non CI use? Overriding npm's default is jarring and violates principle of least surprise. If it's not suppose to be catering to CI use, then I would kindly suggest that CI users set the env var themselves to their liking (which is trivial in virtually every CI system, as you mentioned). Indeed, my client's use is almost exclusively for CI, and we still prefer npm's default.

Underlying this whole discussion is the assumption that info level logging is even preferred for tracing errors. Others have already mentioned that warn level is more than sufficient for identifying errors, (we're already talking about one level noisier than just the errors themselves, anyway).

If we're focusing this setting for CI users, what are the most common scenarios for failure on CI?

  • failure during npm-install (missing package, bad package.json, etc)
  • failure of the build/compile task
  • failure during npm-test or npm-lint

All of these already emit sufficient logging at warn level. Indeed, I find info level logging harms the debugging process when a failure occurs because the failure line is drastically harder to find amidst the verbose output.

LaurentGoderre added a commit to LaurentGoderre/docker-node that referenced this issue Oct 3, 2017
LaurentGoderre added a commit to LaurentGoderre/docker-node that referenced this issue Oct 3, 2017
LaurentGoderre added a commit to LaurentGoderre/docker-node that referenced this issue Oct 3, 2017
LaurentGoderre added a commit to LaurentGoderre/docker-node that referenced this issue Oct 3, 2017
LaurentGoderre added a commit to LaurentGoderre/docker-node that referenced this issue Oct 3, 2017
LaurentGoderre added a commit to LaurentGoderre/docker-node that referenced this issue Oct 3, 2017
LaurentGoderre added a commit to LaurentGoderre/docker-node that referenced this issue Oct 5, 2017
LaurentGoderre added a commit to LaurentGoderre/docker-node that referenced this issue Oct 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants