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

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

Merged
merged 2 commits into from
Apr 3, 2024
Merged

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

merged 2 commits into from
Apr 3, 2024

Conversation

lukekarrys
Copy link
Contributor

@lukekarrys lukekarrys commented Apr 3, 2024

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
Contributor Author

cc @H4ad

Copy link
Contributor

@H4ad H4ad left a comment

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
Contributor Author

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

@H4ad
Copy link
Contributor

H4ad commented Apr 3, 2024

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
132 checks passed
@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.

None yet

3 participants