group doesn't work with async calls well

[mstoykov]

The following script:

import { Counter } from "k6/metrics";
import { group } from "k6";
const delay = () => { return Promise.resolve(); } // this is just a placeholder for something useful.
let l = new Counter("my coutner")

export default () => {
    group("coolgroup", () => {
        l.add(1) // nice

        delay(1).then(() => {
                l.add(1)// not so nice
        })
    })
}

Will not emit "my counter" with "group=coolgroup".

To that the user will need to wrap the inner parts of the then in a group as in

import { Counter } from "k6/metrics";
import { group } from "k6";
const delay = () => { return Promise.resolve(); }
let l = new Counter("my coutner")

export default () => {
    group("coolgroup", () => {
        l.add(1) // nice

        delay(1).then(() => {
            group("coolgroup", () => {
                l.add(1)// not so nice
            });
        })
    })
}

This is because group is kind of

import exec from "k6/execution"
function group (name, fn) {
  let oldGroup = exec.vu.tags["group"];
  exec.vu.tags["group"] = oldGroup + ":" + name;
  try {
    fn() // the actual inner function
  } finally {
    exec.vu.tags["group"] = oldGroup;
  }
}

As the then "callbacks" get called only after the stack is empty the whole group code would have been executed, resetting the group back to the root name (which is empty). And when the then get's called it will have an empty group.

The "workaround" works as it just calls group again.

A more hacking but ... more useful workaround is

import { Counter } from "k6/metrics";
import { group } from "k6";
const delay = () => { return Promise.resolve(); }
let l = new Counter("my coutner")

let originalThen = Promise.prototype.then;
Promise.prototype.then = function(onResolved, onRejected) {
    let o = onResolved
    onResolved = function(res) {
        group("coolgroup", () => { // this in practice will need to get hte group name
            o(res);
        })
    }
    // https://stackoverflow.com/questions/55413715/cant-i-overwrite-the-then-function-of-promise
    return originalThen.call(originalThen.call(this, onResolved, onRejected), undefined, error => {
        console.log(`upload the error to server: ${error}`);
        throw error;
    });
}

export default () => {
    group("coolgroup", () => {
        l.add(1) // nice

        delay(1).then(() => {
            l.add(1)// not so nice
        })
    })
}

This ... likely has other problems.

Solutions:

I would argue this might be just showing that group is a bad abstraction and should be deprecated. It makes little sense in a lot of the cases.

On the other hand people are using it (albeit sometimes wrongly) and likely will still want to use it once k6 has a lot more async APIs (which return promises).

Some of those can be fixed in the actual go code - it can get the group before returning the promise. Which in practice is needed as getting the group in a goroutine while js code is runnign si racy as group() call can be made, changing it, while the goroutine gets it.

This in no way helps though with later .then calls.

So for a more real example

delay(1).then(() => {
  return anotherPromise();
}).then((resultFromAnotherPromise)=>{
  return morepromises();
}.then((.....

for all of those setting group will currently not work great.

async/await support:

While we might be able to fix some of this for case wher group("name" , async function () {...}) through just doing some magic in group - that will likely only work for the await usages, but not if you then ( 😉 ) call .then or something.

So it might work great for some of the "async" stuff but not for the other.

pure callbacks

If somebody has an API that just have callbacks ... having them support group will be on a case by case callback basis. They will need to make certain to get the group from where the callback is defined and then when making it to set the group ... and then unset it.

API changes

I guess it is possible for the js/eventloop to provide API to ...that, but that will likely be quite involved, but might be the easiest way forward.

[oleiade]

First and foremost, thanks for putting such a detailed analysis and explanation of the problem together, much appreciated 👍🏻 🙇🏻

I can see the issue clearly too now, and I kind of come to the same conclusion as you: there doesn't seem to be any elegant solution to this. This leads me to believe that any solution we pick would end up turning into some complicated legacy that we would likely regret.

Disclaimer ⚠️ Much of the changes I can imagine to even remotely attempt at addressing this issue would end up being breaking changes; that is, changing the current look and feel of group.

Thus here are a bunch of notes and questions that the issue raised for me. I don't expect those to bring solutions, but maybe they open the discussion in ways that helps us move towards an acceptable solution somehow:

1. I perceive the group API as behaving/looking like syntactic sugar. If we didn't have group, or deprecated it what would be alternative ways of achieving the same functionality? Besides setting tags manually through the exec module. Do you have ideas already on that front?
2. As I understand it, the key aspect that leads us to this issue is that the content of the group is wrapped in a single big callback, and from there on we lose granular control as to what happens inside. As is, there seems to be no way to enforce some context (tags) to nested calls (in this case promises to resolve). Ideally, I imagine we would want to implicitly wrap the subsequent calls inside of a group callback with some code that tags but I can't see an elegant way to do that either.
3. This is more of an intuition than an idea, but it kept floating around my head: could somehow k6 keep track of promises and their spot in the script's call stack, say as a tree, and at the time of processing the callbacks of the event loop, it would establish parenthood (I, as a promise, was made in the context of a group call, and thus I should execute function X (which would tag me accordingly as a side effect?))
   
4. Something I realized is that I wouldn't consider a purely async/await based solution satisfactory either. With the syntax being syntactic sugar for "put this current thread of execution on pause, do something else, and come back to me when the promise is resolved", I would expect whatever solution we reach to still work with "only/pure" promises, and without the specific syntax.
5. The Promise API has a bunch of nice helpers around working with operations over collections of Promises. I wonder if there might be some inspiration to take from there. (I really don't have a clear idea, but maybe that inspires someone who reads that?) Thinking about the topic, I kept getting back to "can we somehow transform or shift the problem in a way that leads us to a position where we have a flattened sequence or collection of promises we can then iterate/walk and act upon?
6. In general I can see how we could implement specific solutions, directly in the js/eventloop, but my initial gut feeling would be to try to avoid it as much as possible. That sounds like the best way to add some obscure, hard to maintain, implementation detail, deep in the tech stack, and although I'm fine with introducing debt when it's unavoidable, this solution would be my least favorite.

Finally, I don't think you've mentioned if you had any preferences toward one or the other solution yet; if you do, I'd love to hear it.

I hope this melting pot of questions, intuitions, and ideas 💡 turns out somehow useful, and maybe sparks new ideas and approaches. Regardless, I'll keep thinking about it and will come back with anything else that might pop up 🙇🏻

[mstoykov]

would end up turning into some complicated legacy that we would likely regret.

This is mostly why I am leaning to in practice ... deprecating group() or at least not doing anything to make it more usable with async calls.

1. I would expect that the workaround for async calls will be to set the tags each time it is needed manually. To me it seems like it is possible to use some JS API wrappers on a case by case basis, but having them in general will mean that all code needs to take this into account. Or we need to bot change Promise.prototype.then and the code that queues stuff on the event loop. (as the first doesn't use the k6 event loop but is part of goja).

My other problem here is that in a lot of the cases I would argue group adds more or less no value. And people do seem to expect it to do stuff ... it doesn't. For example #880 and #921 were because people expected that group_duration will be just the time it takes to do the requests. As k6 gets more and more features, there will arguably be more and more stuff that users will expect are automatically added or not to different metrics. I would argue most of those need to be decided on a case by case basis by the user.

This also remind me that ... currently group() will emit the metric group_duration as soon as the internal call ends, but this changes will also need to make it wait until the last async call started from within is done. Whatever that means. This likely is just as hairy as the other problems. This also begs the question - should the time be from when group was started until the last thing ended or somehow measure how long things inside it ... take, but not take into account the time it was waiting for the event loop to be free (for example).

An example

group("my cool group", ()=> {
  http.asyncRequest("GET", login_url).then((resp)=>{
    return http.asyncRequest("POST", add_to_cart)
  }).then((resp)=>{
    return http.asyncRequest("GET", goto_cart)
  }).then((resp)=>{
    return http.asyncRequest("POST", pay_url)
  })
})
sleep(10s)// or something else that takes 10s

Should the above group_duration be 10s longer? Or not take that into consideration. How do we then measure what happens off the main thread (so not in the JS executin time) and do we need to do this for each operation? Some kind of "AddToGroupTime?" API maybe.

Since grafana/k6-docs#210 I have been pretty sure most of the uses of group() by k6 users have been ... not very useful.

On the other hand I have seen users use it nicely and I do agree there is value in it. I am just not certain that can't be done in some other way.

Looking at the group + checks high coupling I feel like the correct approach here will be to make it possible to write better check() and group() in pure JS and then maybe adopt one of those at a later point.

Like the Group is not just a metric it is this whole struct and all the things on it and it has a bunch of things directly related to Checks

k6/lib/models.go

Lines 72 to 100 in 0b73cd6

	// A Group is an organisational block, that samples and checks may be tagged with.
	//
	// For more information, refer to the js/modules/k6.K6.Group() function.
	type Group struct {
	// Arbitrary name of the group.
	Name string `json:"name"`
	// A group may belong to another group, which may belong to another group, etc. The Path
	// describes the hierarchy leading down to this group, with the segments delimited by '::'.
	// As an example: a group "Inner" inside a group named "Outer" would have a path of
	// "::Outer::Inner". The empty first item is the root group, which is always named "".
	Parent *Group `json:"-"`
	Path string `json:"path"`
	// A group's ID is a hash of the Path. It is deterministic between different k6
	// instances of the same version, but should be treated as opaque - the hash function
	// or length may change.
	ID string `json:"id"`
	// Groups and checks that are children of this group.
	Groups map[string]*Group `json:"groups"`
	OrderedGroups []*Group `json:"-"`
	Checks map[string]*Check `json:"checks"`
	OrderedChecks []*Check `json:"-"`
	groupMutex sync.Mutex
	checkMutex sync.Mutex
	}

I think I went enough on this point

2. As I understand it, the key aspect that leads us to this issue is that the content of the group is wrapped in a single big callback, and from there on we lose granular control as to what happens inside. As is, there seems to be no way to enforce some context (tags) to nested calls (in this case promises to resolve). Ideally, I imagine we would want to implicitly wrap the subsequent calls inside of a group callback with some code that tags but I can't see an elegant way to do that either.

what happens inside

The thing is that it doesn't happen "inside" it is asynchronous. The whole thing with async calls is that you call them and the program continues running and later at some point you will get your result. In our case we then do somethign with that result and the expectation (from purely visual reading of the code) is that this something will be in the same group.

group("my group", () => {
  doSomething(); // this happens *inside* the group
  asyncCall( // this also happens *inside* the group
    () => {
        someCallHere(); // this happesn *after* the group() has ended
    }
  )
  hereSomething(); // this happesn before someCallHere()
})
moreCode()// someCallHere() happens aftrer this line is executed where we are now *not* inside the group() obviously.

So the issues is basically that due to callbacks (whether from promises or not) are not executed within the group, they act the same, instead of how users will likely expect.

Which is IMO where we should go and just - let users know that group doesn't work with asynchronous calls (or at least not great ;) ).

I guess this means that we need a way to make some kind of similar functionality for async calls 🤷‍♂

3. This is more of an intuition than an idea, but it kept floating around my head: could somehow k6 keep track of promises and their spot in the script's call stack, say as a tree, and at the time of processing the callbacks of the event loop, it would establish parenthood (I, as a promise, was made in the context of a group call, and thus I should execute function X (which would tag me accordingly as a side effect?))

This both needs to work for non promises as well and k6 does not get any notification that a promise was created inside a group. We can probably hack something around Promise.prototype again or even request/make a change to goja.

I guess this also again will mean that the group() will not end until all of those calls end.

4. Something I realized is that I wouldn't consider a purely async/await based solution satisfactory either. With the syntax being syntactic sugar for "put this current thread of execution on pause, do something else, and come back to me when the promise is resolved", I would expect whatever solution we reach to still work with "only/pure" promises, and without the specific syntax.

I am not certain I got what you are saying, but this likely needs to work with non promises as well. See Websocket API example it uses an event API with pure callbacks (that get more than once, so they are called "handlers"). But should if those are defined within a group for them to always be tagged with the group? Does that mean that a group will wait until the websocket is done? How do we know this is has happened for other APIs. Aka what should extension developers do to make this work.

const socket = group("somegroup", () => {
  return new WebSocket('ws://localhost:8080');
});

group("open group", () => {
  // Connection opened
  socket.addEventListener('open', (event) => {
     socket.send('Hello Server!'); // what does this gets tagged with?
  });
});
group("message group", () => {
  // Listen for messages
  socket.addEventListener('message', (event) => {
    console.log('Message from server ', event.data);
  });
});

5. The Promise API has a bunch of nice helpers around working with operations over collections of Promises. I wonder if there might be some inspiration to take from there. (I really don't have a clear idea, but maybe that inspires someone who reads that?) Thinking about the topic, I kept getting back to "can we somehow transform or shift the problem in a way that leads us to a position where we have a flattened sequence or collection of promises we can then iterate/walk and act upon?

The only thing that I can think of is that if group() itself was async function then maybe we can wait for it to finish. But that will be a breaking change.

And that likely can be a new API. But I am not really certain if this is going to be a thing we can either way - so maybe that will be more involved. Also, arguably it is blocked on goja having async functions which currently isn't the case.

6. In general I can see how we could implement specific solutions, directly in the js/eventloop, but my initial gut feeling would be to try to avoid it as much as possible. That sounds like the best way to add some obscure, hard to maintain, implementation detail, deep in the tech stack, and although I'm fine with introducing debt when it's unavoidable, this solution would be my least favorite.

This is also my feeling.

For example if we "record" the group in RegisterCallback (or more likely the parent call on modules.VU) but that only works because RegisterCallback is now the only way to get a Callback. But if we implement #2544 it might be that most code will not use it, and it will be a lot harder to figure out how to "record" the group.

Finally, I don't think you've mentioned if you had any preferences toward one or the other solution yet; if you do, I'd love to hear it.

I guess my proposal at this point is to heavily discourage usage of group with async and try to refactor the internals and expose them in some way ... so something like startGroup, and then endGroup function .... later.

But I guess that means that with such an API anything in between will be part of the group. SO maybe even pauseGroup and resumeGroup.

I guess it will be nice if we could see some kind of other API that does this in the wild ...

[mstoykov]

edit: the below is no longer true see #2728 (comment)

After a lot of internal discussions in which @sniku was instrumental it was decided that:

1. we would support async functions in group and will want to go with async support across the API
2. to fix the above outlined problems it was decided that if a user users await before and after that call we want the same group to be tagged.
3. "Obviously" the group duration also needs to include finishing all of those calls.
4. If you just start something that isn't awaited inside the group it is not going to be tagged with a group.

example with the API from #2825

group("something", async () =>  {
   await asyncRequest("GET", url) // this will be tagged with the group "something"
   await asyncRequest("GET", url) // so will this one
   asyncRequest("GET", url).then((resp) => {
     // do something with resp
     asyncRequest("GET", url) // this will **NOT** be  tagged with a group
   })
   asyncRequest("GET", url) // this will again be tagged with the group "something"
})```

Also #2863 represents another decision made later that `group` will not call `async` functions directly, but will be called the next time a Promise job will be executed. This makes it more alike other `async` taking APIs across the js ecosystem and was what most users expected 

[mstoykov]

After even more discussion it was decided to not support async functions in group and check at this time and actively return exception to prevent anyone from misusing them.

The magical solution of only supporting async/await has both proven to have:

• a lot of corner cases
• not handled promise.then chains
• any other kind of not promise based callback API such as the experimental websocket
  make this solution inconsistent and likely to be confusing when and for what it will tag correctly and or will wait for.

It is also kind of hard to define what a group of request needs to be with async APIs unless you specify per each call.

Given also the fact k6 has very low amount of actually async APIs at this point and the brand new async/await syntax the k6 OSS team thinks its better to explore other possibilities.

For the next release cycle (it is too late for this one unfortunately) we will try to make the group and check API less magical. In practice making it possible for simple JS code to have the same UX as the current special API. This will also enable users and us to experiment more easily with possible APIs.