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

Include_all_commits sometimes shows incorrect number of commits #1515

Closed
nxwi opened this issue Jan 2, 2022 · 11 comments · May be fixed by #1691
Closed

Include_all_commits sometimes shows incorrect number of commits #1515

nxwi opened this issue Jan 2, 2022 · 11 comments · May be fixed by #1691
Labels
bug Something isn't working. high-priority High priority issue or PR. ⭐ top bug Top bug. ⭐ top issue Top issue. stats-card Feature, Enhancement, Fixes related to stats the stats card. upstream Problems caused by upstream issues.

Comments

@nxwi
Copy link

nxwi commented Jan 2, 2022

Describe the bug

On GitHub Stats the number of commits is currently showing the last years commits, after adding include_all_commits it decreases as shown below.

Expected behaviour

Shows the number of commits in 2022

Screenshots / Live demo link (paste the github-readme-stats link as markdown image)

Before adding include_all_commits
image
After adding include_all_commits
image

@rickstaa rickstaa added bug Something isn't working. stats-card Feature, Enhancement, Fixes related to stats the stats card. popular labels Mar 9, 2022
@rickstaa rickstaa removed the popular label Sep 22, 2022
@rickstaa rickstaa changed the title Commits number is wrong Include_all_commits sometimes shows incorrect number of commits Sep 24, 2022
@rickstaa rickstaa changed the title Include_all_commits sometimes shows incorrect number of commits Include_all_commits sometimes shows incorrect number of commits Sep 24, 2022
@rickstaa
Copy link
Collaborator

@nasw1j, I checked the codebase, and I think both count_private and include_all_commits parameters are correctly implemented.

if (include_all_commits) {
stats.totalCommits = await totalCommitsFetcher(username);
}
// if count_private then add private commits to totalCommits so far.
if (count_private) {
stats.totalCommits +=
user.contributionsCollection.restrictedContributionsCount;
}

I, therefore, think the error is caused by incorrect data returned from GitHub's REST API and GraphQL API APIs. If this happens, these wrong values are cached for 4 hours.

const cacheSeconds = clampValue(
parseInt(cache_seconds || CONSTANTS.FOUR_HOURS, 10),
CONSTANTS.FOUR_HOURS,
CONSTANTS.ONE_DAY,
);

When this happens, you can check the console.log to see if there is any error and use the &v=123 syntax to force the cache to reload.

Detailed explanation

If include_all_commits is enabled, and GitHub REST API fails the code will return 0.

const totalCommitsFetcher = async (username) => {
if (!githubUsernameRegex.test(username)) {
logger.log("Invalid username");
return 0;
}
// https://developer.github.com/v3/search/#search-commits
const fetchTotalCommits = (variables, token) => {
return axios({
method: "get",
url: `https://api.github.com/search/commits?q=author:${variables.login}`,
headers: {
"Content-Type": "application/json",
Accept: "application/vnd.github.cloak-preview",
Authorization: `token ${token}`,
},
});
};
try {
let res = await retryer(fetchTotalCommits, { login: username });
let total_count = res.data.total_count;
if (!!total_count && !isNaN(total_count)) {
return res.data.total_count;
}
} catch (err) {
logger.log(err);
}
// just return 0 if there is something wrong so that
// we don't break the whole app
return 0;
};

This will then be used as the total commits. To this value, the private commits will be correctly added if count_private is enabled. Since we used caching, this wrong value will be on the card for 4 hours.

Posible solutions

We can use the previous cache when this problem happens.

@rickstaa
Copy link
Collaborator

rickstaa commented Sep 24, 2022

I quickly checked #1691 created by @Rongronggg9 and think merging to his pull request will significantly improve the commit accuracy. I, however, did not have time to do a thorough review 😅.

Nevertheless, still, one problem persists with #1691 merged. It will show a commit count of 0 when the GitHub Rest API fails. @Rongronggg9 maybe, if the Rest API fails, we can use the old cache or prevent the wrong result from being cached?

res.setHeader("Cache-Control", `public, max-age=${cacheSeconds}`);

Maybe this can be achieved by changing the cache header when the Rest API fails (see https://vercel.com/docs/concepts/edge-network/caching).

@Rongronggg9
Copy link
Contributor

I see. I will find some time to take a look at it.

@rickstaa
Copy link
Collaborator

@Rongronggg9 No pressure your PR looks very promising, take your time! 🚀

@rickstaa
Copy link
Collaborator

@Rongronggg9 See #2177 and 4df8094 for a example on how to purge the cache when the GraphQL and Rest api fail.

Further, please read #211 because we are currently not fetching commits by year. In short, this is because we are trying to prevent the GraphQL and Vercel timeout limits from reaching. I think we could add this more accurate commit fetching, but it should be disabled by default to prevent any rate limits from being hit on the Public Vercel instance. I"m doing something similar on #2159.

@rickstaa
Copy link
Collaborator

I created community/community#35675 to ask GitHub to make the totalCommitContributions and restrictedContributionsCount objects available under the user object. This feature would allow us to solve #1515 for the Public Vercel instance. Please show your support at community/community#35675.

@rickstaa
Copy link
Collaborator

rickstaa commented Feb 7, 2023

For future reference, a (draft) PR to improve this behaviour can be found at #2448.

@rickstaa
Copy link
Collaborator

As pointed out by @Madhav-MKNC in #2851 (comment), for some accounts the total commit count is lower than the yearly commit count. This is also caused by the use of the GitHub Search API, which is known to be buggy.

url: `https://api.github.com/search/commits?q=author:${variables.login}`,

If you want this fixed please add your support to https://github.com/orgs/community/discussions/35675 so we can fix this in the future 🙏🏻. Furthermore, this behaviour will be improved when the GitHub action is released with #1691 merged under an environmental variable.

@rickstaa
Copy link
Collaborator

@rickstaa
Copy link
Collaborator

Should be fixed now that #2448 and #3238 are merged.

@arashkashi
Copy link

I am facing the issue still my commit count are significantly lower than actual ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working. high-priority High priority issue or PR. ⭐ top bug Top bug. ⭐ top issue Top issue. stats-card Feature, Enhancement, Fixes related to stats the stats card. upstream Problems caused by upstream issues.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants