Skip to content

fix: use @npmcli/redact for log redactions - #7334

Merged
lukekarrys merged 2 commits into
latestfrom
lk/redact
Apr 3, 2024
Merged

fix: use @npmcli/redact for log redactions#7334
lukekarrys merged 2 commits into
latestfrom
lk/redact

Conversation

@lukekarrys

@lukekarrys lukekarrys commented Apr 3, 2024

Copy link
Copy Markdown
Contributor

Closes #7314

This does the same thing as #7314 except using a new shared package for the log cleaning @npmcli/redact.

Here are the perf numbers from my local machine after setting a script with npm pkg set scripts.echo="echo". Note that the before seems slower on my machine than previous benchmarks, I think due to some security software that affects filesystem IO performance.

Before

❯ hyperfine --warmup 3 "node ./bin/npm-cli.js run echo"
Benchmark 1: node ./bin/npm-cli.js run echo
  Time (mean ± σ):     349.4 ms ±  20.2 ms    [User: 172.5 ms, System: 42.7 ms]
  Range (min … max):   319.0 ms … 376.1 ms    10 runs

After

❯ hyperfine --warmup 3 "node ./bin/npm-cli.js run echo"
Benchmark 1: node ./bin/npm-cli.js run echo
  Time (mean ± σ):     170.7 ms ±   4.7 ms    [User: 114.7 ms, System: 22.6 ms]
  Range (min … max):   163.6 ms … 184.9 ms    16 runs

@lukekarrys
lukekarrys requested a review from a team as a code owner April 3, 2024 12:45
@lukekarrys

Copy link
Copy Markdown
Contributor Author

cc @H4ad

@H4ad H4ad left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for promoting this, great PR and great improvements, also kudos to @sirenkovladd for finding this first!

@lukekarrys

Copy link
Copy Markdown
Contributor Author

@H4ad curious what benchmarks show in this PR for you compared to #7314?

@H4ad

H4ad commented Apr 3, 2024

Copy link
Copy Markdown
Contributor

Before:

Benchmark 1: node ./bin/npm-cli.js run echo
  Time (mean ± σ):     232.8 ms ±   3.2 ms    [User: 235.4 ms, System: 54.8 ms]
  Range (min … max):   226.4 ms … 237.3 ms    12 runs

After:

Benchmark 1: node ./bin/npm-cli.js run echo
  Time (mean ± σ):     196.0 ms ±   2.5 ms    [User: 179.1 ms, System: 53.8 ms]
  Range (min … max):   192.0 ms … 199.5 ms    14 runs

The new load time for each dependency (top N of most expensive):

MODULE_CJS 60649 [/home/h4ad/Projects/opensource/performance-analysis/npm-cli/node_modules/semver/functions/satisfies.js] [../classes/range]: 6.436ms
MODULE_CJS 60649 [/home/h4ad/Projects/opensource/performance-analysis/npm-cli/node_modules/validate-npm-package-name/lib/index.js] [builtins]: 6.441ms
MODULE_CJS 60649 [/home/h4ad/Projects/opensource/performance-analysis/npm-cli/lib/utils/error-message.js] [validate-npm-package-name]: 6.657ms
MODULE_CJS 60649 [/home/h4ad/Projects/opensource/performance-analysis/npm-cli/lib/cli-entry.js] [semver/functions/satisfies]: 6.811ms
MODULE_CJS 60649 [/home/h4ad/Projects/opensource/performance-analysis/npm-cli/node_modules/npmlog/lib/log.js] [gauge]: 7.489ms
MODULE_CJS 60649 [/home/h4ad/Projects/opensource/performance-analysis/npm-cli/node_modules/normalize-package-data/lib/fixer.js] [validate-npm-package-license]: 7.668ms
MODULE_CJS 60649 [/home/h4ad/Projects/opensource/performance-analysis/npm-cli/lib/utils/exit-handler.js] [./error-message.js]: 7.684ms
MODULE_CJS 60649 [/home/h4ad/Projects/opensource/performance-analysis/npm-cli/lib/utils/log-shim.js] [npmlog]: 9.684ms
MODULE_CJS 60649 [/home/h4ad/Projects/opensource/performance-analysis/npm-cli/lib/utils/exit-handler.js] [./log-shim.js]: 10.266ms
MODULE_CJS 60649 [/home/h4ad/Projects/opensource/performance-analysis/npm-cli/workspaces/config/lib/index.js] [@npmcli/map-workspaces]: 10.349ms
MODULE_CJS 60649 [/home/h4ad/Projects/opensource/performance-analysis/npm-cli/node_modules/@npmcli/package-json/lib/normalize.js] [normalize-package-data/lib/fixer.js]: 10.578ms
MODULE_CJS 60649 [/home/h4ad/Projects/opensource/performance-analysis/npm-cli/lib/npm.js] [@npmcli/config]: 15.221ms
MODULE_CJS 60649 [/home/h4ad/Projects/opensource/performance-analysis/npm-cli/node_modules/@npmcli/package-json/lib/index.js] [./normalize.js]: 17.558ms
MODULE_CJS 60649 [/home/h4ad/Projects/opensource/performance-analysis/npm-cli/node_modules/@npmcli/run-script/lib/run-script.js] [@npmcli/package-json]: 18.414ms
MODULE_CJS 60649 [/home/h4ad/Projects/opensource/performance-analysis/npm-cli/lib/cli-entry.js] [./utils/exit-handler.js]: 18.496ms
MODULE_CJS 60649 [/home/h4ad/Projects/opensource/performance-analysis/npm-cli/lib/commands/run-script.js] [@npmcli/run-script]: 20.343ms
MODULE_CJS 60649 [/home/h4ad/Projects/opensource/performance-analysis/npm-cli/lib/npm.js] [./commands/run-script.js]: 22.623ms
MODULE_CJS 60649 [/home/h4ad/Projects/opensource/performance-analysis/npm-cli/lib/cli-entry.js] [./npm.js]: 25.942ms
MODULE_CJS 60649 [] [/home/h4ad/Projects/opensource/performance-analysis/npm-cli/bin/npm-cli.js]: 60.411ms

@lukekarrys
lukekarrys merged commit 87a61fc into latest Apr 3, 2024
@lukekarrys
lukekarrys deleted the lk/redact branch April 3, 2024 14:24
@github-actions github-actions Bot mentioned this pull request Apr 3, 2024
@wraithgar wraithgar mentioned this pull request Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants