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: prevent totalCommitsFetch error result from being cached #2177

Closed
wants to merge 2 commits into from

Conversation

rickstaa
Copy link
Collaborator

@rickstaa rickstaa commented Oct 10, 2022

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
Copy link

vercel bot commented Oct 10, 2022

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)

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

codecov bot commented Oct 10, 2022

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.

@rickstaa
Copy link
Collaborator Author

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

@rickstaa rickstaa removed their assignment Jan 10, 2023
}`,
);
} else {
res.setHeader("Cache-Control", `no-cache, no-store, must-revalidate`); // Don't cache unsuccessful responses.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@rickstaa rickstaa Jan 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
Copy link
Collaborator Author

rickstaa commented Jan 16, 2023

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

@rickstaa rickstaa closed this Jan 16, 2023
@rickstaa
Copy link
Collaborator Author

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

@rickstaa
Copy link
Collaborator Author

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)).

@rickstaa rickstaa closed this Jan 21, 2023
@Zo-Bro-23
Copy link
Collaborator

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 rickstaa deleted the prevent_total_fetch_zero_cache branch March 5, 2023 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high-priority High priority issue or PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants