feat: rate limit error chaching

[rickstaa]

Rate limit error caching to alleviate PATs.

[vercel[bot]]

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[bot]]

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%)	:arrow_up:
api/index.js	100.00% <100.00%> (ø)
api/pin.js	97.87% <100.00%> (+0.11%)	:arrow_up:
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%)	:arrow_up:

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[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 https://github.com/anuraghazra/github-readme-stats/pull/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]

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

I currently do not have the time to implement this, but it should be doable if we find a way to throw a Vercel error. We can then catch the MAX_RETRY and possibly REST_API_ERROR (see https://github.com/anuraghazra/github-readme-stats/pull/2177) in the API endpoints and throw this Vercel error. If we add if-stale-error to the header, the cards should be served from the cache if the GraphQL or Rest APIs timeout.

[rickstaa]

Not sure if stale-if-error is already supported with Vercel (see https://github.com/vercel/vercel/issues/3362).

[Zo-Bro-23]

I'm not sure if I understand this correctly, but returning a 400 or a 500 error should trigger stale-if-error right? There's nothing special we have to do apart from throwing an API error (but I could be wrong).

[rickstaa]

I'm not sure if I understand this correctly, but returning a 400 or a 500 error should trigger stale-if-error right? There's nothing special we have to do apart from throwing an API error (but I could be wrong).

I thought the same but up till now I don't know exactly how Vercel deals with errors. You can see what I already tried in:

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

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. Could be that Vercel shows the previous cached card when a response with a 500 status is given and only shows the error card after the if-stale-error period is over but I could not find documentation on this matter and did not yet test. We could try to add a condition that breaks the retryer after a certain time. @anuraghazra what do you think?

[rickstaa]

Just putting this here for future reference. Fixing this issue will fix our most mentioned bug https://github.com/anuraghazra/github-readme-stats/issues/1515. 👍

[rickstaa]

If we like this new method, we can also improve it the fetchers/stats-fetcher.js/totalCommitsFetcher so that it throws an error instead of returning 0. This will fix #1515 and #2358.

[anuraghazra]

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.

[rickstaa]

Merging this will fix #1515. @qwerty541 I will check this later this week so we can close #1515.

[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

https://github.com/anuraghazra/github-readme-stats/blob/0a5ff9647742a4424ebc6574f45d101e0afacec0/api/index.js#L103-L109

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 👍🏻.