fix: prevent totalCommitsFetch error result from being cached #2177

rickstaa · 2022-10-10T11:02:23Z

This PR ensures that when the https://api.github.com/search/commits?q=author:anuraghazra API fails, the result is not cached. This prevents people from creating an issue because their commits are 0 for the 4-hour cache period. After this PR is merged, the 0 total commits should only show up when the error occurs.

vercel · 2022-10-10T11:02:26Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
github-readme-stats	✅ Ready (Inspect)	Visit Preview	Jan 10, 2023 at 10:24AM (UTC)

codecov · 2022-10-10T11:03:36Z

Codecov Report

Base: 96.79% // Head: 97.23% // Increases project coverage by +0.43% 🎉

Coverage data is based on head (6c2d175) compared to base (227711c).
Patch coverage: 94.23% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2177      +/-   ##
==========================================
+ Coverage   96.79%   97.23%   +0.43%     
==========================================
  Files          22       22              
  Lines        3835     3902      +67     
  Branches      328      368      +40     
==========================================
+ Hits         3712     3794      +82     
+ Misses        121      106      -15     
  Partials        2        2

Impacted Files	Coverage Δ
api/index.js	`94.33% <85.71%> (-1.62%)`	⬇️
src/fetchers/stats-fetcher.js	`92.41% <97.36%> (+1.40%)`	⬆️
src/cards/top-languages-card.js	`100.00% <0.00%> (ø)`
src/common/createProgressNode.js	`100.00% <0.00%> (ø)`
api/top-langs.js	`97.67% <0.00%> (+2.43%)`	⬆️
src/common/retryer.js	`98.43% <0.00%> (+19.74%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

This commit makes sure that when the `https://api.github.com/search/commits?q=author:anuraghazra` API fails the result is not cached.

rickstaa · 2023-01-10T12:02:57Z

@anuraghazra, maybe we can merge this this will stop people making issues about incorrect total commit count (See #2026 (comment)).

anuraghazra · 2023-01-14T09:57:57Z

api/index.js

+        }`,
+      );
+    } else {
+      res.setHeader("Cache-Control", `no-cache, no-store, must-revalidate`); // Don't cache unsuccessful responses.


Good idea, but I'm just worried that this will make it so that vercel will need to execute more fresh invocations causing increased load when PATs get exhausted.

For example say we get 10k fresh requests per day, and we cache 70% of the requests, if PATs get exhausted we will render & cache the error card and after 4hour everything will go back to normal.

But if we do this, all those 10k requests need to be executed by vercel even though they will all respond with error card.

I think ideal solution would be to decrease the cache seconds on errors: instead of no-cache let's do 1h for error responses (1h since cache will invalidate the same time as PATs get revalidate)

That is 100% true I overlooked that fact. Let's close this for now.

@anuraghazra In that case, we should also change the following code back so that it caches the error:

github-readme-stats/api/wakatime.js

Line 84 in 0ff426d

return res.send(renderError(err.message, err.secondaryMessage));

github-readme-stats/api/top-langs.js

Line 81 in 0ff426d

res.setHeader("Cache-Control", `no-cache, no-store, must-revalidate`); // Don't cache error responses.

github-readme-stats/api/pin.js

Line 83 in 0ff426d

res.setHeader("Cache-Control", `no-cache, no-store, must-revalidate`); // Don't cache error responses.

github-readme-stats/api/index.js

Line 96 in 0ff426d

res.setHeader("Cache-Control", `no-cache, no-store, must-revalidate`); // Don't cache error responses.

These were merged some time ago and might also be depleting the PATs. However, I suspect this catch block is only triggered when something goes wrong in our code.

The fetcher handles errors using throw as well as trycatch, so that means that errors with the API as well as with our code should trigger the trycatch in ./api/index.js, right?

Trying it out in the browser with a random (nonexistent) username gives a no-cache header, so you're right; this should be changed back, since it has the same effect as whatever the PRs trying to do.

@Zo-Bro-23 I changed the code. Please review #2448. 👍

Can we close this PR now since #2448 does the same thing?

This pull request handles errors from the REST API, which will write 0 commits to the card when it fails. While the other pull request handles other errors. I will update this one when #2448 is merged.

As far as I understand, API errors will trigger Javascript errors, which means that what the other PR does will apply to API errors too.

github-readme-stats/src/fetchers/stats-fetcher.js

Lines 215 to 218 in 4b17300

throw new CustomError(

"Something went while trying to retrieve the stats data using the GraphQL API.",

CustomError.GRAPHQL_ERROR,

);

The fetcher is throwing a Javascript error, so GraphQL errors should trigger the trycatch block too right? Or am I missing something?

rickstaa · 2023-01-16T11:07:00Z

@anuraghazra Closing this for now. See #2177 (comment), as it might also be important.

rickstaa · 2023-01-20T08:13:10Z

Re-opening this since we can use this code on private Vercel instances.

rickstaa · 2023-01-21T17:46:23Z

Closing this as I think we should switch to throwing an error when the REST API fails and then use stale-if-error to handle the PAT depletion (see #2448 (comment)).

Zo-Bro-23 · 2023-01-22T02:30:06Z

Closing this as I think we should switch to throwing an error when the REST API fails and then use stale-if-error to handle the PAT depletion (see #2448 (comment)).

stale-if-error seems like a great idea! That will stop the PAT error from showing up during downtime, and since downtime is always less than 1 hour, it can automatically refresh the cache after 1 hour.

rickstaa added the high-priority High priority issue or PR. label Oct 10, 2022

rickstaa requested a review from anuraghazra October 10, 2022 11:03

fix: prevent totalCommitsFetch error result from being cached

3123e68

This commit makes sure that when the `https://api.github.com/search/commits?q=author:anuraghazra` API fails the result is not cached.

