-
Notifications
You must be signed in to change notification settings - Fork 903
chore: refactor workspace count to single route #4809
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
Conversation
Tested this query using a loadscript. Here are the results: 400 workspaces (with count in the route): 400 workspaces (old query): 400 users (old query): I'd love to have API benchmarking at some point, but it seems like this change doesn't impact the performance of the |
@bpmct one thing to note is that the roles of the user can affect performance. An admin user is always going to be the fastest. This is a benchmark of the
The tl;dr is that the more complex the role combination, the more complex the sql filter string will be. We can add indexs to help things out. An admin role has the sql filter string of SELECT * FROM workspaces WHERE
-- The filter
(organization_id :: text != '' OR
organization_id :: text = ANY(ARRAY ['a','b','c']) OR
organization_id :: text != '' OR
group_acl->'allUsers' ? 'read' OR
user_acl->'me' ? 'read') |
@Emyrk oh dang, based on that it seems like querying 400 workspaces for a non-admin could take ~15s if I understand correctly? If that's the case... we should probably not implement this way and eventually use an estimated count. |
No that's not correct. I'm just saying the filter will be slower for a normal user. My benchmark above is not using the SQLfilter, I mentioned this because if we are going to run benchmarks, we should not run them as an admin. That is all |
@bpmct can we run benchmarks as a non-admin? |
@f0ssel my benchmarks are just done via bash scripts that leverage the CLI so you can run actions on behalf of another user with a token. |
After doing my own testing I see no measurable difference in the performance of this endpoint with and without the count field being returned. This is from a regular user on a deployment with 500 other users and 500 workspaces.
I think this is ready to merge! (I can't approve bc it's my PR technically lol) |
// run the query again to get the total count for frontend pagination | ||
filter.Offset = 0 | ||
filter.Limit = 0 | ||
all, err := api.Database.GetAuthorizedWorkspaces(ctx, filter, sqlFilter) | ||
if err != nil { | ||
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ | ||
Message: "Internal error converting workspaces.", | ||
Message: "Internal error fetching workspaces.", | ||
Detail: err.Error(), | ||
}) | ||
return | ||
} | ||
count := len(all) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should still do a query with COUNT(*)
, however it doesn't have to be a duplicated query it could just be some sort of string replace on the query in the database package to avoid the duplicated code issue that this PR tries to fix. Garrett argues that it doesn't add any time to the request from his tests though so at the end of the day it's probably fine
No description provided.