Skip to content
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.

Sign up for GitHub

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

Report usages of the deprecated 'ext.loadimpact' option #4084

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+9 −1
Open

Report usages of the deprecated 'ext.loadimpact' option #4084

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion cmd/test_load.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,11 +265,19 @@ func loadSystemCertPool(logger logrus.FieldLogger) {
func (lct *loadedAndConfiguredTest) buildTestRunState(
configToReinject lib.Options,
) (*lib.TestRunState, error) {
// This might be the full derived or just the consodlidated options
// This might be the full derived or just the consolidated options
if err := lct.initRunner.SetOptions(configToReinject); err != nil {
return nil, err
}

// Here, where we get the consolidated options, is where we check if any
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it here because it looks like a considerably safe place among all the places configuration is read/written and manipulated in general, but any suggestion will be more than welcome.

Copy link
Contributor Author

@joanlopez joanlopez Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, note that this will be reported despite it's a local (k6 run) or a remote execution (k6 cloud), but I guess it's fine because a script that holds ext.loadimpact is still a candidate of experiencing issues if we remove such support.

In any case, if we want to get more accurate metrics (only cloud executions), we can always account reports with this and a test run id.

// of the deprecated options is being used, and we report it.
if _, isPresent := configToReinject.External["loadimpact"]; isPresent {
if err := lct.preInitState.Usage.Strings("deprecated_options", "ext.loadimpact"); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In https://github.com/grafana/k6/pull/4030/files the key is deprecations/<what is deprecated>. I would prefer if we keep the keys more generic and deprecated_options is very specific to options by the looks of it and I do not know of any other option that will make it relevant to have a separate key.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, is your suggestion: deprecations/ext.loadimpact?
I'm happy with that one as well, sure!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that looks okay - yes. Don't know if we want options somewhere in there, but as a one of case this seems good enough.

return nil, err
}
}

// it pre-loads system certificates to avoid doing it on the first TLS request.
// This is done async to avoid blocking the rest of the loading process as it will not stop if it fails.
go loadSystemCertPool(lct.preInitState.Logger)
Expand Down
Loading