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.

Sign up for GitHub

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

Jump to bottom

fix(file-reporter): output logs incrementally #5011

Closed

william2958 wants to merge 4 commits into pnpm:main from william2958:fix/4949

Contributor

william2958 commented Jul 11, 2022

Resolves issue #4949 where pnpm install will crash in a very large repository as the logs object in memory becomes too large for JSON.stringify to handle. Performance has been tested on a large monorepo and this change does not seem to hinder installation time in any measurable way as compared to not logging at all.

The output log format has been changed from:

{
  "0 info pnpm": "foo",
  "1 warn pnpm": {
    "foo": 1,
    "bar": 2
  }
}

to:

{
  "message": "foo",
  "key": "0 info pnpm"
}
{
  "foo": 1,
  "bar": 2,
  "key": "1 warn pnpm"
}

Where each object is written to the file incrementally, instead of storing all logs and writing all at once.

Note: because we are using fs.appendFileSync which does not overwrite previous files, the log file must be cleaned up before any logging.


          fix(file-reporter): output logs incrementally

1df70ff

william2958 requested a review from zkochan as a code owner

July 11, 2022 18:48

welcome bot commented Jul 11, 2022

💖 Thanks for opening this pull request! 💖
Please be patient and we will get back to you as soon as we can.

william2958 added 2 commits

July 11, 2022 16:39


          fix(file-reporter): remove file if successful

ff727f2


          chore: clean up code

ba20445

Member

zkochan commented Jul 11, 2022

Run pnpm changeset in the root of the repository and describe your changes. The resulting files should be committed as they will be used during release.


          chore: add changeset

62151ef

zkochan requested changes

View reviewed changes

packages/file-reporter/src/index.ts

                 streamParser['on']('data', function (logObj: Object) {
-                  if (isUsefulLog(logObj)) {
-                    logs.push(logObj)
+                  if (isUsefulLog(logObj) && global['writeDebugLogFile'] !== false) {

Member

zkochan Jul 11, 2022

I think if global['writeDebugLogFile'] is false, we can early return from the outer function at line 7

packages/file-reporter/src/index.ts

+                    const msgobj = getMessageObj(logObj)
+                    const prettyLogs = prettify(msgobj)
+                    const jsonLogs = JSON.stringify(prettyLogs, null, 2) + '\n'
+                    const dest = fs.existsSync(path.dirname(LOG_FILENAME)) ? LOG_FILENAME : path.basename(LOG_FILENAME)

Member

zkochan Jul 11, 2022

I don't think it is optimal to check the existence of the file hundreds of times.

Member

zkochan Jul 11, 2022

also, why are there two possible locations for the logs?

packages/file-reporter/src/index.ts

+                    const prettyLogs = prettify(msgobj)
+                    const jsonLogs = JSON.stringify(prettyLogs, null, 2) + '\n'
+                    const dest = fs.existsSync(path.dirname(LOG_FILENAME)) ? LOG_FILENAME : path.basename(LOG_FILENAME)
+                    fs.appendFileSync(dest, jsonLogs)

Member

zkochan Jul 11, 2022

is this the best way to do this? Or would it be faster to open a write stream? That would keep up the connection to the file, I guess.

Member

zkochan Jul 11, 2022

we can probably look at how winston does it

packages/file-reporter/src/index.ts

                 function getMessageObj (logobj: Object): Object {
-                  const msgobj = {}
+                  const msgobj: { [key: string]: string } = {}
                   for (const key in logobj) {
                     if (['time', 'hostname', 'pid', 'level', 'name'].includes(key)) continue

Member

zkochan Jul 11, 2022

Suggested change

      
                  if (['time', 'hostname', 'pid', 'level', 'name'].includes(key)) continue
          
                  if (['time', 'hostname', 'pid'].includes(key)) continue

packages/file-reporter/src/index.ts

                   for (const key in logobj) {
                     if (['time', 'hostname', 'pid', 'level', 'name'].includes(key)) continue
                     msgobj[key] = logobj[key]
                   }
+                  const logLevel: string = logobj['level']
+                  const logName: string = logobj['name']
+                  msgobj.key = `${logNum} ${logLevel} ${logName}`

Member

zkochan Jul 11, 2022

this field can be removed.

Member

zkochan commented Jul 11, 2022

TBH, maybe we don't even need these logs. They are not very useful. I think we can remove them. Or we can just write the stacktrace of the error to the log file

Contributor Author

william2958 commented Jul 11, 2022

TBH, maybe we don't even need these logs. They are not very useful. I think we can remove them. Or we can just write the stacktrace of the error to the log file

Okay in that case I will open another PR that looks to remove the logging entirely - for this change should I just remove the file-reporter package entirely?

Member

zkochan commented Jul 11, 2022

I guess, yes

Member

octogonz commented Jul 11, 2022

Or we can just write the stacktrace of the error to the log file

FWIW I've used PNPM for many years and had no idea that this log file even existed. 😄 The output is written under node_modules with a hidden filename. If we decide to improve this feature (rather than simply removing it), perhaps the log file should be documented and given a more conspicuous filename.

But if that additional work would delay the fix of the RangeError crash, maybe it should be a separate work item. (?)

Contributor Author

william2958 commented Jul 11, 2022

I guess, yes

PR for removing file-reporter package: #5012

zkochan closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment