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(github-invite): improve performance of missing members API #56477

Merged

Conversation

cathteng
Copy link
Member

@cathteng cathteng commented Sep 19, 2023

For orgs with many commit authors that are not members of their organization, the missing members API is timing out because there are too many commit authors to serialize (SENTRY-14QD).

We should limit the number of missing members to the 50 most active missing members. I also pulled out an inner join into its own query so it's more readable.

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 19, 2023
@cathteng cathteng marked this pull request as ready for review September 19, 2023 15:56
@cathteng cathteng requested a review from a team September 19, 2023 15:56
@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

Merging #56477 (d3ba954) into master (dc18d59) will decrease coverage by 0.01%.
Report is 4 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #56477      +/-   ##
==========================================
- Coverage   78.64%   78.64%   -0.01%     
==========================================
  Files        5078     5078              
  Lines      218272   218260      -12     
  Branches    36952    36949       -3     
==========================================
- Hits       171670   171650      -20     
- Misses      41064    41070       +6     
- Partials     5538     5540       +2     
Files Changed Coverage
.../api/endpoints/organization_missing_org_members.py 100.00%

Copy link
Contributor

@nhsiehgit nhsiehgit left a comment

Choose a reason for hiding this comment

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

👍

Does separating the query out really make it faster?
Maybe you can prefetch the fk'd table?

@evanpurkhiser
Copy link
Member

Does separating the query out really make it faster?

I think the LIMIT 50 is what's making it faster here

@cathteng
Copy link
Member Author

@evanpurkhiser @nhsiehgit i edited the description so nathan was right to ask. looking into prefetching now

@evanpurkhiser
Copy link
Member

Oh, the query did actually change, the code wasn't just split out 👍

@evanpurkhiser
Copy link
Member

Oh could you share the sentry performance metrics here as well?

@cathteng
Copy link
Member Author

@evanpurkhiser i don't think there are performance metrics because the API call times out? this is the issue SENTRY-14QD

@evanpurkhiser
Copy link
Member

It doesn't always though right? I think there should be a transaction for this endpoint right?

@cathteng
Copy link
Member Author

@evanpurkhiser failure rate 19% here

@cathteng cathteng merged commit 0bf1b30 into master Sep 19, 2023
52 checks passed
@cathteng cathteng deleted the cathy/github-growth/missing-members-api-performance branch September 19, 2023 17:18
@sentry-io
Copy link

sentry-io bot commented Sep 26, 2023

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ OperationalError: QueryCanceled('canceling statement due to user request\n') /api/0/organizations/{organization_slug}/missin... View Issue

Did you find this useful? React with a 👍 or 👎

@github-actions github-actions bot locked and limited conversation to collaborators Oct 11, 2023
@cathteng cathteng changed the title fix(github-growth): improve performance of missing members API fix(github-invite): improve performance of missing members API Dec 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants