Add a way to mark http requests as failed or not(passed) #1856
Conversation
mstoykov
added
the
documentation-needed
mstoykov
force-pushed
the
feature/HTTPReqFailures
branch
from
9e2fdaf
to
b746329
Compare
mstoykov
force-pushed
the
feature/HTTPReqFailures
branch
from
761ba10
to
dac7f05
Compare
| c, _ := goja.AssertFunction(rt.GlobalObject().Get("Number").ToObject(rt).Get("isInteger")) | ||
| v, err := c(goja.Undefined(), a) | ||
| return err == nil && v.ToBoolean() |
There was a problem hiding this comment.
Technically goja should be allowing users to overwrite both Number and isInteger so this has the potential to panic, but to be honest I am not certain guarding for this will be all that more useful than just panicking.
And unfortunately, I am not certain how to get this in any other way that will be more ... safe.
Codecov Report
@@ Coverage Diff @@
## master #1856 +/- ##
==========================================
+ Coverage 70.82% 70.86% +0.04%
==========================================
Files 182 183 +1
Lines 14164 14318 +154
==========================================
+ Hits 10032 10147 +115
- Misses 3511 3545 +34
- Partials 621 626 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
js/internal/modules/modules.go
Outdated
| mod := modules[name] | ||
| if i, ok := mod.(interface { | ||
| NewGlobalModule() interface{ NewModuleInstance() interface{} } | ||
| }); ok { |
There was a problem hiding this comment.
This is a bit funky, but it's to avoid an import from internal, right? A comment would be nice to have.
There was a problem hiding this comment.
It was mostly because I didn't want to create a named interface in some package.
But if the approach is approved I am probably going to stuff it in common as I prefer the longer names (not just New) and this has potential for writing it wrong.
I would argue some more "integration" tests will fix that, but it will be nicer to not need those for just checking that you provided the correct interface
There was a problem hiding this comment.
I can also move just the HTTP changes to a separate commit or PR they are kind of small really
There was a problem hiding this comment.
I'd definitely prefer named interfaces. Also, why the 2-level design here? What is the benefit of that compared to having just a single interface with NewModuleInstance()?
There was a problem hiding this comment.
Basically what I said in this comment
Without the two levels, any "global" variables for each VU for a given module will need to be part of the one struct (or global variables).
This makes less of sense for our production code as there we only have 1 instance of those, but for tests I would argue it will be nice to have multiple global module states without the need to clean them between tests ;).
I am actually currently trying to move this to a separate PR with named interfaces and everything but the "global module instance per test" actually needs even more support in the modules registration framework as it technically needs to be made of instance per test and then that one instance to be used to create per VU instances of the module. That is currently not used by anything but the data/SharedArray can use that to not actually have to use context to store its data ;).
I am pretty sure we can make it work without one of the levels(by just skipping the "root" struct) but I think this makes it more explicit and let us (if needed) have a shared state for all the global instances of a module - I can't currently figure out what that will be used for (neither in production code nor in the k6 tests) but 🤷 I prefer not to have to refactor this again just because we figure out a case.
There was a problem hiding this comment.
Yeah ... I also think I prefer to try to make that PR after this is merged ... either as is (in this parts of the code) or with small changes, as it turns out it will need to be slightly bigger and will probably also include slight changes to other modules in order to verify that what we provide as interfaces is sufficient.
For example while trying to do it I figure out that we can technically give some vuState (unlike the lib.State) to each module instance - that can possibly have the context inside of it. That context needs to change between calls to the iterations so we can't just give it a context, so I am not particularly certain it is a great idea but might be what we use to get the context around instead of the current bridge 🤷
There was a problem hiding this comment.
I am very confused, to be honest 😅 I don't see how you's be able to have both per-module and per-VU state with the current code, given that you currently always make both a new "global" module and a new module instance for every VU that imports the module: https://github.com/loadimpact/k6/blob/71a349072b43f59ec716c1c72ee16d0a9c4c8fba/js/internal/modules/modules.go#L42
Besides,
I can't currently figure out what that will be used for (neither in production code nor in the k6 tests) but 🤷
Weren't you the one advocating for the YAGNI principle before? 😄 If you want to merge this as it is, please remove one level of interfaces here and maybe add it again in the proper module. I also think that we might just add a separate interface in the future, if we ever need both per-module and per-VU storage in a module, but we can discuss that in the other PR you want to make.
There was a problem hiding this comment.
The point is that I want to make
- per vu modules(NewModuleInstance) for this PR as that is what we need
- per test modules (NewGlobalModule) for things such as the
data/SharedArray. This is currently not supported ... as well I haven't implemented it ;). Arguably it can be used for something in the HTTP module (or any other modules as well) - the root module (naming stuff is hard 😭 ) is basically there so we can have something to generate the GlobalModule.
Arguably it can just be a function that returns it not an interface (although as we know from net/http those are isomorphic ;) ) which I guess will be ... easier to write by someone developing a module? But I also think that having 2 interfaces and three "types" RootModule, GlobalModule, ModuleInstance (have I mentioned naming stuff is hard) is easier to reason about 🤷♂️
My current plan is that the modules registry will get the RootModule(whether a function or struct/interface) and then to get a module instance out you will need to instantiate the module registry which will call NewGlobalModule once and cache it for each module that is imported and call NewModuleInstance on that GlobalModule each time that is initialized. This (the module registry initialization) is IMO mostly useful for testing as otherwise if you decide to use the module registry in tests you will get 1 shared GlobalModule for all the test in a single golang package.
I guess I can drop some of it .. but if I am going to remake it, either way, remaking it in this PR seems pointless to me ;) We are not making this a public interface yet, or documentation it ...
There was a problem hiding this comment.
👍 🤷♂️ 😅 You are right, something like a global module instance would be better place for the SharedArray values than the current lib.State BS (we probably want to remove even more things from there!), and per-VU storage would be useful for things like NTLM and digest authentication caches.
mstoykov
force-pushed
the
feature/HTTPReqFailures
branch
2 times, most recently
from
8b05abd
to
5b3c26b
Compare
js/internal/modules/modules.go
Outdated
|
|
||
| // PerTestModule is a simple interface representing a per test module object that can be used to | ||
| // make a per VU instance of the Module | ||
| type PerTestModule interface { |
There was a problem hiding this comment.
Hmm this name doesn't seem like a good idea. If a module doesn't implement this interface, it will actually be a per-test module (or, rather, per instance module), but if it does implement it, it will be a per-VU module. So, I think this should be called PerVUModule or ModulePerVU or something like that.
There was a problem hiding this comment.
The idea is that this interface should be the per-test instance and it generates a per VU module. Which it currently does. I do agree it is currently mostly per instance one, but making it truly per test will take more refactoring of the modules so I am not making this in this PR. So I would argue the name is pretty accurate both in what it can be used (in production code) and in what it will stay in the future(even though then there will be a one more level).
The idea is that I will actually continue with #1858 and add the code so that all of that is implemented, but doing it in this PR seems unnecessary and this is all in the internal module, so nobody can use it beside us (mostly xk6 I guess ;)).
There was a problem hiding this comment.
I get the ideas and I still maintain this current interface name is misleading. My suggestions above aren't great, but are definitely less confusing. And if not them, how about HasPerVUModules?
btw do we really need a name for an interface that has per-instance modules? Wouldn't that be covered by just having a different constructor? For example, this code: https://github.com/loadimpact/k6/blob/82cab8b693963659d5f0cfb4cbf785c792577a0f/js/modules/k6/data/data.go#L36-L38
can be something like this:
modules.Register("k6/data", &data{
sharedObjects: common.NewSharedObjects(),
// ... whatever other per-instance state we want
}) There was a problem hiding this comment.
I am for just leaving it for the PR I have already started and will probably publish next week or so.
The answer the other constructor variant:
yes ... but only if you don't want for you to be able to juts import SharedArray from "k6/data" in the js packages without needing to work with data (which will need to be exported) from the k6/data module directly.
So as always , yes we can remove some abstraction, but we have to to then know how to make it work everywhere. And I would argue this will be even more useful for xk6 extensions as ... I technically could've implemented this with one more SharedArray hack like this https://github.com/loadimpact/k6/blob/82cab8b693963659d5f0cfb4cbf785c792577a0f/js/common/initenv.go#L70-L73
There was a problem hiding this comment.
I am for just leaving it for the PR I have already started and will probably publish next week or so.
😕 By then v0.31.0 will be released, so why should this have a misleading name until then when the fix is just to rename it?
There was a problem hiding this comment.
Again the current interface is meant for a per test (...) that has a way to create a per vu module.
Can I have a per-test module that doesn't implement PerTestModule? Of course. Then PerTestModule is not a good name for the interface, is it?
Leaving interfaces unnamed is just postponing the problem where we apparently don't agree on naming things, which probably means we don't think of this in the same terms, which probably means it's a good idea to discuss them now and reach a consensus...
but it is definitely not per VU
That's why I suggested HasPerVUModules as a name of the interface. It signals that the parent object, which is used in modules.Register(), has a unique instance of the module unique in every VU. Here's another suggestion:
// HasModuleInstancePerVU should be implemented by all native Golang modules that
// would require per-VU state. k6 will call their NewModuleInstance() methods
// every time a VU imports the module and use its result as the returned object.
type HasModuleInstancePerVU interface {
NewModuleInstance() interface{}
}There was a problem hiding this comment.
the point of this code is to make the current responseCallback possible without:
- one more global but actually per module shared state store
- not rewriting all the modules to do it.
So it is only checking for a specific case and that will be changed and all modules will just implement the interface as that takes 5 lines and removes the wondering of whetether or not your module is correct or not. And then I would expect that we will just make it required for xk6 modules as well after some time of just logging that the should be implementing the new interface ...
For the purposes of actually merging this code .. .I am fine with w/e I just don't think that a temporary name for a temporary interface in a an internal module, matters enough for a design committee
There was a problem hiding this comment.
It matters because I am the one reviewing the code and the current name doesn't make sense to me. The fact that we still disagree about this means that either the name is bad, or I am completely missing something (which, unless I am a complete idiot, still means something in the code can be better and more obvious).
In any case, even an internal name has to make sense and describe accurately the thing it's naming... I think I understand how, what and why and you're trying to achieve here, but I might be wrong, so I'll take a pause and re-read through this whole discussion and #1858 later today.
There was a problem hiding this comment.
I agree that the current PerTestModule name is slightly confusing. Considering that the function is called NewVUModule(), HasPerVUModule makes sense to me.
In any case, it's an internal interface we can always rename later, so don't think we should spend a lot of time on this.
There was a problem hiding this comment.
I've laid out my arguments in #1858 (comment) how I think this should be evolved in k6 v0.32.0 and why. Having thought about this a lot more and re-read the discussions here and in the issue, I still maintain that this is a better name than the current interface:
type HasModuleInstancePerVU interface {
NewModuleInstance() interface{}
}Maybe even this, if we want to be super-descriptive:
type HasModuleInstancePerVU interface {
NewModuleInstancePerVU() interface{}
}
mstoykov
force-pushed
the
feature/HTTPReqFailures
branch
from
2a8351b
to
fde4130
Compare
This is done through running a callback on every request before emitting the metrics. Currently only a built-in metric looking at good statuses is possible, but a possibility for future JS based callbacks is left open. The implementation specifically makes it hard to figure out anything about the returned callback from JS and tries not to change any other code so it makes it easier for future implementation, but instead tries to do the bare minimum without imposing any limitations on the future work. Additionally because it turned out to be easy, setting the callback to null will make the http library to neither tag requests with `expected_response` nor emit the new `http_req_failed` metric, essentially giving users a way to go back to the previous behaviour. part of #1828
mstoykov
force-pushed
the
feature/HTTPReqFailures
branch
from
fde4130
to
d86e24a
Compare
This is done through running a callback on every request before emitting
the metrics. Currently only a built-in metric looking at good statuses
is possible, but a possibility for future JS based callbacks is left
open.
The implementation specifically makes it hard to figure out anything
about the returned callback from JS and tries not to change any other
code so it makes it easier for future implementation, but instead tries
to do the bare minimum without imposing any limitations on the future
work.
Additionally because it turned out to be easy, setting the callback to
null will make the http library to neither tag requests as passed nor
emit the new
http_req_failedmetric, essentially giving users a way togo back to the previous behaviour.
The cloud output is also not changed as the
http_req_lialready isaggregated based on tags so if an
http_req_liis received that has tagpassed:truethen the wholehttp_req_liis about requests that have"passed".
part of #1828