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

Federation: return useful statistic information for nodeinfo #19561

Merged
merged 10 commits into from
May 2, 2022

Conversation

6543
Copy link
Member

@6543 6543 commented Apr 29, 2022

TODO:

  • UserStatistics
  • document new settings
  • cache
  • ...

@6543 6543 added pr/wip This PR is not ready for review topic/federation labels Apr 29, 2022
@6543 6543 added this to the 1.17.0 milestone Apr 29, 2022
@techknowlogick
Copy link
Member

@zeripath recently added caching to some of these stats, are you able to leverage the cache as well?

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 29, 2022
@6543
Copy link
Member Author

6543 commented Apr 29, 2022

@techknowlogick added cache

@6543 6543 removed the pr/wip This PR is not ready for review label Apr 29, 2022
@codecov-commenter

This comment was marked as outdated.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 30, 2022

The question again, why does Gitea need to provide the accurate count number instead of the estimated count number?

I do not think it's worth introducing new setting options (#19474) to provide the accurate count number which is using slow full table (index) scan. In the other PR: there are options about estimated or not / cache TTL, already doubled to 4 new options, and in future maybe more 6, 8 options if more APIs are introduced ....

Could the problem be simplified to: only provide estimated count number for all users/actions/issues, etc?

From #19474:

I would prefer to avoid introducing new options, and just tell users that these metrics are estimated. The reason is that the accurate count number of action/star/user/etc do not help admin really. If you read a count number 1000 at first time, next minute it may become 1100, there is no really accurate and meaningful count number indeed. So in the end there is no difference between providing an estimated number or an accurate number.

The UI can just tell admin that these numbers are estimated, it should be enough.

@lunny
Copy link
Member

lunny commented Apr 30, 2022

What's the aim to expose these statistic information?

@6543
Copy link
Member Author

6543 commented Apr 30, 2022

so services likehttps://the-federation.info/ (https://github.com/thefederationinfo/the-federation.info/issues/149) can discover nodes and make suggestions where to join the "federated gitea network"

@6543
Copy link
Member Author

6543 commented Apr 30, 2022

@wxiaoguang @zeripath to the cache and aproximate counts:

you can not count users aproximate, as orgs are also in the same table.
also a single count query max every 2min should not be a problem at all (see caching)

beside the caching in #19474, I would like to implement it as router middleware later on (not part of this pull atm), so we can transparent say m.Get("...", context.Cache(2min), ....) - so we can speed ups some more apis ;)

@6543
Copy link
Member Author

6543 commented Apr 30, 2022

I do not think it's worth introducing new setting options

I still think we should add this, all federated software I know of has this flag/setting (synapse, diaspora, ...)

@6543
Copy link
Member Author

6543 commented Apr 30, 2022

The UI can just tell admin that these numbers are estimated, it should be enough.

nodeinfo is not for the admin but for the federation ... as per label

@wxiaoguang
Copy link
Contributor

I do not think it's worth introducing new setting options

I still think we should add this, all federated software I know of has this flag/setting (synapse, diaspora, ...)

The new setting options I meant is estimated or not, cache TTL, and doubled to 4 options in another PR.

For this one, ShareUserStatistics does what it should do and seems right.

@6543
Copy link
Member Author

6543 commented Apr 30, 2022

ok, is something else blocking it?

@lunny
Copy link
Member

lunny commented Apr 30, 2022

I do not think it's worth introducing new setting options

I still think we should add this, all federated software I know of has this flag/setting (synapse, diaspora, ...)

Maybe we need a doc about federation which information will be exposed.

@6543
Copy link
Member Author

6543 commented May 1, 2022

@lunny I think we need a new document section for federation, since this is only one part we should explain & document

should I add it in this pull?

models/user/user.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels May 1, 2022
@6543 6543 requested a review from techknowlogick May 1, 2022 16:12
@6543 6543 requested a review from Gusted May 2, 2022 12:38
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 2, 2022
@Gusted
Copy link
Contributor

Gusted commented May 2, 2022

Federation: return usefull statistic information for nodeinfo

-> Federation: return useful statistic information for nodeinfo

@wxiaoguang wxiaoguang changed the title Federation: return usefull statistic information for nodeinfo Federation: return useful statistic information for nodeinfo May 2, 2022
@wxiaoguang wxiaoguang merged commit e2a3f3d into go-gitea:main May 2, 2022
@6543 6543 deleted the populate-nodeinfo branch May 2, 2022 14:41
zjjhot added a commit to zjjhot/gitea that referenced this pull request May 3, 2022
* giteaofficial/main:
  Fix some slice problems (incorrect slice length) (go-gitea#19592)
  Fix sending empty notifications (go-gitea#19589)
  Handle the error of a missing blob object fix go-gitea#19530 (go-gitea#19552)
  Remove legacy `+build:` constraint (go-gitea#19582)
  Federation: return useful statistic information for nodeinfo (go-gitea#19561)
  Upgrade required git version to 2.0 (go-gitea#19577)
  add smtp password to install page (go-gitea#17564)
  ignore DNS error when doing migration allow/block check (go-gitea#19566)
  [skip ci] Updated translations via Crowdin
  Dont overwrite err with nil & rename PullCheckingFuncs to reflect there usage (go-gitea#19572)
  Improve UI on mobile (go-gitea#19546)
  Add API to check if team has repo access (go-gitea#19540)
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
…a#19561)

Add statistic information for total user count, active user count, issue count and comment count for `/nodeinfo`
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/federation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants