Fix/1443 remove the JS runtime from threshold calculations

[oleiade]

This Pull Request addresses the minimum scope described in #1443 and replaces the current thresholds' expression evaluation implementation. It drops the reliance on the Goja JS runtime, in favor of a pure-go based implementation. With this PR, thresholds are parsed, and evaluated directly in Go, and all JS runtime-related threshold code has been dropped. It sticks to the existing format, and does not introduce any of the design and format changes proposed in #1443 and #1441.

Design notes regarding the PR's content:

• The introduction of a minimalistic parser combinators library, taken and adapted from Jeffhail benthos' bloblang implementation. The design choice of combinators was motivated by their ease of use, of extension, and general maintainability and performance. Looking at Remove the JS runtime from threshold calculations #1443 and [Archived] Thresholds overhaul - the ideal state of thresholds #1441 it also appeared like the best choice when it comes to further improvements we might want to make. The choice to extract the code from Bloblang was motivated by the fact that this was definitely the cleanest and idiomatic implementation I could find; but it was unfortunately (to my knowledge) not offered as a dedicated library module. I also brought a few improvements in the process, by modifying and extending the existing API to fit K6's use-case.
• I put the library's code in pkg. I was not entirely sure what the best place would be for code that's generic enough to be a good independent library, but not big enough to make it into a dedicated repo. pkg is often used as the place where projects put their public reusable code. I'm not hung onto it at all. Please propose an alternative if you prefer to put it in another place 👀
• I added a sink map to the Thresholds struct, and pass it around to children Threshold instances when they need to be evaluated. It's the simplest thing I could think of, but with my limited understanding of the current design of K6, I might have missed another way of doing it.

Let me know what you think 👀 🐻

[oleiade]

I'd be really interested in your thoughts on how I approached testing @inancgumus; I'm all ears for any recommendations on that topic specifically 👂🏻 🙏🏻

[oleiade]

The Pull Request makes the engine's TestEngineAbortedByThresholds test fail, which raises a question on my side: from what I gather, the test verifies that a threshold expression that evaluates to false will lead it to abort. However, the expression it tests against is 1+1==3, which is a valid JS expression, but which the Go parser considers as invalid. Do we want to support this kind of expressions still? If not, how should we go about the multitude of test cases depending on expressions such as 1+1==2 (I've come accross them quite often)?

[mstoykov]

I haven't gone through the code, yet, but I would expect expressions like 1+1=2 and so on to possibly be valid. But also from UX perspective, it will be better if we require that something from the metric is used and if not to error that the threshold isn't based on the metric.

Given that, for the purposes of the tests, you can rewrite (as you seem to have done) in a way that you get the test to pass -where it requires that the threshold fails just to write a threshold that will fail when evaluated but still based on the metric.

[yorugac]

@oleiade I've only started digging into this, hopefully will finish tomorrow 😅 But I have a preliminary question: the work you reference in the PR description (Jeffhail benthos' bloblang) -- should it be referenced in the code as well? I imagine it depends on how much you adapted it...

Also, CI failure shows:

--- FAIL: TestClient (0.01s)
    --- FAIL: TestClient/ReflectUnregistered (0.03s)
        client_test.go:650: 
            	Error Trace:	client_test.go:650
            	            				client_test.go:687
            	Error:      	"can't list services: can't send request: EOF at go.k6.io/k6/js/common.Bind.func1 (native)" does not contain "rpc error: code = Unimplemented desc = unknown service grpc.reflection.v1alpha.ServerReflection"
            	Test:       	TestClient/ReflectUnregistered
FAIL

This doesn't look like a flaky one... Did it happen before? I can't repeat it on the current master 😕

[oleiade]

@yorugac Sorry for the huge chunk of code I've just dropped 🙏🏻 I prefer to make smaller iterative PRs in general, but I found it hard to avoid in this case. You're right about mentioning Jeff Hail's work somehow in the code. I wanted to find out what would be the best way to do that, and it somehow slipped out of my mind. I've adapted and extended the API, and modified some implementation here and there, but a big part of the code is still verbatim. I would like to add a copy of his copyright license at the top of the file (as is the MIT license etiquette, I think?); and mention that the base code comes from bloblang's implementation. Does that sound like a good approach.

Regarding the failing test, as I haven't touched the code since yesterday, and only rebased my PR on master, I can only assume that either the failing test comes from master, or that it impacted my code's behavior somehow. I'll look into it, thanks for pointing it out! 👍🏻

[oleiade]

@yorugac I've modified the combinators package description to mention that most of the code comes from jeffhail's work. I also added their License to the top of the file, as the MIT requests. Do you find it sufficient?

[yorugac]

Thank you for the clarification on licenses, @oleiade ! It is now clear enough to me what happens with the code adaptation in this PR 👍 But I'm not sure about how it should be done in k6 code base. I.e. should there be 2 full license headers as is currently done in combinators.go: it takes a lot of space and IIRC, there was some talk about dropping all of those headers? The only precedent in k6 that I know of ATM is the tc39 files and those are left without a header.
@na--, @mstoykov, could you please add your opinion on what should be done in this case with license / authorship?

