-
Notifications
You must be signed in to change notification settings - Fork 1.1k
perf: reduce calls to GetWorkspaceByAgentID in GetWorkspaceAgentByID #21046
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
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.
Should we add a fast path test here?
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 not very easy to do with the amount of mocking we do in some of these tests, especially given that tests circumvent actually calls to the DB to retrieve the agent in the agentFn
the function and test that are updated in dbauthz are the code path that will be executed in production when agentFn is called from any of the sub-APIs in the agentapi package
| } | ||
| } | ||
|
|
||
| // Inject RBAC object into context for dbauthz fast path, avoid having to |
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.
Any opportunity to create a helper for this logic?
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.
definitely! I'd just thought of that while implementing yet another PR for this same fastpath pattern, and added the helper function there.
I had planned to do all the cleanup to use the helper in existing places in a single PR after the existing PRs, easier for reviewers IMO, but I can refactor any one of the existing PRs to do the helper + reuse in there if you think that's easier.
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.
Emyrk
left a comment
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 helper function to deduplicate code would be preferred 👍
|
All contributors have signed the CLA ✍️ ✅ |
8c4a0f5 to
067ab1d
Compare
4bc6a88 to
1ec456b
Compare
1ec456b to
23187b4
Compare
#21047) This PR piggy backs on the agent API cached workspace added in earlier PRs to provide a fast path for avoiding `GetWorkspaceByID` calls in `GetLatestWorkspaceBuildByWorkspaceID` via injection of the workspaces RBAC object into the context. We can do this from the `agentConnectionMonitor` easily since we already cache the workspace. --------- Signed-off-by: Callum Styan <callumstyan@gmail.com>
23187b4 to
3af884b
Compare
This PR piggy backs on the agent API cached workspace added in #20662 to provide a fast path for avoiding
GetWorkspaceByAgentIDcalls in dbauthz'sGetWorkspaceAgentByID. This query is not the most expensive, but has a significant call volume at ~16 million calls per week.Profiling shows that the majority of these calls are via the agent API's

agentFn.