-
Notifications
You must be signed in to change notification settings - Fork 2
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
Page responses times #128
Comments
Yeah, I've noticed both, but the project dashboard is the one I notice the most. This page used to be really slow in production as well, I'm not sure what we would have done there. I see the same timings on my side, and those timings are backend response times (though there is some additional DOM render time too). I haven't looked into this either though. It would help to have the debug toolbar on the beta instance in our private beta probably, to help debug this a bit quicker. But at least we have NR, so that's pretty easy too. Just a guess, but maybe pagination is be affecting this (though build listing is already paginated in prod and isn't slow)? This would be a great place to jump in if you'd like. There is some additional DOM load time, but it seems the backend is the main culprit at the moment. It would be great to get this tuned more either way. If the issue is expensive Python code, I lean towards loading this data from the API async -- I've used this pattern to avoid resolving URLs as the view iterates over Version instances, etc. |
Yeah. This is 99% backend for sure because those times come from NR. However, we may be trying to "show too much in the frontend" in just one request. We could either, consider removing some elements or using async as you suggested 👍 |
Yeah, looks like select on builds, which is almost certainly the project listing showing data about the most recent build or build commit -- this could be dropped or moved async. A sample transaction: |
Also, I opened #31 a while ago as I don't actually think the listing page has the best information on it already. The non-beta dashboard shows the latest build status, which is ... weird. So, I'm avoiding that logic. |
Yeah, as suspected, the project listing uses a few model methods that aren't optimized. I'll want to revisit this later, to optimize and cache some new model fields. I'll reuse |
- Use a table list instead of a .ui.list element. There are some alignment issues with .ui.list and a lot of content. This mimicks the change on the build detail page too, which is now a table as well - Drop a lot of content that was slowing down the page -- project/build metadata mostly - Adds a card column to the project listing page Refs #31 Refs #128 Refs #87
Odd, I've removed basically everything from the template except the This is the same thing as the current templates though. However, I'm also now seeing the same slow load time at The build listing page performance is no better either. I'm still happy with the clean up of stubbed in metadata/fields though, and quite like the layout now. I'm going to move on from this, this would be good to focus on during the public beta, or before if we get ambitious. |
Think I might have found a clue here. I set up pyinstrument on the beta dashboard in production and did some profiling. I was trying to resolve #209 but having trouble even getting the page to load. But I gave a try to the project listing and saw this: So that already seems like a lot of time for I commented out the conditional block calling So, this can be resolved through some different template logic, hopefully. |
Ayup. Same on build listing page too: I'm going to bump this one up in priority, it should hopefully be a quick fix. Unfortunately, readthedocs/readthedocs.org#7277 doesn't seem usable in this case, where I am calling this in a listing view, once per @humitos are you familiar with the correct query to use here? I could expose a |
I posted my proposed solution in #49 (comment). I think the correct pattern is keep using |
Maybe I'm not understanding what you're suggesting correctly, but wouldn't a 5s cache invalidate before it would be useful? The problem on at least the project listing page is that the The build listing page can likely just use the context variable approach, I'm guessing that view is duplicating the same check against a single version. |
Here, I'm talking about the
The cache won't solve the problem of iterating over a list of projects. It solves the general problem of using Iterating over multiple projects on the same template may require a specific solution, not a general one. I don't think caching the function/query for more time is a good solution --mainly because we are talking about permissions. We should improve the SQL query to solve this case by adding an index or similar approaches to make those queries faster. |
Okay, maybe the fix needs to go in a different direction entirely then. I understand the conflict between Alternative solutions for this issue could include doing a permission check from the front end code right before the user clicks the button for settings. This is the primary place these checks were added. I would only need some attribute in the API responses to indicate that the user has the correct permissions. This is not ideal though. The buttons would need some default state, and really just shouldn't show for users that don't have permissions. |
Actually, it doesn't seem like there are any attributes in the API response that I can use to check for authorization. I'm looking at both API v2 and v3 responses for a project. @humitos is there another endpoint or something else we can use to inspect project authorization for a user in either API currently? |
Build list performance is addressed with #244, but the project list performance will require something else, given we run checks against up to 15 projects per paginated page view. Options I see remaining here:
|
What data exactly do you need here? The projects list endpoint (https://docs.readthedocs.io/en/stable/api/v3.html#projects-list) returns all the project the user has admin access to: https://github.com/readthedocs/readthedocs.org/blob/621fa5d5f2ad07934c0c3c8e7eb234a688158aa8/readthedocs/api/v3/views.py#L154-L159 I understand that a logic like "if the project is in the response list, then, show the link to settings page" should work. |
For community, the answer is even simpler. The dashboard project list for an authenticated user doesn't even need the But on commercial, I don't think we can rely on the project list. Users can have different permission levels and we also show read only projects in that list. I think always having the I was looking for that description of permissions in the project API response -- what level of permission does the requesting user have on the project in the API response. If we don't have this in the response already, the approach of conditionally disabling the UI is a bit hacky already and I don't yet feel great about it. I may have to think more and come back to this. |
Also worth mentioning that the API addition of the missing permission information would be usable in other API driven UI, like project/version/build/organization popup cards. Right now, these are a mix of template and API driven UI as the API doesn't have parity or the features are implemented as model methods only accessible to templates and application code. |
Thinking about this more, I think the correct approach is to add a project API response field that brings in the |
Add `?expand=permissions` to project list/detail API endpoints. Related readthedocs/ext-theme#128
Hrm, adding a permission check for all the projects returned will definitely slow down the response. We can give this a try and see how it performs if you want. I did a quick PR that implements "permissions": {
"admin": true
}
The PR is at readthedocs/readthedocs.org#10978. |
I've never paid attention to load time using expand fields. Will normal, unexpanded requests trigger the permission check code? I'm assuming it doesn't, which seems like a reasonable middle ground. I'll pull that PR locally and try it out |
Add `?expand=permissions` to project list/detail API endpoints. Related readthedocs/ext-theme#128
So, page load time did indeed improve dramatically using the API over the template Code changes were overall pretty minor, but I did have to switch APIv2 to APIv3 endpoints though. So this puts us back at: That is, we need APIv3 to support unauthenticated, public requests in order to support this functionality for public project views. Needed to continue:
|
Yesterday, I was trying to replicate this change to the organization listing, but found it was still very slow to load. There might be something else happening on that view that is causing the slow down on the initial request. |
So I actually think the organization listing view is not the |
* APIv3: return `permissions` expandable field on projects Add `?expand=permissions` to project list/detail API endpoints. Related readthedocs/ext-theme#128 * APIv3: return `permissions` expandable field on projects Add `?expand=permissions` to project list/detail API endpoints. Related readthedocs/ext-theme#128 * Remove duplicated field * Add test cases for `?expand=permissions`
I've been having a feeling that some pages were behaving slow. In particular project dashboard and build listing. Today, I wanted to remove doubts about this and I checked these times on New Relic:
I haven't explored too much yet because I don't have good connection from the train, but I'm almost sure this is because we are doing many queries to the db to show all the data it's currently showing. With a simple look, I can tell they are the
builds_build
queries the ones that are consuming most of the time in these pages to render.The text was updated successfully, but these errors were encountered: