feat: rate limit error chaching

[rickstaa]

Rate limit error caching to alleviate PATs.

[vercel]

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

Name	Status	Preview	Comments	Updated (UTC)
github-readme-stats	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 17, 2023 1:21pm

[codecov]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (64f56e8) 98.36% compared to head (ccb6b92) 98.37%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2448   +/-   ##
=======================================
  Coverage   98.36%   98.37%           
=======================================
  Files          27       27           
  Lines        5885     5915   +30     
  Branches      526      526           
=======================================
+ Hits         5789     5819   +30     
  Misses         93       93           
  Partials        3        3           

Files Changed	Coverage Δ
api/gist.js	97.72% <100.00%> (+0.13%)	⬆️
api/index.js	100.00% <100.00%> (ø)
api/pin.js	97.87% <100.00%> (+0.11%)	⬆️
api/top-langs.js	100.00% <100.00%> (ø)
src/common/retryer.js	94.73% <100.00%> (ø)
src/common/utils.js	98.95% <100.00%> (+0.01%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[anuraghazra]

Whats happening here?

CONSTANTS.ERROR_CACHE_SECONDS / 2 in max-age
CONSTANTS.ERROR_CACHE_SECONDS in s-maxage
CONSTANTS.ONE_DAY in revalidate?

[anuraghazra]

wait this code is on master lol. Have no idea why is it like this 😆

[anuraghazra]

Okay this was the change #2124

[rickstaa]

Yea, this was when I would sometimes merge PRs without your review since they had been open for a long time. Sorry for the confusion 😅. Feel free to change it to any caching behavior you like. The reasoning for the current caching code can be found at #2124 (comment).

[rickstaa]

@anuraghazra, we can also remove these lines and keep the same behavior as for the case when no error is thrown. Both are fine with me.

[anuraghazra]

Let's keep the same behaviour as old one. Not really confident with how this header will behave on prod.

[Zo-Bro-23]

Seems all good!

See my other comments though; particular this.

I'm approving the changes the related to the error caching, but I'm not really sure what some of the other changes do. If you're happy then feel free to merge 🚀

[Zo-Bro-23]

I'm good for merge 🚀

@anuraghazra thoughts?

[rickstaa]

As suggested by @anuraghazra on discord, maybe let's change this to using if-stale-error https://developer.fastly.com/learning/concepts/stale/#:~:text=stale%2Dif%2Derror%20tells%20Fastly,during%20periods%20of%20server%20instability (see image below).

We can then throw a Vercel error when the PATs are depleted instead of displaying the max tries error on the card. The browser will then try to serve a stale response instead until the if-stale-error period has ended. I think this will lead to a lot less perceived downtime. This can also replace #2177 by making sure that an error is thrown when the REST API fails.

@anuraghazra, @Zo-Bro-23 what do you think?

[anuraghazra]

Yes, but let's test it on dummy deployment before going to prod with this

[rickstaa]

Yes, but let's test it on dummy deployment before going to prod with this

100% 👍🏻.

[rickstaa]

I can currently respond with a custom error card and attach a 500 status code or show Vercels own 500 error. I was not able to change Vercels error messages.

Just to confirm we will always be rendering our own custom error SVG card right? not the vercel's error page. If we render vercel's error page that might cause issues since we will not be serving content-type=image/svg at that point.

@anuraghazra Yeah, in the end, I replaced the following code

https://github.com/anuraghazra/github-readme-stats/blob/c504c9338ed830ad619e008ba9ff76422398687f/api/index.js#L99C24-L109

with

github-readme-stats/api/index.js

Lines 103 to 109 in 0a5ff96

	res.setHeader(
	"Cache-Control",
	`max-age=${CONSTANTS.ERROR_CACHE_SECONDS / 2}, s-maxage=${
	CONSTANTS.ERROR_CACHE_SECONDS
	}, stale-while-revalidate=${CONSTANTS.ONE_DAY}`,
	); // Cache the error response less frequently.
	return res.send(renderError(err.message, err.secondaryMessage));

The downside of this is that we can not use stale-if-error, but the card error on the card will be more descriptive.

[rickstaa]

Hey @qwerty541 and @anuraghazra,

In our case, it seems that there's no significant advantage to having user requests occur at a frequency twice that of the server, especially since we don't regularly clear the cache or dynamically alter server data. Based on insights from this source, it appears that we might be able to simplify our caching headers like this:

Cache-control: max-age=CONSTANTS.ERROR_CACHE_SECONDS, stale-while-revalidate=${CONSTANTS.ONE_DAY}

However, it's important to note that the information presented in that StackOverflow answer might be outdated, as it conflicts with more recent specifications. Unfortunately, I couldn't find any documentation supporting the behaviour described in the new specifications.

Various sources on the internet suggest using both the max-age and s-maxage directives. I'm not an expert on caching headers, so I'm unsure whether specifying both max-age and s-maxage is necessary in our case. Your insights on this would be greatly appreciated!

[rickstaa]

@anuraghazra, @qwerty541, this pull request can be merged if we decide which caching period we want to use for errors and whether we can simplify the caching headers 👍🏻.