-
Notifications
You must be signed in to change notification settings - Fork 47
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: log tarball retrieval from cache #403
Conversation
npm currently signals where the http request ended up coming from. $ npm view npm --loglevel http
npm http fetch GET 200 https://registry.npmjs.org/npm 284ms (cache revalidated) ~/D/s/install $ npm i abbrev --loglevel http
npm http fetch GET 200 https://registry.npmjs.org/abbrev 216ms (cache revalidated)
npm http fetch GET 200 https://registry.npmjs.org/abbrev/-/abbrev-3.0.0.tgz 123ms (cache miss) It does seem like it does not log the tgz requests if there is a cache hit. This is a bug. ~/D/s/install $ npm i abbrev --loglevel http
npm http fetch GET 200 https://registry.npmjs.org/abbrev 17ms (cache hit)
npm http fetch GET 200 https://registry.npmjs.org/semver 159ms (cache revalidated)
npm http fetch GET 200 https://registry.npmjs.org/typescript 201ms (cache updated)
npm http fetch GET 200 https://registry.npmjs.org/typescript/-/typescript-5.6.3.tgz 413ms (cache miss) |
Ohh I see what's going on. We're bypassing the caching being done in our fetch library and doing it directly. This logging statement may be the right place then. I wonder if this is "technically" an http level logging, since that's typically the log level folks set when debugging http/caching issues. Also, as much as I usually try to avoid "bike shedding" I think the output of this should maybe be closer to that of the http/fetch log lines. |
Yes, something http entry for tarball cache hits is missing.
In general, the mix of HTTP and local caching details in one log entry is confusing. If the cache is involved, no HTTP GET requests are sent (an HTTP cache hit would typically result in a 304). It might be better to separate
|
100%
This is a great suggestion. |
i will give it a try |
Part 2 is here npm/npm-registry-fetch#273 The It should look like this:
|
This will get tests passing. Not sure why they're not running in CI right now. /D/n/p/b/pr (tarball_cache_log|✚1) $ git diff
diff --git a/test/fetcher.js b/test/fetcher.js
index 8b2adb3..765266b 100644
--- a/test/fetcher.js
+++ b/test/fetcher.js
@@ -207,6 +207,10 @@ t.test('extract', t => {
}).extract(target + '/badcache')
.then(({ resolved, integrity }) => {
t.match(logs, [
+ [ 'http',
+ 'cache',
+ /file:test\/fixtures\/abbrev-1.1.1.tgz.*(cache hit)/,
+ ],
['warn', 'tar', 'zlib: incorrect header check'],
[
'silly',
@@ -298,7 +302,11 @@ t.test('extract', t => {
algorithm: 'sha512',
sri: Object,
}, 'got expected error')
- t.same(logs, [
+ t.match(logs, [
+ [ 'http',
+ 'cache',
+ /file:test\/fixtures\/abbrev-1.1.1.tgz.*(cache hit)/,
+ ],
[
'silly',
'tarball', |
Avoid confusion between actual remote fetches and cache hits (which don't involve a fetch). Ref npm/pacote#403 (comment)
@mbtools if you apply that patch I can land this PR. If you can't get to it soon I'll make a new PR and get another npm cli team member to approve it so I can land it. |
I was traveling on Canadian Thanksgiving. Changed it |
🤖 I have created a release *beep* *boop* --- ## [19.0.1](v19.0.0...v19.0.1) (2024-10-15) ### Bug Fixes * [`cbf94e8`](cbf94e8) [#389](#389) prepare script respects scriptshell config (#389) (@milaninfy) * [`2b2948f`](2b2948f) [#403](#403) log tarball retrieval from cache (#403) (@mbtools, @wraithgar) ### Dependencies * [`a9fc4d1`](a9fc4d1) [#405](#405) bump sigstore from 2.2.0 to 3.0.0 (#405) (@bdehamer) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
If tarballs are cached, the npm install log contains no information on where it got the tarball from. This helps troubleshoot issues with caching and
preferOnline/preferOffline
, for example.