Refactor group and check to not be tightly coupled (#3750)

* Refactor group and check to not be tightly coupled

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

Co-authored-by: Joan López de la Franca Beltran <5459617+joanlopez@users.noreply.github.com>

api/v1/group_routes.go

@@ -6,7 +6,7 @@ import (
 )
 
 func handleGetGroups(cs *ControlSurface, rw http.ResponseWriter, _ *http.Request) {
-	root := NewGroup(cs.RunState.Runner.GetDefaultGroup(), nil)
+	root := NewGroup(cs.RunState.GroupSummary.Group(), nil)
 	groups := FlattenGroup(root)
 
 	data, err := json.Marshal(newGroupsJSONAPI(groups))
@@ -18,7 +18,7 @@ func handleGetGroups(cs *ControlSurface, rw http.ResponseWriter, _ *http.Request
 }
 
 func handleGetGroup(cs *ControlSurface, rw http.ResponseWriter, _ *http.Request, id string) {
-	root := NewGroup(cs.RunState.Runner.GetDefaultGroup(), nil)
+	root := NewGroup(cs.RunState.GroupSummary.Group(), nil)
 	groups := FlattenGroup(root)
 
 	var group *Group

api/v1/group_routes_test.go

@@ -37,6 +37,7 @@ func getTestRunState(tb testing.TB, options lib.Options, runner lib.Runner) *lib
 		TestPreInitState: piState,
 		Options:          options,
 		Runner:           runner,
+		GroupSummary:     lib.NewGroupSummary(piState.Logger),
 		RunTags:          piState.Registry.RootTagSet().WithTagsFromMap(options.RunTags),
 	}
 }
@@ -64,15 +65,37 @@ func getControlSurface(tb testing.TB, testState *lib.TestRunState) *ControlSurfa
 func TestGetGroups(t *testing.T) {
 	t.Parallel()
 
+	cs := getControlSurface(t, getTestRunState(t, lib.Options{}, &minirunner.MiniRunner{}))
+	require.NoError(t, cs.RunState.GroupSummary.Start())
+	cs.RunState.GroupSummary.AddMetricSamples([]metrics.SampleContainer{
+		metrics.Sample{
+			TimeSeries: metrics.TimeSeries{
+				Metric: cs.RunState.BuiltinMetrics.GroupDuration,
+				Tags:   cs.RunState.Registry.RootTagSet().With("group", "::group 1"),
+			},
+		},
+		metrics.Sample{
+			TimeSeries: metrics.TimeSeries{
+				Metric: cs.RunState.BuiltinMetrics.GroupDuration,
+				Tags:   cs.RunState.Registry.RootTagSet().With("group", ""),
+			},
+		},
+		metrics.Sample{
+			TimeSeries: metrics.TimeSeries{
+				Metric: cs.RunState.BuiltinMetrics.GroupDuration,
+				Tags:   cs.RunState.Registry.RootTagSet().With("group", "::group 1::group 2"),
+			},
+		},
+	})
+	require.NoError(t, cs.RunState.GroupSummary.Stop())
+
 	g0, err := lib.NewGroup("", nil)
 	assert.NoError(t, err)
 	g1, err := g0.Group("group 1")
 	assert.NoError(t, err)
 	g2, err := g1.Group("group 2")
 	assert.NoError(t, err)
 
-	cs := getControlSurface(t, getTestRunState(t, lib.Options{}, &minirunner.MiniRunner{Group: g0}))
-
 	t.Run("list", func(t *testing.T) {
 		t.Parallel()
 

cmd/run.go

@@ -165,6 +165,8 @@ func (c *cmdRun) run(cmd *cobra.Command, args []string) (err error) {
 		return err
 	}
 
+	outputs = append(outputs, testRunState.GroupSummary)
+
 	metricsEngine, err := engine.NewMetricsEngine(testRunState.Registry, logger)
 	if err != nil {
 		return err
@@ -192,7 +194,7 @@ func (c *cmdRun) run(cmd *cobra.Command, args []string) (err error) {
 			logger.Debug("Generating the end-of-test summary...")
 			summaryResult, hsErr := test.initRunner.HandleSummary(globalCtx, &lib.Summary{
 				Metrics:         metricsEngine.ObservedMetrics,
-				RootGroup:       testRunState.Runner.GetDefaultGroup(),
+				RootGroup:       testRunState.GroupSummary.Group(),
 				TestRunDuration: executionState.GetCurrentTestRunDuration(),
 				NoColor:         c.gs.Flags.NoColor,
 				UIState: lib.UIState{

cmd/test_load.go

@@ -277,6 +277,7 @@ func (lct *loadedAndConfiguredTest) buildTestRunState(
 		Runner:           lct.initRunner,
 		Options:          lct.derivedConfig.Options, // we will always run with the derived options
 		RunTags:          lct.preInitState.Registry.RootTagSet().WithTagsFromMap(configToReinject.RunTags),
+		GroupSummary:     lib.NewGroupSummary(lct.preInitState.Logger),
 	}, nil
 }
 

cmd/ui.go

@@ -114,7 +114,8 @@ func printExecutionDescription(
 	default:
 		for _, out := range outputs {
 			desc := out.Description()
-			if desc == engine.IngesterDescription {
+			switch desc {
+			case engine.IngesterDescription, lib.GroupSummaryDescription:
 				continue
 			}
 			if strings.HasPrefix(desc, dashboard.OutputName) {

js/initcontext_test.go

@@ -342,9 +342,6 @@ func TestRequestWithBinaryFile(t *testing.T) {
 	bi, err := b.Instantiate(context.Background(), 0)
 	require.NoError(t, err)
 
-	root, err := lib.NewGroup("", nil)
-	require.NoError(t, err)
-
 	logger := logrus.New()
 	logger.Level = logrus.DebugLevel
 	logger.Out = io.Discard
@@ -354,7 +351,6 @@ func TestRequestWithBinaryFile(t *testing.T) {
 	bi.moduleVUImpl.state = &lib.State{
 		Options: lib.Options{},
 		Logger:  logger,
-		Group:   root,
 		Transport: &http.Transport{
 			DialContext: (netext.NewDialer(
 				net.Dialer{
@@ -488,9 +484,6 @@ func TestRequestWithMultipleBinaryFiles(t *testing.T) {
 	bi, err := b.Instantiate(context.Background(), 0)
 	require.NoError(t, err)
 
-	root, err := lib.NewGroup("", nil)
-	require.NoError(t, err)
-
 	logger := logrus.New()
 	logger.Level = logrus.DebugLevel
 	logger.Out = io.Discard
@@ -500,7 +493,6 @@ func TestRequestWithMultipleBinaryFiles(t *testing.T) {
 	bi.moduleVUImpl.state = &lib.State{
 		Options: lib.Options{},
 		Logger:  logger,
-		Group:   root,
 		Transport: &http.Transport{
 			DialContext: (netext.NewDialer(
 				net.Dialer{

js/modules/k6/grpc/params_test.go

@@ -148,15 +148,12 @@ func newParamsTestRuntime(t *testing.T, paramsJSON string) (*modulestest.Runtime
 
 	testRuntime := modulestest.NewRuntime(t)
 	registry := metrics.NewRegistry()
-	root, err := lib.NewGroup("", nil)
-	require.NoError(t, err)
 
 	logger := logrus.New()
 	logger.SetLevel(logrus.InfoLevel)
 	logger.Out = io.Discard
 
 	state := &lib.State{
-		Group: root,
 		Options: lib.Options{
 			SystemTags: metrics.NewSystemTagSet(
 				metrics.TagName,
@@ -171,7 +168,7 @@ func newParamsTestRuntime(t *testing.T, paramsJSON string) (*modulestest.Runtime
 
 	testRuntime.MoveToVUContext(state)
 
-	_, err = testRuntime.VU.Runtime().RunString(`let params = ` + paramsJSON + `;`)
+	_, err := testRuntime.VU.Runtime().RunString(`let params = ` + paramsJSON + `;`)
 	require.NoError(t, err)
 
 	params := testRuntime.VU.Runtime().Get("params")

js/modules/k6/grpc/teststate_test.go

@@ -142,13 +142,8 @@ func newTestState(t *testing.T) testState {
 // ToVUContext moves the test state to the VU context.
 func (ts *testState) ToVUContext() {
 	registry := metrics.NewRegistry()
-	root, err := lib.NewGroup("", nil)
-	if err != nil {
-		panic(err)
-	}
 
 	state := &lib.State{
-		Group:     root,
 		Dialer:    ts.httpBin.Dialer,
 		TLSConfig: ts.httpBin.TLSClientConfig,
 		Samples:   ts.samples,

js/modules/k6/http/request_test.go

@@ -128,8 +128,6 @@ type httpTestCase struct {
 func newTestCase(t testing.TB) *httpTestCase {
 	tb := httpmultibin.NewHTTPMultiBin(t)
 
-	root, err := lib.NewGroup("", nil)
-	require.NoError(t, err)
 	registry := metrics.NewRegistry()
 
 	logger := logrus.New()
@@ -152,13 +150,12 @@ func newTestCase(t testing.TB) *httpTestCase {
 	state := &lib.State{
 		Options:    options,
 		Logger:     logger,
-		Group:      root,
 		TLSConfig:  tb.TLSClientConfig,
 		Transport:  tb.HTTPTransport,
 		BufferPool: lib.NewBufferPool(),
 		Samples:    samples,
 		Tags: lib.NewVUStateTags(registry.RootTagSet().WithTagsFromMap(map[string]string{
-			"group": root.Path,
+			"group": lib.RootGroupPath,
 		})),
 		BuiltinMetrics: metrics.RegisterBuiltinMetrics(registry),
 	}

js/modules/k6/http/response_test.go

@@ -9,6 +9,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"go.k6.io/k6/lib"
 	"go.k6.io/k6/metrics"
 )
 
@@ -109,7 +110,6 @@ func TestResponse(t *testing.T) {
 	samples := ts.samples
 	rt := ts.runtime.VU.Runtime()
 	state := ts.runtime.VU.State()
-	root := state.Group
 	sr := tb.Replacer.Replace
 
 	tb.Mux.HandleFunc("/myforms/get", myFormHandler)
@@ -147,17 +147,14 @@ func TestResponse(t *testing.T) {
 		})
 
 		t.Run("group", func(t *testing.T) {
-			g, err := root.Group("my group")
+			groupName, err := lib.NewGroupPath(lib.RootGroupPath, "my group")
 			require.NoError(t, err)
-			old := state.Group
-			state.Group = g
 			state.Tags.Modify(func(tagsAndMeta *metrics.TagsAndMeta) {
-				tagsAndMeta.SetTag("group", g.Path)
+				tagsAndMeta.SetTag("group", groupName)
 			})
 			defer func() {
-				state.Group = old
 				state.Tags.Modify(func(tagsAndMeta *metrics.TagsAndMeta) {
-					tagsAndMeta.SetTag("group", old.Path)
+					tagsAndMeta.SetTag("group", "")
 				})
 			}()
 

js/modules/k6/k6.go

@@ -5,13 +5,14 @@ import (
 	"errors"
 	"fmt"
 	"math/rand"
-	"sync/atomic"
+	"strings"
 	"time"
 
 	"github.com/dop251/goja"
 
 	"go.k6.io/k6/js/common"
 	"go.k6.io/k6/js/modules"
+	"go.k6.io/k6/lib"
 	"go.k6.io/k6/metrics"
 )
 
@@ -105,25 +106,23 @@ func (mi *K6) Group(name string, val goja.Value) (goja.Value, error) {
 	if common.IsAsyncFunction(mi.vu.Runtime(), val) {
 		return goja.Undefined(), fmt.Errorf(asyncFunctionNotSupportedMsg, "group")
 	}
-	g, err := state.Group.Group(name)
+	oldGroupName, _ := state.Tags.GetCurrentValues().Tags.Get(metrics.TagGroup.String())
+	// TODO: what are we doing if group is not tagged
+	newGroupName, err := lib.NewGroupPath(oldGroupName, name)
 	if err != nil {
 		return goja.Undefined(), err
 	}
 
-	old := state.Group
-	state.Group = g
-
 	shouldUpdateTag := state.Options.SystemTags.Has(metrics.TagGroup)
 	if shouldUpdateTag {
 		state.Tags.Modify(func(tagsAndMeta *metrics.TagsAndMeta) {
-			tagsAndMeta.SetSystemTagOrMeta(metrics.TagGroup, g.Path)
+			tagsAndMeta.SetSystemTagOrMeta(metrics.TagGroup, newGroupName)
 		})
 	}
 	defer func() {
-		state.Group = old
 		if shouldUpdateTag {
 			state.Tags.Modify(func(tagsAndMeta *metrics.TagsAndMeta) {
-				tagsAndMeta.SetSystemTagOrMeta(metrics.TagGroup, old.Path)
+				tagsAndMeta.SetSystemTagOrMeta(metrics.TagGroup, oldGroupName)
 			})
 		}
 	}()
@@ -148,8 +147,6 @@ func (mi *K6) Group(name string, val goja.Value) (goja.Value, error) {
 }
 
 // Check will emit check metrics for the provided checks.
-//
-//nolint:funlen
 func (mi *K6) Check(arg0, checks goja.Value, extras ...goja.Value) (bool, error) {
 	state := mi.vu.State()
 	if state == nil {
@@ -174,17 +171,14 @@ func (mi *K6) Check(arg0, checks goja.Value, extras ...goja.Value) (bool, error)
 	var exc error
 	obj := checks.ToObject(rt)
 	for _, name := range obj.Keys() {
-		val := obj.Get(name)
-
-		// Resolve the check record.
-		check, err := state.Group.Check(name)
-		if err != nil {
-			return false, err
+		if strings.Contains(name, lib.GroupSeparator) {
+			return false, lib.ErrNameContainsGroupSeparator
 		}
+		val := obj.Get(name)
 
 		tags := commonTagsAndMeta.Tags
 		if state.Options.SystemTags.Has(metrics.TagCheck) {
-			tags = tags.With("check", check.Name)
+			tags = tags.With("check", name)
 		}
 
 		if common.IsAsyncFunction(rt, val) {
@@ -207,28 +201,19 @@ func (mi *K6) Check(arg0, checks goja.Value, extras ...goja.Value) (bool, error)
 			succ = false
 		}
 
-		// Emit! (But only if we have a valid context.)
-		select {
-		case <-ctx.Done():
-		default:
-			sample := metrics.Sample{
-				TimeSeries: metrics.TimeSeries{
-					Metric: state.BuiltinMetrics.Checks,
-					Tags:   tags,
-				},
-				Time:     t,
-				Metadata: commonTagsAndMeta.Metadata,
-				Value:    0,
-			}
-			if booleanVal {
-				atomic.AddInt64(&check.Passes, 1)
-				sample.Value = 1
-			} else {
-				atomic.AddInt64(&check.Fails, 1)
-			}
-
-			metrics.PushIfNotDone(ctx, state.Samples, sample)
+		sample := metrics.Sample{
+			TimeSeries: metrics.TimeSeries{
+				Metric: state.BuiltinMetrics.Checks,
+				Tags:   tags,
+			},
+			Time:     t,
+			Metadata: commonTagsAndMeta.Metadata,
 		}
+		if booleanVal {
+			sample.Value = 1
+		}
+
+		metrics.PushIfNotDone(ctx, state.Samples, sample)
 
 		if exc != nil {
 			return false, exc