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

Perf: Fix N+1s on partners csv export #4994

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

coalest
Copy link
Collaborator

@coalest coalest commented Feb 6, 2025

Description

We are getting some timeouts in production for partners exports from an org with lots of partners. This fixes the N+1 query issues for these requests.

Type of change

  • Bug/performance

How Has This Been Tested?

Test suite and manually. Although the current partners request spec is only testing for a 2XX HTTP response, so some specs need to be added.

@coalest coalest force-pushed the fix-partners-export-n-plus-one branch from ffbb419 to 485299a Compare February 6, 2025 15:17
@coalest coalest changed the title Perf: Fix N+1s on artners csv export Perf: Fix N+1s on partners csv export Feb 6, 2025
@cielf
Copy link
Collaborator

cielf commented Feb 6, 2025

I'm afraid It's not noticeably faster on a simple manual check once on the production branch and once with this branch for the bank in question. FWIW, They are quite light on the county information, though, -- < 40 out of the 770. This took around 15 seconds both on the production branch and this one.

So I hope/suspect the culprit is the providing_diapers and providing_period_supplies. This particular partner doesn't provide diapers at all.

@cielf cielf requested a review from dorner February 6, 2025 19:13
@cielf
Copy link
Collaborator

cielf commented Feb 6, 2025

Hey @dorner -- I asked Cory to look into this because one of the banks at Wednesday's meeting is seeing timeouts whenever they try to do the partners export. It's taking about 15 seconds on my local fwiw.

@coalest
Copy link
Collaborator Author

coalest commented Feb 6, 2025

Ok i will refactor the export into a service and fix the other N+1s. It should drastically improve the performance.

@coalest
Copy link
Collaborator Author

coalest commented Feb 7, 2025

@cielf I just pushed a commit that should remove all the N+1's from the partners export.

I can't seem to find specs testing the generated data for this partners export (neither for the Partner#generate_csv method nor for the /partners.csv request), which means I will first have to add specs that pass with the current request and then make sure my refactoring hasn't changed any behavior.

With that said, would you mind testing this branch locally since you have a production-like database on your computer. I wouldn't test it for correctness yet (because I have yet to write tests), but I would be interested to know if it fixes the timeout issue, or if I need to dig deeper.

If it's still slow on this branch, could you share the output from Rack Mini Profiler? How to:

  1. Click on the export partner agencies button.
  2. Refresh the page. (for some reason the profiling result from this button doesn't automatically get added to the current badge)
  3. Click on the performance badge.
  4. Click on the /partners.csv request, and then click "share" to see an output that is shareable.

Screenshot for reference:
image

@cielf cielf self-requested a review February 7, 2025 21:17
Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

1/ Much faster. I'll want to check the results next to a version from the prod branch. @dorner can you do a tech pass?
2/ Lint is failing -- also some tests, but I haven't looked at if they are related.
3/ (Mostly an aside) Looking at the results from this period bank, I have my doubts about the accuracy of the "Providing Period Supplies", but I need to check it against the production version to see if it's a problem introduced here. (I don't think it is.)

@coalest coalest marked this pull request as draft February 7, 2025 21:37
@cielf
Copy link
Collaborator

cielf commented Feb 7, 2025

@coalest Hrm. They are not producing identical results -- one of them has quoted strings for all the areas that are blank, and the other doesn't. That makes it harder to tell if there are any substantive differences.

@coalest
Copy link
Collaborator Author

coalest commented Feb 7, 2025

Yea this is just a rough draft so far, wasn't expecting all the tests to pass because I moved some methods around but haven't fixed the tests yet. Mostly wanted to see if the issue was due to the N+1's or from somewhere else. I will try to polish this up soon.

@cielf
Copy link
Collaborator

cielf commented Feb 8, 2025

Followup on me wondering if the "Providing Period Supplies" is right -- this bank actually has a lot of partners that they haven't distributed to in the last year. I checked 10 that showed "N", and they matched. So let's not worry about that.

@coalest
Copy link
Collaborator Author

coalest commented Feb 8, 2025

Moved the previous model export specs to the new export service. So all tests are passing.

Still need to write request specs to have more confidence that the behavior before and after this refactor isn't changing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants