- Notifications
You must be signed in to change notification settings - Fork1.1k
feat: add task status reporting load generator runner#20538
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
base:main
Are you sure you want to change the base?
Conversation
spikecurtis commentedOct 29, 2025 • 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.
This stack of pull requests is managed byGraphite. Learn more aboutstacking. |
6301335 to6d573a2Compare
ethanndickson 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.
LGTM, only nitpicks from me
| type Config struct { | ||
| // AgentID is the workspace agent ID to which to connect. | ||
| AgentID uuid.UUID `json:"agent_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.
Looks unused since task statuses are reported per-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.
Good catch.
It's actually worse than this though. The API route to report status only works with an Agent Token; a regular API token won't work. So, we need some way to either run this in the workspace or otherwise get ahold of the key.
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 don't recall the details of how external workspaces work - could we just register ourselves as an external 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.
| } | ||
| func (c *Config) Validate() error { | ||
| if c.AgentID == uuid.Nil { |
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.
| ifc.AgentID== uuid.Nil { | |
| ifc.WorkspaceID== uuid.Nil { |
| MetricLabelValues []string `json:"metric_label_values"` | ||
| } | ||
| func (c *Config) Validate() error { |
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 unused
Uh oh!
There was an error while loading.Please reload this page.
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| case <-r.cfg.StartReporting: | ||
| r.logger.Info(ctx, "starting to report task status") |
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.
IfwatchWorkspaceUpdates always callsDone on the waitgroup, even if it fails, we wouldn't need this timeout right? Then you could get rid of theStartReporting channel, and just have this function wait on the waitgroup.
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.
TheRun Sequence calls for waiting to establish a baseline after all updates websockets are connected before we start sending status reports. That's the purpose of thisStartReporting channel: it allows the calling code to wait for the baseline, then start all the status reports.
8c9b24f todc27761Compare6d573a2 to8bafc4fCompare8bafc4f tobd4b0c6Compare
Uh oh!
There was an error while loading.Please reload this page.
Adds the Runner, Config, and Metrics for the scaletest load generator for task status.
Part ofcoder/internal#913