Fix races around touching state.Group outside of the event loop

[mstoykov]

There is no locking around the state.Group and it is modified by calls to group() while on the event loop. So for the time being to modify it - it is needed to be on the event loop.

Also the adding of the tag isn't needed as that is done automatically by group()

[mstoykov]

found while testing for #542

[inancgumus]

LGTM, but I believe we need a new issue to add Group support back in the xk6-browser.

[mstoykov]

a new issue to add Group support back in the xk6-browser.

what do you mean?

the tag group is still added - the code that is removed here didn't need to exist.

Do you want an issue to make the logging of group to be brought back for the two cases where it was removed?

[inancgumus]

Do you want an issue to make the logging of group to be brought back for the two cases where it was removed?

Never mind, I confused it with something else. The PR is good as it is 👍

[imiric]

Hey, I'm just getting around to reviewing this, apologies for the delay.

I'm confused about this:

Also the adding of the tag isn't needed as that is done automatically by group()

group() only adds the tag for the group_duration metric, but not for the metrics we're emitting here (data_sent), nor for the logger fields we're adding.

I see that you created grafana/k6#2728, which is probably why I wasn't able to make it work just now testing on the main commit before this was merged (a23053d), but it brings up the question of how can we make this work for custom metrics, like browser_dom_content_loaded, which didn't even access state.Group. We should figure out a way to make this work for all cases, so 👍 on adding an issue for it.

It seems unlikely that group() will be deprecated anytime soon, and there are some non-trivial API changes needed to support these async scripts, as that issue mentions. 😅

[mstoykov]

group() only adds the tag for the group_duration metric

https://github.com/grafana/k6/blob/0568f273b157829d94b7869cd495025eadd9f2bb/js/modules/k6/k6.go#L108-L111

It does add them to the state.Tags from which you fork the tags and then emit the metric. So if you are within a group and you are not hitting 2728 - you don't need the code I have removed. If you are hitting 2728 - the code I removed again doesn't help you ;)

So the answer to what should xk6-browser team do is - nothing ;)

It seems unlikely that group() will be deprecated anytime soon, and there are some non-trivial API changes needed to support these async scripts, as that issue mentions. sweat_smile

I mean ... "unlikely" yes, but depending on what the fixes for 2728 turn out to be, it might turn out that it makes more sense than anything else.

In general I would argue that when emitting metrics asyncrhonously you should note/save the tags from the lib.State when you start the asynchrnous call while still on the event loop. And then use that. This also will work with single level group ... still won't work if you have a then afterwards.

group("some group", () => {
  someAsyncCall(). // if before someAsyncCall returns a promise it gets the `state.Tags` - it will be fine
   then(() => {
    // anything here will not run in the "context" of the group
    // this is what 2728 is about.
   })
});