On the review part. combinators pkg looks quite nice and Result is reminding me of some other languages 😄 I'm not sure about its name though: maybe rename it to just parser? I currently assume "combinators" came from the original code but it doesn't make much sense in k6 context IMHO. I don't have a strong opinion about pkg folder except there is only lib at the moment: are we moving lib -> pkg? It doesn't make sense to me to have 2 folders without understanding the difference between them.

Overall, I like how this change shapes up a lot! Theo, please see the comments: there are some suggestions about renamings and some requests about logic and clarifications. And one more thing I've been wondering about: should some benchmarking be done here?

[oleiade]

@yorugac Thanks for the review! I'll look into your comments and address them when I can.

Regarding the naming, I don't think parser would make a lot of sense considering that parser combinators are actually a "generic" parsing technique/toolbox you build parsers on top of: as opposed to a recursive descent or LRLA parser for instance, which you tailor specifically to the format you're trying to parse. I also kept it separate in the spirit of a) being able to reuse it to extend the threshold's parsing in the future (to fit with the initial proposal this PR partly answers to) and b) making it possible for a potential third-party repo to use it directly from our repo by adding github.com/grafana/k6/pkg/combinators to their dependencies. Initially I considered naming it crocombine and putting in a separate repo...

Regarding benchmarking, I've thought about it, without never actually getting to implementing it. Now that you ask about it, I'll add some :)

[oleiade]

Note that I will make sure to squash all "Update", and "fixup!" commits before merging 👍🏻

[yorugac]

LGTM! According to the current "thresholds plan" as was discussed in this PR 😄

[na--]

The next step here, in another PR, would be to tie the threshold validation with the metrics in our metrics registry. This validation is awesome, much better than what existed (or rather, what didn't exist 😅) before, but we can do it right after the initial execution of the init context. Right now, this script:

import { sleep } from 'k6';
import { Counter } from 'k6/metrics';
import exec from 'k6/execution';


const myCounter = new Counter('my_counter');

export const options = {
    vus: 5,
    iterations: 30,
    thresholds: {
        my_counter: ['p(95)<200'], // error, Counters do not have p()
    },
};


export default function () {
    console.log(`[t=${(new Date()) - exec.scenario.startTime}ms] VU{${exec.vu.idInTest}} ran scenario iteration ${exec.scenario.iterationInTest}`);
    myCounter.add(1);
    sleep(1);
}

will emit something like this:

INFO[0000] [t=0ms] VU{3} ran scenario iteration 0        source=console
INFO[0000] [t=0ms] VU{5} ran scenario iteration 2        source=console
INFO[0000] [t=0ms] VU{2} ran scenario iteration 1        source=console
INFO[0000] [t=0ms] VU{1} ran scenario iteration 3        source=console
INFO[0000] [t=0ms] VU{4} ran scenario iteration 4        source=console
INFO[0001] [t=1001ms] VU{2} ran scenario iteration 7     source=console
INFO[0001] [t=1001ms] VU{4} ran scenario iteration 5     source=console
INFO[0001] [t=1001ms] VU{3} ran scenario iteration 9     source=console
INFO[0001] [t=1001ms] VU{5} ran scenario iteration 6     source=console
INFO[0001] [t=1001ms] VU{1} ran scenario iteration 8     source=console
ERRO[0002] Threshold error                               component=engine error="threshold 0 run error: unable to apply threshold p(95)<200 over metrics; reason: no metric supporting the p(95) aggregation method found" m=my_counter

but right now there is nothing preventing us from validating the thresholds before we actually start running the script, registry should have all of the metrics (custom or built-in) and their types right after the last line here:

k6/cmd/run.go

Lines 113 to 115 in 23575eb

	registry := metrics.NewRegistry()
	builtinMetrics := metrics.RegisterBuiltinMetrics(registry)
	initRunner, err := newRunner(logger, src, runType, filesystems, runtimeOptions, builtinMetrics, registry)

[na--]

Fixing #1346 can probably be done at the same time as ⬆️ , but I'm not finding an already open issue for it. @oleiade, do you want to create one or should I?

[na--]

And, thinking about it, #2320 will also be easier when we don't have to do any validation in Thresholds.Run() 🤔 🎉 Each threshold can be "pre-compiled" by just turning it into a Go lambda or something like that. Once we do the validation once after the init context execution, that a threshold impression is valid for the metric type it is defined for, we shouldn't need any more validation afterwards 🎉

[oleiade]

Very good points indeed! I've created #2330 to keep track of it 🎉

[na--]

LGTM, so much nicer than the JS hack ❤️ 😅 it also unlocks us to implement so many other fixes and improvements like https://github.com/grafana/k6/pull/2251/files#r782875298, #1346 and threshold improvements in general 🎉

[oleiade]

We experienced issue with this feature in the larger, internal, context of K6, and decided to revert it, and delay its release to v0.37.0. See #1443 for more details.