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

[feat] Custom logging location #4466

Closed
1 task done
EvanCarroll opened this issue Dec 2, 2021 · 9 comments · Fixed by #4594
Closed
1 task done

[feat] Custom logging location #4466

EvanCarroll opened this issue Dec 2, 2021 · 9 comments · Fixed by #4594
Labels
Enhancement new feature or improvement

Comments

@EvanCarroll
Copy link

EvanCarroll commented Dec 2, 2021

Before Opening Please...

  • Search for an existing/duplicate RRFC which might be relevant to your RRFC

There is one RFC about logging: using STDERR for errors instead of STDOUT. This is unrelated.

Motivation ("The Why")

Currently some CI providers, specifically GitLab, restrict job artifacts to being relative to the repository. Presumably, this stops you from uploading runner files that may have dirty-state left from previous runs in non-containerized workflows. Specifically the GitLab documents say,

The paths keyword determines which files to add to the job artifacts. All paths to files and directories are relative to the repository where the job was created.

This effectively means NPM logs on error can not be captured as job artifacts.

Example

How

Current Behaviour

What I would like to do is something like this (ignore the actual error)

$ npx ng e2e
npm ERR! could not determine executable to run
npm ERR! A complete log of this run can be found in:
npm ERR!     /root/.npm/_logs/2021-11-22T17_24_50_085Z-debug.lo

Then I want to upload /root/.npm/_logs/2021-11-22T17_24_50_085Z-debug.log as a job artifact.

This is not currently possible

Desired Behaviour

In light of GitLab not permitting global files to be uploaded as job artifacts I would like an option to set the log file,

npx [--log <LOCATION>] ng e2e

Such that LOCATION could be relative to the cwd.

Then I could do,

$ npx --log "./.npm/_logs/" ng e2e

And set ./.npm/_logs/* as my artifacts to upload to GitLab.

References

@ljharb
Copy link
Contributor

ljharb commented Dec 2, 2021

What difficulties are there with copying the log to the desired location prior to creating the artifact?

@EvanCarroll
Copy link
Author

Does the RFC process require proving difficulty, or mere inconvenience and improvement?

Nothing is difficult or impossible with an open operating system.

@ljharb
Copy link
Contributor

ljharb commented Dec 2, 2021

I don't know about a requirement, but any addition to anything should be weighed against the alternatives without it. In other words, every convenience/improvement for one group is an inconvenience/detriment for another, so subjective decisions always need to be made.

If the delta between:

npm foo --log=$logdir/$filename bar
# or
NPM_CONFIG_LOG=$logdir/$filenamenpm foo bar

and

npm foo bar
cp $HOME/.npm/_logs $logdir

doesn't feel like it outweighs the downside of the added complexity to npm, then that would be a very important consideration for the RFC.

@whschultz
Copy link

If we have multiple npm commands running at the same time, being able to specify a custom log location for each one individually would help ensure that we know which log goes with which command.

@lukekarrys
Copy link
Contributor

I would be in favor of this. I would use it in the cases described above but also when debugging as I often do npm <COMMAND> --cache=. when all I really want is to have the logs in a separate place to compare across runs without messing with any of my "real" logs.

Some questions I have about expected behavior @EvanCarroll:

  • We currently clean out the logs directory with the logs-max config option (which defaults to 10). If supplying your own logdir would you expect that to ever be cleaned out for you? I would lean towards no.
  • npm expects to own whatever cache directory it uses (which is where logs are currently stored). So something like this is run when creating a logfile:
const dir = path.dirname(logfile)
mkdirpInferOwner.sync(dir)
fs.openSync(logfile, 'a')
const st = fs.lstatSync(dir)
fs.chownSync(dir, st.uid, st.gid)
fs.chownSync(file, st.uid, st.gid)

If supplying your own logdir, would you expect anything like that to happen? Or to error if npm can't access the directory? I would lean towards not doing anything special to the directory or logfile and erroring if npm can't write to it.

@lukekarrys
Copy link
Contributor

If we have multiple npm commands running at the same time, being able to specify a custom log location for each one individually would help ensure that we know which log goes with which command.

I agree with this use case being another beneficial reason to add this option, but I don't think the filename should be made configurable, only the directory. npm has some logic to prevent runaway logfile writing in the cases of infinite loops that involves changing the log file name, so passing a complete filename wouldn't actually give you the file being written.

@EvanCarroll
Copy link
Author

EvanCarroll commented Dec 29, 2021

We currently clean out the logs directory with the logs-max config option (which defaults to 10). If supplying your own logdir would you expect that to ever be cleaned out for you? I would lean towards no.

I have no strong preference on this as ultimately I'm just logging for the CI run. However I would treat them the same only so in the future this may be totally configurable in some future .npmrc where you can specify your log directory, and other options like rotation/cleanup. In the traditional Linux FHS scheme, it seems likely that some users will want these logs on different partitions like /var/log or following something like XDG's $XDG_STATE_HOME. Perhaps this is something to note npm/rfcs#427

If supplying your own logdir, would you expect anything like that to happen? Or to error if npm can't access the directory? I would lean towards not doing anything special to the directory or logfile and erroring if npm can't write to it.

I would not chown things I didn't create. I would error if I couldn't access them or if I had reason to believe they were egregiously misconfigured or otherwise likely to cause a security problem (this is what SSH does if you chmod your private key improperly ~/.ssh/id_rsa).

@lukekarrys
Copy link
Contributor

lukekarrys commented Jan 12, 2022

Thanks @EvanCarroll, I agree with both those points.

I can make an actual RFC for this based on the current discussion if it gets accepted, but I'm a +1 on it. To summarize the current implementation being discussed:

  • Add a config item that will write log files to that directory instead of the default <NPM_CACHE>/_logs
  • Setting this config will not change the behavior of the logs-max behavior or rotating of logfiles
  • Setting this config will not create or chown the directory but instead error if its not accessible or doesnt exist

@darcyclarke darcyclarke removed the Agenda will be discussed at the Open RFC call label Jan 17, 2022
@lukekarrys lukekarrys self-assigned this Jan 20, 2022
@lukekarrys lukekarrys transferred this issue from npm/rfcs Feb 23, 2022
@lukekarrys lukekarrys added the Enhancement new feature or improvement label Feb 23, 2022
@lukekarrys lukekarrys changed the title [RRFC] Custom logging location [feat] Custom logging location Feb 23, 2022
@lukekarrys
Copy link
Contributor

This ended up not needing an explicit RFC so I transferred to the cli repo as an enhancement

lukekarrys added a commit that referenced this issue Mar 22, 2022
This also allows logs-max to be set to 0 to disable log file writing.

Closes #4466
Closes #4206
lukekarrys added a commit that referenced this issue Mar 23, 2022
This also allows logs-max to be set to 0 to disable log file writing.

Closes #4466
Closes #4206
lukekarrys added a commit that referenced this issue Mar 23, 2022
This also allows logs-max to be set to 0 to disable log file writing.

Closes #4466
Closes #4206
lukekarrys added a commit that referenced this issue Mar 23, 2022
This also allows logs-max to be set to 0 to disable log file writing.

Closes #4466
Closes #4206
fritzy pushed a commit that referenced this issue Mar 24, 2022
This also allows logs-max to be set to 0 to disable log file writing.

Closes #4466
Closes #4206
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement new feature or improvement
Projects
None yet
5 participants