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

Admin Customers performance #5196

Merged
merged 6 commits into from
Apr 17, 2020

Conversation

Matt-Yorkley
Copy link
Contributor

What? Why?

Improves performance in admin customers page.

What should we test?

Admin customers page is working correctly, should be much faster for large hubs.

Release notes

Improved performance on admin customers page

Changelog Category: Fixed

@Matt-Yorkley Matt-Yorkley self-assigned this Apr 9, 2020
@Matt-Yorkley Matt-Yorkley force-pushed the customers-performance branch 2 times, most recently from 691e57e to 09a51ca Compare April 9, 2020 18:56
@Matt-Yorkley
Copy link
Contributor Author

Tested locally with a medium-sized hub (using production data). In production this endpoint regularly takes 40+ seconds to load.

BEFORE: 14.6 seconds and ~3200 queries

Screenshot from 2020-04-09 16-29-43

AFTER: 3.3 seconds and 26 queries

Screenshot from 2020-04-09 17-48-47

@Matt-Yorkley Matt-Yorkley marked this pull request as ready for review April 9, 2020 19:31
@@ -17,8 +17,9 @@ def index
respond_to do |format|
format.html
format.json do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be really nice to extract this to the API, but I think it would require a lot more changes to be piled on top of this PR...

Fixes N+1 queries on customer tags
Copy link
Contributor

@luisramos0 luisramos0 left a comment

Choose a reason for hiding this comment

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

awesome!
I think we should try to look for other places where tags are killing us and improve 👌

app/controllers/admin/customers_controller.rb Show resolved Hide resolved
app/controllers/admin/customers_controller.rb Outdated Show resolved Hide resolved
app/controllers/admin/customers_controller.rb Show resolved Hide resolved
@Matt-Yorkley
Copy link
Contributor Author

Matt-Yorkley commented Apr 10, 2020

I just double-checked some of the SQL before and after and I realised I forgot something crucial here! It's a polymorphic association, so we have to specify the class the tags are attached to:

Screenshot from 2020-04-10 20-18-46

New commit added. Hopefully we can extract this into a reusable service as some point and utilise it elsewhere.

@Matt-Yorkley
Copy link
Contributor Author

Matt-Yorkley commented Apr 12, 2020

Hopefully we can extract this into a reusable service as some point and utilise it elsewhere.

I had a quick look and it looks like we can use this to improve reports 💪

@luisramos0
Copy link
Contributor

awesome, we should create an issue for that like: extract customer_tags_by_id and re-use it 🎉

if credit_cards.loaded?
credit_cards.to_a.find(&:is_default)
else
credit_cards.where(is_default: true).first
Copy link
Contributor

Choose a reason for hiding this comment

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

have you tried defining this as an association and using AR association's :condition? This feels to me like something AR should do and not us.

We can improve it in a different PR if you prefer. I don't want to delay this PR.

Copy link
Contributor

@sauloperez sauloperez left a comment

Choose a reason for hiding this comment

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

👏

@filipefurtad0 filipefurtad0 added the pr-staged-au staging.openfoodnetwork.org.au label Apr 16, 2020
@filipefurtad0
Copy link
Contributor

Hey @Matt-Yorkley ,

Uff. This one was a brainer to set up - and, well, made me very grateful this and all performance efforts :-)

In AU and as superadmin:

  • I had a look in reports for an enterprise with plenty of customers, found Ballantyne (Thornbury).
  • Turned over to which has over a hundred. navigated to Users > Permissions > and added a user to manage this enterprise
  • logged into this users account
  • navigated to /admin/customers/
  • Reloaded the page and had a few screenshots, before and after the PR

Somehow, I could not "measure" a tangible difference... Load times for the /admin/customers/ page:

Before:
image

After this PR:
image

Perhaps the customer number isn't large enough?
Tried performing the usual operations here, as well, as in the users page. I found nothing unusual.

I'm moving this to ready to go, but adding the feedback label, just in case you feel there is anything more I should try here.

@filipefurtad0 filipefurtad0 added feedback-needed and removed pr-staged-au staging.openfoodnetwork.org.au labels Apr 16, 2020
@Matt-Yorkley
Copy link
Contributor Author

Thanks Filipe! Interesting results. I just had another look, and the optimisations here will apply more to larger hubs that have any/all of the following:

  • lots of customers
  • lots of card payments
  • lots of completed orders
  • using tagging on customers

I think Prom Coast Food Hub is a good one to go for if you're testing large hubs on Aus Staging.

@filipefurtad0
Copy link
Contributor

Ah - that one has over 600 customers. I guess I should just have asked :-)
Great, I'll have a second look at this.
Thanks @Matt-Yorkley

@filipefurtad0
Copy link
Contributor

Ok! Now things look quite different.

Time to refresh page:
Before PR: ~10 sec
After PR: ~5 sec

Time to display data, after clicking the [ Show all (589 more) ] button:
Before PR ~30 sec
After PR ~20 sec

Pic before PR:
BEFORE_PR

Pic after PR:
AFTER_PR

Thanks for your help @Matt-Yorkley
Good to go.

@mkllnk mkllnk merged commit 2e31f23 into openfoodfoundation:master Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants