- Notifications
You must be signed in to change notification settings - Fork1k
fix: stop incrementing activity on empty agent stats#15204
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
johnstcn left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Looks good to me! Thanks for finding this! Just need to fix up the failing tests.
| typeBatcherinterface { | ||
| Add(now time.Time,agentID uuid.UUID,templateID uuid.UUID,userID uuid.UUID,workspaceID uuid.UUID,st*agentproto.Stats,usagebool)error | ||
| Add(now time.Time,agentID uuid.UUID,templateID uuid.UUID,userID uuid.UUID,workspaceID uuid.UUID,st*agentproto.Stats,usagebool) |
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.
praise: This does make the whole interface more convenient to use 👍
| err:=r.opts.StatsBatcher.Add(now,workspaceAgent.ID,workspace.TemplateID,workspace.OwnerID,workspace.ID,stats,usage) | ||
| // update prometheus metrics | ||
| ifr.opts.UpdateAgentMetricsFn!=nil { | ||
| user,err:=r.opts.Database.GetUserByID(ctx,workspace.OwnerID) |
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.
thought (non-blocking): It's unfortunate to have to do this just to get the username related to the workspace.
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.
Yeah we just have generally avoided database types that embed other types as a default strategy which leads to cleaner types but unfortunately sometimes more fetching. I think it's OK for now and can always be optimized if found to be a bottleneck later. It's a good goal to get this func as low IO as possible.
0dd942e intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Closes#15174
This adds safeguards around when we decide to bump workspace activity to limit it to only bump if we have active connections to the workspace.
I also cleaned up some code, mostly async code that no longer needed to be async because we moved so many operations to batchers that don't do IO on the
Addcalls.