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

Refactor group() and check() tight coupling with the VU.State #2869

Closed
na-- opened this issue Jan 23, 2023 · 1 comment · Fixed by #3750
Closed

Refactor group() and check() tight coupling with the VU.State #2869

na-- opened this issue Jan 23, 2023 · 1 comment · Fixed by #3750
Assignees
Labels
Milestone

Comments

@na--
Copy link
Member

na-- commented Jan 23, 2023

This tight coupling is preventing us from implementing group() and check() purely in JavaScript. It is also preventing us from implementing similar functions, but with better UX (e.g. #1066, #2865) or that work better with async / await (#2848, #2728)

The tight coupling can be removed with only a very small loss of efficiency, by just having the MetricEngine process check and group_duration metrics as they appear and group them based on their group tag. Here is what basically needs to be removed:

k6/lib/vu_state.go

Lines 41 to 42 in 1d99b0b

// Current group; all emitted metrics are tagged with this.
Group *Group

k6/lib/runner.go

Lines 71 to 72 in 1d99b0b

// Returns the default (root) Group.
GetDefaultGroup() *Group

k6/js/modules/k6/k6.go

Lines 98 to 104 in 1d99b0b

g, err := state.Group.Group(name)
if err != nil {
return goja.Undefined(), err
}
old := state.Group
state.Group = g

check, err := state.Group.Check(name)

@na-- na-- added the refactor label Jan 23, 2023
mstoykov added a commit that referenced this issue Feb 14, 2023
Before this check was only returning `false` if it also could emit it,
and true otherwise.

This should rarely be a problem as the same context that is being
checked here is the one that interrupts the VM - so no code will be able
to run after this.

Unfortunately the code checking if it should be emitted is racing with
the one that interrupts the VM. So it is possible for the VM to still
not be interrupted when a `check` returns a wrong value. It is even
possible for more code to be run before the interrupt is actually
called.

The code still checks the context as this also updates the internal
check structure and we don't want to that if the context is done.
The above should be changed with #2869

Fixes #2912
mstoykov added a commit that referenced this issue Mar 6, 2023
Before this check was only returning `false` if it also could emit it,
and true otherwise.

This should rarely be a problem as the same context that is being
checked here is the one that interrupts the VM - so no code will be able
to run after this.

Unfortunately the code checking if it should be emitted is racing with
the one that interrupts the VM. So it is possible for the VM to still
not be interrupted when a `check` returns a wrong value. It is even
possible for more code to be run before the interrupt is actually
called.

The code still checks the context as this also updates the internal
check structure and we don't want to that if the context is done.
The above should be changed with #2869

Fixes #2912
@mstoykov mstoykov self-assigned this Apr 25, 2023
@mstoykov mstoykov added this to the v0.45.0 milestone Apr 26, 2023
@andrewslotin andrewslotin modified the milestones: v0.45.0, v0.46.0 May 31, 2023
@mstoykov mstoykov modified the milestones: v0.46.0, v0.47.0 Jul 31, 2023
@codebien codebien modified the milestones: v0.47.0, v0.48.0 Sep 25, 2023
@olegbespalov olegbespalov removed this from the v0.48.0 milestone Nov 16, 2023
@olegbespalov
Copy link
Contributor

For the record. After an internal discussion, we decided to clean a milestone since the issue was jumping between milestones without completion.

Once we determine which milestone it lands, we set the right one.

@mstoykov mstoykov removed their assignment May 9, 2024
@mstoykov mstoykov self-assigned this May 21, 2024
@mstoykov mstoykov added this to the v0.52.0 milestone May 21, 2024
mstoykov added a commit that referenced this issue May 21, 2024
Group and check were tightly coupled between them and also to the end of
test summary and the REST API.

This this change the tight coupling is removed. Group and check no
longer interact with each other while being ran.

In order to keep the same end of test summary and REST API behavior the
code makes the same aggregation it used to be but *only* based on the
metrics emitted by `check` and `group`.

There are several breaking changes still in it as a bunch of things are
no longer useful, and technically not implementable if we want to remove
the tight coupling.

The only one that is relevant though is that
`lib.State` no longer has `Group`.

There are 2 extensions using that and both of them use it to use the
"magic" that tight `group` and `check` together.

Fixes #2869
mstoykov added a commit that referenced this issue May 21, 2024
Group and check were tightly coupled between them and also to the end of
test summary and the REST API.

This change the tight coupling is removed. Group and check no
longer interact with each other while being run.

In order to keep the same end of test summary and REST API behavior the
code makes the same aggregation it used to be but *only* based on the
metrics emitted by `check` and `group`.

There are several breaking changes still in it as a bunch of things are
no longer useful, and technically not implementable if we want to remove
the tight coupling.

The only one that is relevant though is that
`lib.State` no longer has `Group`.

There are 2 extensions using that and both of them use it to use the
"magic" that tight `group` and `check` together.

Fixes #2869
@mstoykov mstoykov linked a pull request May 28, 2024 that will close this issue
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants