- Notifications
You must be signed in to change notification settings - Fork1k
chore(coderd/database): optimize AuditLogs queries#18600
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…dit logs query with conditional joins
| workspace_builds.build_number=1 | ||
| ) | ||
| LEFT JOIN organizationsONaudit_logs.organization_id=organizations.id | ||
| SELECTsqlc.embed(audit_logs), |
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'd a comment thatCountAuditLogs also needs to be extended whenever we extend the filtering onGetAuditLogsOffset
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.
Or maybe better - add a test that compares the filter object for this method and count method
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 have added a new test which compares WHERE clause for both queries :)
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.
LGTM. I've based my implementation of the connection log queries off this PR without any trouble:#18629
Uh oh!
There was an error while loading.Please reload this page.
coderd/audit.go Outdated
| dblogs,err:=api.Database.GetAuditLogsOffset(ctx,filter) | ||
| // Use the same filters to count the number of audit logs | ||
| count,err:=api.Database.CountAuditLogs(ctx, database.CountAuditLogsParams{ |
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.
It's going to be very easy for someone to forget to update this in both places. Maybe you should make thesearchquery.AuditLogs package return both of these params types so they're constructed in the same place
Or add a comment in both places
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.
Moved to searchquery
| LEFT JOIN usersONaudit_logs.user_id=users.id | ||
| LEFT JOIN organizationsONaudit_logs.organization_id=organizations.id | ||
| -- First join on workspaces to get the initial workspace create | ||
| -- to workspace build 1 id. This is because the first create is | ||
| -- is a different audit log than subsequent starts. | ||
| LEFT JOIN workspacesON | ||
| audit_logs.resource_type='workspace'AND | ||
| audit_logs.resource_id=workspaces.id |
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.
The indent formatting in this whole file seems messed up now
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.
Fixed
695de6e intomainUh oh!
There was an error while loading.Please reload this page.
Closes#17689
This PR optimizes the audit logs query performance by extracting the count operation into a separate query and replacing the OR-based workspace_builds with conditional joins.
Query changes
workspace_buildsjoin with OR conditions with separate conditional joinswb_buildfor workspace_build audit logs (which is a direct lookup)wb_workspacefor workspace create audit logs (via workspace)Optimized AuditLogsOffset query:
https://explain.dalibo.com/plan/4g1hbedg4a564bg8
New CountAuditLogs query:
https://explain.dalibo.com/plan/ga2fbcecb9efbce3