Skip to content

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

Merged
merged 8 commits into from
Nov 10, 2022

Conversation

f0ssel
Copy link
Contributor

@f0ssel f0ssel commented Oct 31, 2022

No description provided.

@presleyp presleyp requested a review from bpmct November 4, 2022 17:27
@bpmct
Copy link
Member

bpmct commented Nov 5, 2022

Tested this query using a loadscript. Here are the results:

400 workspaces (with count in the route):

Screen Shot 2022-11-05 at 10 21 53 AM

400 workspaces (old query):

Screen Shot 2022-11-05 at 10 28 35 AM

400 users (old query):

Screen Shot 2022-11-05 at 10 21 07 AM

I'd love to have API benchmarking at some point, but it seems like this change doesn't impact the performance of the /api/v2/workspaces endpoint. I'm also a huge fan of having the count in the query!

@bpmct bpmct requested review from presleyp and removed request for bpmct November 5, 2022 15:33
@f0ssel f0ssel closed this Nov 8, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Nov 8, 2022
@presleyp presleyp reopened this Nov 8, 2022
@presleyp presleyp self-assigned this Nov 8, 2022
@presleyp presleyp marked this pull request as ready for review November 8, 2022 21:21
@presleyp presleyp requested a review from a team as a code owner November 8, 2022 21:21
@Emyrk
Copy link
Member

Emyrk commented Nov 8, 2022

@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 rbac.Filter based on roles. Note that this PR uses a sql filter, the benchmark I am posting does an in memory rego query, so not apples to apples, but something to consider. We should write a benchmark for the sql stuff too and see how roles affect it.

cpu: Intel(R) Core(TM) i7-6770HQ CPU @ 2.60GHz
BenchmarkRBACFilter/NoRoles-8              14619             74175 ns/op           21405 B/op        443 allocs/op
BenchmarkRBACFilter/Admin-8              1945520               871.8 ns/op           395 B/op          0 allocs/op
BenchmarkRBACFilter/OrgAdmin-8             13588             84391 ns/op           24594 B/op        504 allocs/op
BenchmarkRBACFilter/OrgMember-8            12663             85327 ns/op           26515 B/op        539 allocs/op
BenchmarkRBACFilter/ManyRoles-8            29784             39719 ns/op           13188 B/op        291 allocs/op
BenchmarkRBACFilter/AdminWithScope-8      772476              1397 ns/op             139 B/op          3 allocs/op

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 true. Where a more complex user will have the sql filter of something like:

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')

@presleyp presleyp requested a review from a team November 9, 2022 14:52
@kylecarbs
Copy link
Member

@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.

@Emyrk
Copy link
Member

Emyrk commented Nov 9, 2022

@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, so it's a slower method than in this PR so this PR is a faster method than the benchmarks.

I mentioned this because if we are going to run benchmarks, we should not run them as an admin. That is all

@f0ssel
Copy link
Contributor Author

f0ssel commented Nov 9, 2022

@bpmct can we run benchmarks as a non-admin?

@bpmct
Copy link
Member

bpmct commented Nov 9, 2022

@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.

@f0ssel
Copy link
Contributor Author

f0ssel commented Nov 9, 2022

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.

Lifting the server siege...
Transactions:                    102 hits
Availability:                 100.00 %
Elapsed time:                   9.76 secs
Data transferred:              59.18 MB
Response time:                  0.09 secs
Transaction rate:              10.45 trans/sec
Throughput:                     6.06 MB/sec
Concurrency:                    0.99
Successful transactions:         102
Failed transactions:               0
Longest transaction:            0.12
Shortest transaction:           0.08

I think this is ready to merge! (I can't approve bc it's my PR technically lol)

Comment on lines +140 to +151
// 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)
Copy link
Member

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

@f0ssel f0ssel merged commit 766a2ad into main Nov 10, 2022
@f0ssel f0ssel deleted the f0ssel/remove-workspace-count branch November 10, 2022 18:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants