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

Discrepancy: WatchersCount vs Activity.Watching.GetAllWatchers #2208

Closed
SeanKilleen opened this issue Jun 11, 2020 · 8 comments
Closed

Discrepancy: WatchersCount vs Activity.Watching.GetAllWatchers #2208

SeanKilleen opened this issue Jun 11, 2020 · 8 comments
Labels
Status: Needs info Full requirements are not yet known, so implementation should not be started Status: Stale Used by stalebot to clean house Type: Support Any questions, information, or general needs around the SDK or GitHub APIs

Comments

@SeanKilleen
Copy link
Contributor

SeanKilleen commented Jun 11, 2020

Follow-up to #2182.

Hi @shiftkey @KarolGrzesiak,

Thanks for putting in a PR to return the watchers count on the repository object. I was just on a stream the other day talking about how it could save me a bunch of API calls, and then there it is. :)

I noticed a strange discrepancy and I want to make sure I'm understanding correctly.

Should Repository.WatcherCount be expected to contain the same results as _githubClient.Activity.Watching.GetAllWatchers(repoId); ?

I've noticed that Repository.WatcherCount and utilize the Activity API appear to produce different results. And in the cases where they differ, the Activity API appears to be returning the count that I see in the UI for watchers.

Do you have any thoughts on why this might be? Do they have two different intended purposes and are named similarly?

SeanKilleen added a commit to excellalabs/konmaripo that referenced this issue Jun 11, 2020
SeanKilleen added a commit to excellalabs/konmaripo that referenced this issue Jun 11, 2020
@shiftkey
Copy link
Member

@SeanKilleen is there a public repository you can share that illustrates the problem? (or was showing the issue?)

The first thing that comes to mind here is caching, with three potential ways to get this number:

  • watchers_count on the repository returned via the API could be cached for a period of time because of all the different places in the GitHub API where repository is used
  • enumerating the List Watchers API would require getting the latest details about each watcher, and thus is more likely to be correct (but slower)
  • but if the website is showing a different watchers count it could be computing this in a different way to the API

@SeanKilleen
Copy link
Contributor Author

SeanKilleen commented Jun 11, 2020

@shiftkey excellalabs/konmaripo#63 would work (if you provide a key/orgName in settings, and disable the auth). I'll try to get a simpler repro. The current state of that PR has both data points showing side by side in the details page for a given repository. I'll try to do a PR off of my PR so that you can just provide a key & orgName and not muck around with other stuff.

I think I've ruled out caching as a consideration on my end because this is happening on startup of the app on first view (we retrieve the list of repositories and there's another "detail" page where I provide the results of getting watchers via the activity API). The summary page is returning Watcher.

I'd expect GitHub caching wouldn't be relevant here. This is a tool to archive old repos, and some of these watchers haven't changed for years so I imagine would be showing up.

Although...maybe it's because the repos in question are old that the WatcherCount property is smaller than I'd expect?

@SeanKilleen
Copy link
Contributor Author

SeanKilleen commented Jun 11, 2020

@shiftkey repro branch here: https://github.com/excellalabs/konmaripo/tree/repro-for-octokit-issue-2208

Requires you to input AccessToken and OrganizationName in appSettings or user secrets.

In my case, the steps are:

  • Click "Sunsetting" in the menu
  • A list of repos will load
  • Choose a repository to go to its details page
  • Note the item under the eye icon (from the Repository object) and the Watchers (from the activity API) are different.

image

I can't understand if there's anything in particular about a given repo that causes this to be the returned number.

One thought -- is there a difference between people who are subscribed vs auto-subscribed because they watch repositories by default in an org? If the default folks wouldn't count toward WatcherCount, we may be on to something.

@shiftkey
Copy link
Member

What's wild about your repo is that the field we're supposed to be deprecating is correct when looking at it directly from the API https://api.github.com/repos/excellalabs/konmaripo:

  "watchers_count": 0,
   ...
  "subscribers_count": 3

I was under the impression that we were standardizing on one value here, but the example repo in our docs makes me wonder if they're actually different underneath, and this is just some confusion between the API and the website itself.

Also, the List Watchers API is actually described as GET /repos/:owner/:repo/subscribers and in your situation https://api.github.com/repos/excellalabs/konmaripo/subscribers returns the 3 entities.

So maybe watchers_count is actually the value that we're supposed to obsolete? I need to investigate some more because it's all rather fuzzy right now...

@SeanKilleen
Copy link
Contributor Author

So maybe watchers_count is actually the value that we're supposed to obsolete?

I think you're on to something there. Mostly I'm relieved that it's at least stumped you temporarily too. 😆

@SeanKilleen
Copy link
Contributor Author

SeanKilleen commented Jun 11, 2020

What's wild about your repo is that the field we're supposed to be deprecating is correct when looking at it directly from the API https://api.github.com/repos/excellalabs/konmaripo

Ah, to be clear, this repo isn't the one with the incorrect count. It's the one with the project that is utilizing Octokit. May be worth confirming the repro steps against the branch I linked earlier.

(This is not urgent by the way. I appreciate you jumping on it so quickly but if you need to return to it later I understand.)

@SeanKilleen
Copy link
Contributor Author

One other theory I'm going to follow up on but dropping here:

This org is a private org and some people have since moved on. I wonder if it's possible that the watchers list contains people who no longer have access, and so the effective Watcher count is therefore smaller.

@nickfloyd nickfloyd added Type: Support Any questions, information, or general needs around the SDK or GitHub APIs Status: Needs info Full requirements are not yet known, so implementation should not be started and removed followup-needed Type: Support Any questions, information, or general needs around the SDK or GitHub APIs labels Oct 27, 2022
@github-actions
Copy link

👋 Hey Friends, this issue has been automatically marked as stale because it has no recent activity. It will be closed if no further activity occurs. Please add the Status: Pinned label if you feel that this issue needs to remain open/active. Thank you for your contributions and help in keeping things tidy!

@github-actions github-actions bot added the Status: Stale Used by stalebot to clean house label Jul 26, 2023
@github-actions github-actions bot closed this as completed Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Needs info Full requirements are not yet known, so implementation should not be started Status: Stale Used by stalebot to clean house Type: Support Any questions, information, or general needs around the SDK or GitHub APIs
Projects
None yet
Development

No branches or pull requests

3 participants