Add event system for JS modules #3112
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3112 +/- ##
==========================================
- Coverage 72.61% 72.19% -0.42%
==========================================
Files 252 255 +3
Lines 19405 19579 +174
==========================================
+ Hits 14090 14135 +45
- Misses 4422 4547 +125
- Partials 893 897 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
cmd/tests/cmd_run_test.go
Outdated
| // HACK: We need to be able to run multiple tests in parallel on the events | ||
| // module, but since it does event subscription in a sync.Once and the API | ||
| // addresses need to be different for separate tests, we register separate | ||
| // modules to workaround it. I experimented with passing the address via | ||
| // the script, but the sync.Once is still a problem. | ||
| // Note that this also forbids running these tests with more than -count=1, | ||
| // unless we dynamically increment the module name... /sigh | ||
| modules.Register("k6/x/testevents1", events.New( | ||
| ts.GlobalState.DefaultFlags.Address, []event.Type{ | ||
| event.Init, event.TestStart, event.IterStart, event.IterEnd, | ||
| event.TestEnd, event.Exit, | ||
| })) |
There was a problem hiding this comment.
Any ideas for improving this?
Note that this is also the reason for this TestVersion change. Since we can't rely on this registration being stable, the version output could include one or more testevents modules listed as "Extensions". So I just removed that check to avoid it.
There was a problem hiding this comment.
Since some of the last changes in js package the module system gets teh modules it should support
Line 110 in cf90835
This is not exposed so that you can create a runner wtih specific modules.
At this point I feel like this is not a bad idea, but also that likely we should do some moving around of the current modules registration system ... once again.
I don't really like how one of the modules are in one place and the other in another and are being merged strangely.
A quicker fix might be for the Runner to get only the "non-standard"/extension modules. But again this likely will be too big of a change.
I am mostly writing to ask you to actually do teh hack with the ever increasing index so that -count 2 can run as I always find it very strange and frustrating when tests suddenly stop working with it and I am using it for some bug/race teasing.
An example of the hack: (whcih I think I can remove in that place 😅 )
k6/js/init_and_modules_test.go
Lines 38 to 44 in cf90835
There was a problem hiding this comment.
At this point I feel like this is not a bad idea, but also that likely we should do some moving around of the current
modulesregistration system ... once again.
Yeah. The main issue is that there's a global extensions map. Even if modules could be initialized separately or multiple times, they can only be registered once with the same name in this global map.
So we need to return a local map, similarly as in getInternalJSModules(), but first we should get rid of the registration in init() pattern all extensions use, which is at the crux of the issue.
In an initial version of this, I experimented with passing event.System as part of a modules.State object to browser.New(), which meant that modules could be initialized separately with a different event.System, and we wouldn't need to use sync.Once. They could also receive the script options early, to decide if some initialization is needed, which could also be done in NewModuleInstance(), if the module is actually imported. Currently reading options is not possible in NewModuleInstance since vu.State is nil. But the name collision with the global map was still a problem, and since you were against changing New(), I abandoned this.
That module name increment hack is awful, but I suppose it's our only option right now if we want to avoid a substantial refactor, so I'll do that. 😞
There was a problem hiding this comment.
So we need to return a local map, similarly as in getInternalJSModules(), but first we should get rid of the registration in init() pattern all extensions use, which is at the crux of the issue
the init() pattern has nothing to do with this.
Even if we were using something else I doubt the js.Runner would've been written to get it from outside back then to begin with. If it is not taking it from the outside - it will still get it in some "global" way that will mean that you won't control it.
And that is the problem. Removign init() will just break all extensions and will (IMO) make it more likely we will break them again and again in the future.
The curretn solution that I see that won't be that bad is for js.Runner#New (and NewArchive) to take map[string]interface{} with the custom modules and propage it through the code. Instead of it being taken from getInternalJSModules()
The rest of the comment - I don't understand:
- at a call to
New()orNewModulesInstnace()the options are not completed - so getting access to them is pointless, it won't help you in most cases. since you were against changing New()that is because it is not part of the API. It is just a convention. And at the time it is called there is nothing you can do with it. If you want to make it part of the API you will basically make a completely new API.
There was a problem hiding this comment.
the
init()pattern has nothing to do with this.
It does if we want to remove the global singleton, which I'd argue we should. Otherwise you can't guarantee that whatever is tracking external modules is initialized by the time extensions try to register in init().
The curretn solution that I see that won't be that bad is for js.Runner#New (and NewArchive) to take map[string]interface{} with the custom modules and propage it through the code. Instead of it being taken from getInternalJSModules()
That would improve js tests, but we would still have this issue in E2E tests, where we don't have access to the Runner.
at a call to New() or NewModulesInstnace() the options are not completed - so getting access to them is pointless, it won't help you in most cases.
With the current way we initialize JS modules, you're right. We can't pass consolidated options that include the script options without first initializing the module.
But there's no reason module initialization should happen at the moment they're required. Options could be passed lazily once k6 has consolidated them, for which we'd need a new API, yes. It's obvious that the current approach using sync.Once as a workaround is insufficient, and that this global module registry and the way registration is done are parts of the problem.
But we don't have to solve these issues here. I'd be happy to brainstorm some solutions if you're up for it. Let me know.
There was a problem hiding this comment.
It does if we want to remove the global singleton, which I'd argue we should.
That is probably a good idea - but will nto help you at all in this case.
whatever is tracking external modules
If you mean the module that we register them in - I can be certain it wll be initialized as htat is how init figure outs it order - it will initialize first the modules and run the init functions from the leaves of the "module graph" towards the center.
https://go.dev/ref/spec#Package_initialization
A package with no imports is initialized by assigning initial values to all its package-level variables followed by calling all init functions in the order they appear in the source, possibly in multiple files, as presented to the compiler. If a package has imports, the imported packages are initialized before initializing the package itself. If multiple packages import a package, the imported package will be initialized only once. The importing of packages, by construction, guarantees that there can be no cyclic initialization dependencies.
Package initialization—variable initialization and the invocation of init functions—happens in a single goroutine, sequentially, one package at a time. An init function may launch other goroutines, which can run concurrently with the initialization code. However, initialization always sequences the init functions: it will not invoke the next one until the previous one has returned.
That would improve js tests, but we would still have this issue in E2E tests, where we don't have access to the Runner.
so you will need to have a way to propagate this to runner either way.
In the end the runner needs to have the modules somehow and if you want to control what modules get there you will need to provide them somehow.
But there's no reason module initialization should happen at the moment they're required.
well it kind of is required for module instances to be initialized then.
If you mean the "root module" it either needs to be initialized some time before the first module instance or ... just before it.
In both cases - options are not finalized.
I am not certain what the remaining comments are referring to as I haven't code reviewed the whole PR just decided to comment on the particular questions you asked.
But we don't have to solve these issues here. I'd be happy to brainstorm some solutions if you're up for it. Let me know.
I guess given that we are now in the cooldown - we should probably at least try to figure out if we should do it.
There was a problem hiding this comment.
Hey @imiric , thanks a lot for this!
I've reviewed the PR focusing only on whether it fulfills the k6 browser use case requirements or not.
Overall it LGTM. I have only one consideration similar to the one discussed in this thread:
¿Would it be possible to extend the current IterData struct to also send the scenario options? In that case we would have direct access to the browser settings for the scenario, otherwise we'll have to keep a reference to each of the k6modules.VU passed as input to the NewModuleInstance call, is that right? Which most probably would lead us to have more than one event handling goroutine.
|
@ka3de We can certainly send the options in
Don't you already keep this reference? You store it in the context, and then retrieve it with I think you'll need to devise a system with a goroutine that listens for k6 events, dispatches whatever process you need, like starting or stopping browser processes, and stores the mapping of scenario name to WS URL. Then each VU will look up the scenario it's in, and retrieve the WS URL it needs to connect to from the map. At least that's how I'm picturing it, but you guys would know better. |
Yep, I also thought about probably having to make this more generic, and therefore how it could potentially get out of control in terms of data sent. So yes, completely understand these concerns, they make sense.
So we would get all options definition in
Yes, we do, but that is in the context of an already initialized browser instance, as it's in scope only for a single iteration. But in this case, ideally the events listening will be done through a single goroutine for the whole test run, that is the one in charge to initialize the browser instances. On another note. Coming back to this comment, I think I said "too happily" that we could work around this, but, if there is no wait for the I don't have any knowledge on the Edit: To clarify, this waiting on |
|
As discussed, in our initial experimentation, from k6-browser perspective, trying to integrate with the event system implemented in this PR, we encountered an issue that I will try to explain: As per our current model in k6-browser, we start and stop a browser instance per iteration. This is the reason why our main motivation was to listen to the To initialize the browser we need the This For that reason, the proposal is to either:
|
imiric
force-pushed
the
feat/2432-events
branch
2 times, most recently
from
5c5bbc7
to
a97bdd0
Compare
|
Hey @ka3de, please take a look at the latest changes. The event system is now split into a global one that emits the I experimented with the filter approach, and making all events local to the VU, but they were more complex than just splitting it. You would still need to subscribe to VU events in We now also wait for The split has made the test a bit flaky, which is why the CI is currently failing. I'm looking into fixing that, but this shouldn't impact the functionality, and you can start testing. EDIT: OK, the test issues are fixed now. Let me know if you have any other issues with this. |
There was a problem hiding this comment.
LGTM!
I mostly will want to add a note that the Events API is experimental.
My other comments, might not be as valuable, depending on when and how we continue with this event system.
I feel like that for that we probably should discuss what we want from it and how we expect it will work. I personally would've done a lot of stuff in what would be considered "totally the opposite way". But I don't see us discussing this productively in comments in this PR.
And I definitely don't expect that even if we agree on changing on how it works we will do it in this PR.
cmd/run.go
Outdated
| emitEvent(&event.Event{ | ||
| Type: event.Exit, | ||
| Data: &event.ExitData{Error: err}, | ||
| })() |
There was a problem hiding this comment.
I find the calling of the function returned by emitEvent hard to see.
I know what happens here and that we wait for this in this case.
But if I read this in a month while trying to figure out something, I am pretty sure I will miss that we call a function back.
There was a problem hiding this comment.
Sure, I can see that.
Would it help if we named the returned function instead of calling it inline?
| emitEvent := func(evt *event.Event) func() { | ||
| waitDone := c.gs.Events.Emit(evt) | ||
| return func() { | ||
| waitCtx, waitCancel := context.WithTimeout(globalCtx, waitEventDoneTimeout) | ||
| defer waitCancel() | ||
| if werr := waitDone(waitCtx); werr != nil { | ||
| logger.WithError(werr).Warn() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This construct repeats with basically the context and the timeout repeated.
I am not certain if it will be better to
- not have it, but make
waitDone(returned byemit) to take a timeout instead of a context 🤷. Which IMO will make this nicer to write by hand. - move it to
eventspackage and add a "few" arguments
There was a problem hiding this comment.
It did previosly receive a time.Duration, but we decided to replace it with a context because it avoided embedding one in the System struct, and passing one avoids using a separate timer (time.After()). A context is also safer since it can be based on a parent context, which ensures it can exit early. And passing both a context and a duration seems redundant.
So I see this as a helper function specific to each caller. It doesn't make sense to have it part of the event package, as the logic here could be different. It's only similar in both cmd and js because we essentially want to treat it in the same way. But what if the caller wants to log an Error instead, or fail a test, or whatever?
cmd/run.go
Outdated
| // TODO: Subscribe all initialization processes (outputs, VUs and executors) | ||
| // to the Init event. This would allow running them concurrently, and they | ||
| // could be synchronized by waiting for the event processing to complete. | ||
| // This could later be expanded to also initialize browser processes. |
There was a problem hiding this comment.
This has nothing to do with the current PR.
I am not certain that starting to use this for everything we can internally is a good idea. Mostly because I would not find it more readable, and I don't see a case where that will be beneficial.
Direct calls are easier to follow. So at least with the given examples here I feel like this will make things harder to read and follow.
Events make a lot of sense in a lot of settings and in some cases are necessary.
I would recommend opening an issue about this. And maybe presenting a case where moving to events improves things.
I am okay with leaving the TODO
There was a problem hiding this comment.
Mostly because I would not find it more readable, and I don't see a case where that will be beneficial.
The benefit is explained in the TODO: it would allow running all independent initialization processes concurrently, and waiting for them to be done before starting the test. Currently the outputs are initialized first, then the VUs and executors. If we plan to eventually also do browser initialization at this point, which we mostly agree on, then running these sequentially doesn't make sense.
I agree that events make the code more difficult to follow, but the implementation here is not any different from using a WaitGroup or a channel to synchronize concurrent tasks. It just formalizes it with event names.
But since it's too early to make this decision, and it would need more discussion, I'll remove the TODO.
| @@ -42,6 +42,10 @@ type VU interface { | |||
| // Context return the context.Context about the current VU | |||
| Context() context.Context | |||
|
|
|||
| // Events allows subscribing to global k6 execution events, such as Init and | |||
| // Exit, and to local (per-VU) events, such as IterStart and IterEnd. | |||
| Events() common.Events | |||
There was a problem hiding this comment.
Can we put a Note that this is experimental API.
Also we probably should drop it from RegisterCallback 🤔
This addresses the issue that made it difficult to keep track of events for a specific VU, since the browser module needs to initialize browser processes for each VU[1]. This splits the event system into a global and local (per-VU) one, and the JS module can decide to which they want to subscribe to.
The browser module uses the IterStart and IterEnd events for browser initialization and shutdown, so we need to wait for them to complete. There is some concern that this might add some delay to the overall iteration duration, and not just to the iterations where browsers processes are managed, but it should be negligible overall. We should run benchmarks after this change to confirm this.
…rlier This ensures that the Exit event is sent even in the case of an early error, such as a script exception. Resolves #3112 (comment)
imiric
dismissed stale reviews from mstoykov, olegbespalov, and ka3de
via
91fc97e
|
@mstoykov @olegbespalov I addressed most of your comments in 91fc97e, and rebased this on It would be difficult to clean up the history at this point, so I'll just squash+merge this instead. |
This is a simple event system that JS modules written in Go can use to subscribe to test execution events they're interested in, and notify the event emitter when the event processing is done. In practice this is required for some modules like k6-browser so that they can start and stop browser processes depending on the execution status, but it could also be useful to any module that needs to do some cleanup. We could also expose it to outputs or any other extension, and eventually to JS scripts as well.
Currently these events are emitted:
Init: when k6 starts initializing outputs, VUs and executors.TestStart: when the execution scheduler starts running the test.TestEnd: when the test execution ends.IterStart: when a VU starts an iteration.IterEnd: when a VU ends an iteration.Exit: when the k6 process is about to exit.We can discuss if more are needed, but from my discussion with the k6-browser team, these should fulfill their requirements.
Note that this is not meant to be part of v0.45.0, so we should merge it after the release.
Closes #2432