Closed
Bug 956987
Opened 11 years ago
Closed 11 years ago
Rocketfuel API /api/v1/rocketfuel/collections is too slow
Categories
(Marketplace Graveyard :: API, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: cvan, Assigned: mat)
References
()
Details
(Whiteboard: [qa-])
The fastest one was the response for region=us @ 2.28s:
https://marketplace.firefox.com/api/v1/rocketfuel/collections/?_user=<CENSORED>&carrier=&cat=®ion=us
region=worldwide @ 2.96s:
http://cl.ly/image/2r2G1d0J0A0U
region=br @ 4.50s:
http://cl.ly/image/3n1D2B2H230j
as fast on prod @ 2.75s:
https://marketplace.firefox.com/api/v1/rocketfuel/collections/?carrier=&cat=®ion=br
as long on -dev @ 11.93s:
https://marketplace-dev.allizom.org/api/v1/rocketfuel/collections/?carrier=&cat=®ion=br
the rest:
http://cl.ly/image/020Z3b2D1j2N
Can we do any faster? It seems like the more apps there are in a collection, the slower the response takes. These numbers are from the Mozilla Wifi. From my apartment Internet, these endpoints were taking 30-33s. We can do front-end caching for these URLs if we really need to, but it seems like these times are indicative of a bigger problem.
Comment 1•11 years ago
|
||
Yeah, it's known to be a bugger, and it's just going to be getting worse with the feed, as it adds another layer of relational goodness. There was some discussion of this last week in one of our IRC channels.
Bug 956794 and its children are going to be really important to get right.
Blocks: 926640
Assignee | ||
Comment 2•11 years ago
|
||
Yeah I haven't been focusing on this because the frontend uses /search/featured/ which does the job through ES, but rocketfuel endpoints is slow.
Part of the problem is, it needs the db to have fresh data and it does a lot of queries per app. I have a few ideas I'm going to try:
- Find out where we can use .only() to avoid loading useless stuff in the app serializer (.values()/.values_list() would be nice but AFAIK django-cache-machine doesn't cache those)
- Find out if we return extra stuff that's not needed. At the very least, we should probably be using FireplaceAppSerializer, I don't think we do
- Find out if there are queries that are made more than once for some reason (it does seem to be the case at first glance, might be because of double instantiation of querysets)
Obviously the feed is going to invalidate some of this work but I think most of the optimizations I'm going to find are not going to be collection-specific, there is room in the app stuff alone.
Assignee: nobody → mpillard
Priority: -- → P2
Assignee | ||
Comment 3•11 years ago
|
||
https://github.com/mozilla/zamboni/pull/1593 and https://github.com/mozilla/zamboni/pull/1624 should help, a lot.
Assignee | ||
Updated•11 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•11 years ago
|
||
Merged both PRs:
https://github.com/mozilla/zamboni/commit/eb95aa7f67bbd407fda16ee6da8b1c67a1431a08
https://github.com/mozilla/zamboni/commit/d7732a7b0bd9933ded967b3c8145e56f3d1fd30d
There are still a few things we can do to improve performance, I believe there are still a couple extra queries we could avoid, but let's see what this does first.
Assignee | ||
Comment 5•11 years ago
|
||
You can see the first part of the performance improvement in the graph:
http://graphite.nag.mktmon.services.phx1.mozilla.com/render/?width=580&height=308&vtitle=count&target=stats.timers.marketplace.api.mkt.collections.views.CollectionViewSet.GET.upper_90&target=stats.timers.marketplace.api.mkt.collections.views.CollectionViewSet.POST.upper_90&target=stats.timers.marketplace.api.mkt.collections.views.CollectionViewSet.PATCH.upper_90&target=stats.timers.marketplace.api.mkt.collections.views.CollectionViewSet.PUT.upper_90&target=stats.timers.marketplace.api.mkt.collections.views.CollectionViewSet.DELETE.upper_90&target=drawAsInfinite%28stats.timers.marketplace.update.count%29&from=-7days&title=7%20days&target=threshold%28500,%20%22poor%22,%20orange%29&target=threshold%281000,%20%22bad%22,%20red%29&target=drawAsInfinite%28color%28stats.timers.addons.update.count,%20%22magenta%22%29%29
It'll be interesting to look at the 15 days graph once the second part lands.
Assignee | ||
Comment 6•11 years ago
|
||
Note: we had to add back 'regions' to the simple representation of apps because that caused bug 964802. It would have been nice to have a different representation for apps in collections for fireplace and for curation tool. We could cheat and do that in the CollectionESAppSerializer but that's not ideal.
Assignee | ||
Comment 7•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•