- Notifications
You must be signed in to change notification settings - Fork1k
feat: add scaletest Runner for dynamicparameters load gen#19890
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
a368b05 to0b6d03cCompareThere 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 after a lint
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| typeConfigstruct { | ||
| TemplateVersion uuid.UUID`json:"template_version"` | ||
| SessionTokenstring`json:"session_token"` | ||
| Metrics*Metrics`json:"-"` | ||
| MetricLabelValues []string`json:"metric_label_values"` | ||
| } |
ethanndicksonSep 22, 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.
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.
Optional: could use a Validate function here, I've found the one on my load generator somewhat useful, such as for checking that my time.Duration timeouts aren't zero.
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.
Overall this is a good start. It is missing tests/assertions for some more dynamic elements, modules, some terraform functions, user context.
In my experience, the verbosity required to run an exchange and test is just too cumbersome. So we should come up with some patterns like the unit tests have:
coder/enterprise/coderd/dynamicparameters_test.go
Lines 566 to 584 in0b6d03c
| preview,pop:=coderdtest.SynchronousStream(stream) | |
| init:=pop() | |
| require.Len(t,init.Diagnostics,0,"no top level diags") | |
| coderdtest.AssertParameter(t,"isAdmin",init.Parameters). | |
| Exists().Value("false") | |
| coderdtest.AssertParameter(t,"adminonly",init.Parameters). | |
| NotExists() | |
| coderdtest.AssertParameter(t,"groups",init.Parameters). | |
| Exists().Options(database.EveryoneGroup,"developer") | |
| // Switch to an admin | |
| resp,err:=preview(codersdk.DynamicParametersRequest{ | |
| ID:1, | |
| Inputs:map[string]string{ | |
| "colors":`["red"]`, | |
| "thing":"apple", | |
| }, | |
| OwnerID:userAdminData.ID, | |
| }) |
Then it could be easier to add more query/response round trips and assert more properties of the response.
| case resp, ok := <-respCh: | ||
| if !ok { | ||
| return xerrors.Errorf("dynamic parameters stream closed before change response") | ||
| } | ||
| _, _ = fmt.Fprintf(logs, "change response: %+v\n", resp) | ||
| r.cfg.Metrics.LatencyChangeResponseSeconds. | ||
| WithLabelValues(r.cfg.MetricLabelValues...). | ||
| Observe(time.Since(initTime).Seconds()) | ||
| if resp.ID != 1 { | ||
| return xerrors.Errorf("unexpected response ID: %d", resp.ID) | ||
| } | ||
| if err := checkNoDiagnostics(resp); err != nil { | ||
| return xerrors.Errorf("unexpected change response diagnostics: %w", err) | ||
| } | ||
| return 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.
We do not assert any param condition on the response. I wonder if we can refactor theParameterAsserter structure to be useful here:
| funcAssertParameter(t*testing.T,namestring,params []codersdk.PreviewParameter)*ParameterAsserter { |
It really streamlines the assertion code to something like:
coderdtest.AssertParameter(t,"groups",resp.Parameters).Exists().Options(database.EveryoneGroup,"admin","auditor").Value("admin")
It just currently only works in unit tests.
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 feel like this is missing the point --- we are scale testing, not functional testing. I'm not looking to check that the response we get is "correct" in any detailed sense of the term, just that the response has not thrown errors and is doing the computation we want it to.
Uh oh!
There was an error while loading.Please reload this page.
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.
Is this using the echo provisioner? And is that intentional?
That's the test of the scaletest runner/load generator. The actual scale test won't use the echo provisioner. |
I'm not sure what you mean by "more dynamic elements." Modules is a good call out. Are there particular terraform functions that are relevant for a scale test? The point here is not to verify the functionality of dynamic parameters, but how it behaves under load. The test template does make use of the user roles and username. Are there other elements of the user context that need special attention in terms of scale? |
0b6d03c tocefa48cCompare289f021 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
relates tocoder/internal#912
Adds a new scaletest Runner to generate dynamic parameters load.
A later PR will add the CLI command, including creating the template & version.