rickstaa force-pushed the prevent_total_fetch_zero_cache branch from ed60739 to 3123e68 Compare October 18, 2022 18:22

vercel bot deployed to Preview October 18, 2022 18:23 View deployment

rickstaa mentioned this pull request Nov 22, 2022

Total commit number changes when card width is not set to 500 #2284

Closed

rickstaa self-assigned this Jan 9, 2023

rickstaa mentioned this pull request Jan 10, 2023

Hide border option influences commit count #2026

Closed

Merge branch 'master' into prevent_total_fetch_zero_cache

6c2d175

vercel bot deployed to Preview January 10, 2023 10:24 View deployment

rickstaa removed their assignment Jan 10, 2023

anuraghazra reviewed Jan 14, 2023

View reviewed changes

rickstaa closed this Jan 16, 2023

rickstaa reopened this Jan 20, 2023

rickstaa force-pushed the master branch from 8bc69e7 to d5d1f44 Compare January 21, 2023 10:32

rickstaa self-assigned this Jan 21, 2023

rickstaa force-pushed the master branch 2 times, most recently from 86aafe8 to 8bc69e7 Compare January 21, 2023 16:47

rickstaa mentioned this pull request Jan 21, 2023

feat: rate limit error chaching #2448

Merged

rickstaa closed this Jan 21, 2023

rickstaa deleted the prevent_total_fetch_zero_cache branch March 5, 2023 10:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: prevent totalCommitsFetch error result from being cached #2177

fix: prevent totalCommitsFetch error result from being cached #2177

rickstaa commented Oct 10, 2022 •

edited

Loading

vercel bot commented Oct 10, 2022 •

edited

Loading

codecov bot commented Oct 10, 2022 •

edited

Loading

rickstaa commented Jan 10, 2023

anuraghazra Jan 14, 2023

rickstaa Jan 15, 2023

rickstaa Jan 16, 2023 •

edited

Loading

Zo-Bro-23 Jan 20, 2023

rickstaa Jan 21, 2023

Zo-Bro-23 Jan 21, 2023

rickstaa Jan 21, 2023

Zo-Bro-23 Jan 22, 2023

rickstaa commented Jan 16, 2023 •

edited

Loading

rickstaa commented Jan 20, 2023

rickstaa commented Jan 21, 2023

Zo-Bro-23 commented Jan 22, 2023

	throw new CustomError(
	"Something went while trying to retrieve the stats data using the GraphQL API.",
	CustomError.GRAPHQL_ERROR,
	);

fix: prevent totalCommitsFetch error result from being cached #2177

fix: prevent totalCommitsFetch error result from being cached #2177

Conversation

rickstaa commented Oct 10, 2022 • edited Loading

vercel bot commented Oct 10, 2022 • edited Loading

codecov bot commented Oct 10, 2022 • edited Loading

Codecov Report

rickstaa commented Jan 10, 2023

anuraghazra Jan 14, 2023

Choose a reason for hiding this comment

rickstaa Jan 15, 2023

Choose a reason for hiding this comment

rickstaa Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

Zo-Bro-23 Jan 20, 2023

Choose a reason for hiding this comment

rickstaa Jan 21, 2023

Choose a reason for hiding this comment

Zo-Bro-23 Jan 21, 2023

Choose a reason for hiding this comment

rickstaa Jan 21, 2023

Choose a reason for hiding this comment

Zo-Bro-23 Jan 22, 2023

Choose a reason for hiding this comment

rickstaa commented Jan 16, 2023 • edited Loading

rickstaa commented Jan 20, 2023

rickstaa commented Jan 21, 2023

Zo-Bro-23 commented Jan 22, 2023

rickstaa commented Oct 10, 2022 •

edited

Loading

vercel bot commented Oct 10, 2022 •

edited

Loading

codecov bot commented Oct 10, 2022 •

edited

Loading

rickstaa Jan 16, 2023 •

edited

Loading

rickstaa commented Jan 16, 2023 •

edited

Loading