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.

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

Fix/1443 remove js runtime from threshold calculation #2400

Merged

Conversation

oleiade
Copy link
Member

@oleiade oleiade commented Feb 25, 2022

This Pull Request does the following:

  • brings back the reverted threshold parsing.
  • adds support for the --no-thresholds option of the run command
  • introduces a Thresholds.Parse method. Calls it as part of the run command
  • modifies tests in thresholds_test.go which assumed UnmarshalJSON would parse thresholds

This PR acts as a tentative to address the minimum scope necessary to merge Thresholds parsing safely to master. It will need to lead to further refactors and improvements both in k6 itself, and in our internal infrastructure.

Reviewers can safely ignore the merge commit, as it was already approved in #2251. Reviewers should focus on the last commit only.

…_the_js_runtime_from_threshold_calcultations""

This reverts commit 22a0db3.
@oleiade oleiade self-assigned this Feb 25, 2022
stats/thresholds_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

Apart from:

  1. the one thing that leads to nil pointer exceptions that I did actually fix in my patch
  2. the tests not calling Parse -which probably would've caught 1

I think this okay 👍

stats/thresholds.go Outdated Show resolved Hide resolved
@oleiade oleiade force-pushed the fix/1443_remove_js_runtime_from_threshold_calculation branch 2 times, most recently from 985d033 to 49cb3d9 Compare February 25, 2022 16:25
cmd/run.go Outdated Show resolved Hide resolved
@oleiade oleiade force-pushed the fix/1443_remove_js_runtime_from_threshold_calculation branch 3 times, most recently from 4190426 to 7fe6d03 Compare February 28, 2022 10:48
This commit ensures that thresholds parsing is a separate step, done
only when the `--no-threshold` flag is not set.

Note that this commit deactivates tests that assumed
`Thresholds.UnmarshalJSON` to effectively parse the thresholds.
@oleiade oleiade force-pushed the fix/1443_remove_js_runtime_from_threshold_calculation branch from 7fe6d03 to 69a2169 Compare February 28, 2022 11:11
@na-- na-- added this to the v0.37.0 milestone Mar 2, 2022
// If parsing the threshold expressions failed, consider it as an
// invalid configuration error.
if !runtimeOptions.NoThresholds.Bool {
for _, thresholds := range conf.Options.Thresholds {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if we move this loop to the conf.options.ValidateTreshholds ? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that is probably a small enough change to be done, but given that we are trying to get it merged before the release - I vote for leaving this for the next PR in the next release that will either way add a bunch of Validation.

}

// Otherwise, attempt to parse a percentile expression
if strings.HasPrefix(input, tokenPercentile+"(") && strings.HasSuffix(input, ")") {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we change the logical condition to, we can benefit from the early return with the error that strictly follows from the check, like:

if !strings.HasPrefix(input, tokenPercentile+"(") || !strings.HasSuffix(input, ")") {
     return "", null.Float{}, fmt.Errorf("failed parsing method from expression")
}

And this change also reduces the nesting of the ifs, which improves readability.

WDYT?

Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

LGTM - I am going to make some more manual tests but hopefully we don't need to change anything to get this in v0.37.0 and make other non necessary fixes for v0.38.0

Copy link
Contributor

@olegbespalov olegbespalov left a comment

Choose a reason for hiding this comment

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

Okay, keeping my comments for the following PRs 👍

@oleiade
Copy link
Member Author

oleiade commented Mar 3, 2022

Thanks everyone, for your input 🎉
Merging 🦖

@oleiade oleiade merged commit 1e28a3e into master Mar 3, 2022
@mstoykov mstoykov deleted the fix/1443_remove_js_runtime_from_threshold_calculation branch October 4, 2022 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants