From ebbf64c9150f46d4bd2eb27afd60a358342297b4 Mon Sep 17 00:00:00 2001 From: Mihail Stoykov Date: Tue, 21 May 2024 18:26:30 +0300 Subject: [PATCH 1/4] 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 --- api/v1/group_routes.go | 4 +- api/v1/group_routes_test.go | 27 +++++- cmd/run.go | 4 +- cmd/test_load.go | 1 + cmd/ui.go | 3 +- js/initcontext_test.go | 8 -- js/modules/k6/grpc/params_test.go | 5 +- js/modules/k6/grpc/teststate_test.go | 5 -- js/modules/k6/http/request_test.go | 5 +- js/modules/k6/http/response_test.go | 11 +-- js/modules/k6/k6.go | 61 +++++-------- js/modules/k6/k6_test.go | 34 +------- js/modules/k6/ws/ws_test.go | 3 - js/runner.go | 22 +---- js/runner_test.go | 91 -------------------- lib/models.go | 28 ++++-- lib/runner.go | 5 -- lib/test_state.go | 114 +++++++++++++++++++++++++ lib/testutils/minirunner/minirunner.go | 9 -- lib/vu_state.go | 3 - 20 files changed, 203 insertions(+), 240 deletions(-) diff --git a/api/v1/group_routes.go b/api/v1/group_routes.go index d4c9f1773e4..ee25b4ba09b 100644 --- a/api/v1/group_routes.go +++ b/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 diff --git a/api/v1/group_routes_test.go b/api/v1/group_routes_test.go index 69fec5dd25a..92d8b1b4502 100644 --- a/api/v1/group_routes_test.go +++ b/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,6 +65,30 @@ 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{})) + _ = 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"), + }, + }, + }) + _ = cs.RunState.GroupSummary.Stop() + g0, err := lib.NewGroup("", nil) assert.NoError(t, err) g1, err := g0.Group("group 1") @@ -71,8 +96,6 @@ func TestGetGroups(t *testing.T) { 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() diff --git a/cmd/run.go b/cmd/run.go index ee64c6f54a7..28654f71166 100644 --- a/cmd/run.go +++ b/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{ diff --git a/cmd/test_load.go b/cmd/test_load.go index b7017c02d18..89f5bed04f0 100644 --- a/cmd/test_load.go +++ b/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 } diff --git a/cmd/ui.go b/cmd/ui.go index ce924788176..a231e4a3554 100644 --- a/cmd/ui.go +++ b/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) { diff --git a/js/initcontext_test.go b/js/initcontext_test.go index 871f0c393c2..f89ed46b27e 100644 --- a/js/initcontext_test.go +++ b/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{ diff --git a/js/modules/k6/grpc/params_test.go b/js/modules/k6/grpc/params_test.go index c43c6d91fa2..7d3d8eb378e 100644 --- a/js/modules/k6/grpc/params_test.go +++ b/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") diff --git a/js/modules/k6/grpc/teststate_test.go b/js/modules/k6/grpc/teststate_test.go index 0340e9391e8..24ba46cfef6 100644 --- a/js/modules/k6/grpc/teststate_test.go +++ b/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, diff --git a/js/modules/k6/http/request_test.go b/js/modules/k6/http/request_test.go index 400c9617009..d9da44a10a2 100644 --- a/js/modules/k6/http/request_test.go +++ b/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), } diff --git a/js/modules/k6/http/response_test.go b/js/modules/k6/http/response_test.go index 440387b678b..e28128c04f2 100644 --- a/js/modules/k6/http/response_test.go +++ b/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", "") }) }() diff --git a/js/modules/k6/k6.go b/js/modules/k6/k6.go index f5bb139520c..a166d6b8631 100644 --- a/js/modules/k6/k6.go +++ b/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 diff --git a/js/modules/k6/k6_test.go b/js/modules/k6/k6_test.go index 74432901088..dda4805adeb 100644 --- a/js/modules/k6/k6_test.go +++ b/js/modules/k6/k6_test.go @@ -92,20 +92,16 @@ func TestGroup(t *testing.T) { t.Parallel() tc := testCaseRuntime(t) state := tc.testRuntime.VU.State() - root := state.Group require.NoError(t, tc.testRuntime.VU.Runtime().Set("fn", func() { groupTag, ok := state.Tags.GetCurrentValues().Tags.Get("group") require.True(t, ok) assert.Equal(t, groupTag, "::my group") - assert.Equal(t, state.Group.Name, "my group") - assert.Equal(t, state.Group.Parent, root) })) _, err := tc.testRuntime.RunOnEventLoop(`k6.group("my group", fn)`) assert.NoError(t, err) - assert.Equal(t, state.Group, root) groupTag, ok := state.Tags.GetCurrentValues().Tags.Get("group") require.True(t, ok) - assert.Equal(t, groupTag, root.Name) + assert.Equal(t, groupTag, "") }) t.Run("Invalid", func(t *testing.T) { @@ -349,29 +345,6 @@ func TestCheckTypes(t *testing.T) { } } -func TestCheckContextExpiry(t *testing.T) { - t.Parallel() - - tc := testCaseRuntime(t) - - v, err := tc.testRuntime.RunOnEventLoop(`value = k6.check(null, { "check": true })`) - require.NoError(t, err) - assert.Equal(t, true, v.Export()) - - check, _ := tc.testRuntime.VU.State().Group.Check("check") - assert.Equal(t, int64(1), check.Passes) - assert.Equal(t, int64(0), check.Fails) - - tc.testRuntime.CancelContext() - - v, err = tc.testRuntime.RunOnEventLoop(`k6.check(null, { "check": true })`) - require.NoError(t, err) - assert.Equal(t, true, v.Export()) - - assert.Equal(t, int64(1), check.Passes) - assert.Equal(t, int64(0), check.Fails) -} - func TestCheckTags(t *testing.T) { t.Parallel() tc := testCaseRuntime(t) @@ -408,16 +381,13 @@ func testCaseRuntime(t testing.TB) *testCase { require.NoError(t, testRuntime.VU.RuntimeField.Set("k6", m.Exports().Named)) registry := metrics.NewRegistry() - root, err := lib.NewGroup("", nil) - assert.NoError(t, err) samples := make(chan metrics.SampleContainer, 1000) state := &lib.State{ - Group: root, Options: lib.Options{ SystemTags: &metrics.DefaultSystemTagSet, }, Samples: samples, - Tags: lib.NewVUStateTags(registry.RootTagSet().WithTagsFromMap(map[string]string{"group": root.Path})), + Tags: lib.NewVUStateTags(registry.RootTagSet().WithTagsFromMap(map[string]string{"group": lib.RootGroupPath})), BuiltinMetrics: metrics.RegisterBuiltinMetrics(registry), } testRuntime.MoveToVUContext(state) diff --git a/js/modules/k6/ws/ws_test.go b/js/modules/k6/ws/ws_test.go index 7239b2ee17c..c81425be758 100644 --- a/js/modules/k6/ws/ws_test.go +++ b/js/modules/k6/ws/ws_test.go @@ -85,11 +85,8 @@ func newTestState(t testing.TB) testState { testRuntime := modulestest.NewRuntime(t) samples := make(chan metrics.SampleContainer, 1000) - root, err := lib.NewGroup("", nil) - require.NoError(t, err) registry := metrics.NewRegistry() state := &lib.State{ - Group: root, Dialer: tb.Dialer, Options: lib.Options{ SystemTags: metrics.NewSystemTagSet( diff --git a/js/runner.go b/js/runner.go index aae07f7a9ed..70316e47e92 100644 --- a/js/runner.go +++ b/js/runner.go @@ -48,7 +48,6 @@ var nameToCertWarning sync.Once type Runner struct { Bundle *Bundle preInitState *lib.TestPreInitState - defaultGroup *lib.Group BaseDialer net.Dialer Resolver netext.Resolver @@ -84,16 +83,10 @@ func NewFromArchive(piState *lib.TestPreInitState, arc *lib.Archive) (*Runner, e // NewFromBundle returns a new Runner from the provided Bundle func NewFromBundle(piState *lib.TestPreInitState, b *Bundle) (*Runner, error) { - defaultGroup, err := lib.NewGroup("", nil) - if err != nil { - return nil, err - } - defDNS := types.DefaultDNSConfig() r := &Runner{ Bundle: b, preInitState: piState, - defaultGroup: defaultGroup, BaseDialer: net.Dialer{ Timeout: 30 * time.Second, KeepAlive: 30 * time.Second, @@ -105,7 +98,7 @@ func NewFromBundle(piState *lib.TestPreInitState, b *Bundle) (*Runner, error) { BufferPool: lib.NewBufferPool(), } - err = r.SetOptions(r.Bundle.Options) + err := r.SetOptions(r.Bundle.Options) return r, err } @@ -248,7 +241,6 @@ func (r *Runner) newVU( VUIDGlobal: vu.IDGlobal, Samples: vu.Samples, Tags: lib.NewVUStateTags(vu.Runner.RunTags), - Group: r.defaultGroup, BuiltinMetrics: r.preInitState.BuiltinMetrics, TracerProvider: r.preInitState.TracerProvider, } @@ -344,11 +336,6 @@ func (r *Runner) Teardown(ctx context.Context, out chan<- metrics.SampleContaine return err } -// GetDefaultGroup returns the default (root) Group. -func (r *Runner) GetDefaultGroup() *lib.Group { - return r.defaultGroup -} - // GetOptions returns the currently calculated [lib.Options] for the given Runner. func (r *Runner) GetOptions() lib.Options { return r.Bundle.Options @@ -559,17 +546,16 @@ func (r *Runner) runPart( }() vu.moduleVUImpl.ctx = ctx - group, err := r.GetDefaultGroup().Group(name) + groupPath, err := lib.NewGroupPath(lib.RootGroupPath, name) if err != nil { return goja.Undefined(), err } if r.Bundle.Options.SystemTags.Has(metrics.TagGroup) { vu.state.Tags.Modify(func(tagsAndMeta *metrics.TagsAndMeta) { - tagsAndMeta.SetSystemTagOrMeta(metrics.TagGroup, group.Path) + tagsAndMeta.SetSystemTagOrMeta(metrics.TagGroup, groupPath) }) } - vu.state.Group = group v, _, _, err := vu.runFn(ctx, false, fn, nil, vu.Runtime.ToValue(arg)) if deadlineError := r.checkDeadline(ctx, name, v, err); deadlineError != nil { @@ -692,7 +678,7 @@ func (u *VU) Activate(params *lib.VUActivationParams) lib.ActiveVU { if opts.SystemTags.Has(metrics.TagIter) { tagsAndMeta.SetSystemTagOrMeta(metrics.TagIter, strconv.FormatInt(u.iteration, 10)) } - tagsAndMeta.SetSystemTagOrMetaIfEnabled(opts.SystemTags, metrics.TagGroup, u.state.Group.Path) + tagsAndMeta.SetSystemTagOrMetaIfEnabled(opts.SystemTags, metrics.TagGroup, lib.RootGroupPath) tagsAndMeta.SetSystemTagOrMetaIfEnabled(opts.SystemTags, metrics.TagScenario, params.Scenario) }) diff --git a/js/runner_test.go b/js/runner_test.go index b491282a053..305dada40b5 100644 --- a/js/runner_test.go +++ b/js/runner_test.go @@ -89,24 +89,6 @@ func TestRunnerNew(t *testing.T) { }) } -func TestRunnerGetDefaultGroup(t *testing.T) { - t.Parallel() - r1, err := getSimpleRunner(t, "/script.js", `exports.default = function() {};`) - require.NoError(t, err) - assert.NotNil(t, r1.GetDefaultGroup()) - - registry := metrics.NewRegistry() - builtinMetrics := metrics.RegisterBuiltinMetrics(registry) - r2, err := NewFromArchive( - &lib.TestPreInitState{ - Logger: testutils.NewLogger(t), - BuiltinMetrics: builtinMetrics, - Registry: registry, - }, r1.MakeArchive()) - require.NoError(t, err) - assert.NotNil(t, r2.GetDefaultGroup()) -} - func TestRunnerOptions(t *testing.T) { t.Parallel() r1, err := getSimpleRunner(t, "/script.js", `exports.default = function() {};`) @@ -398,8 +380,6 @@ func TestDataIsolation(t *testing.T) { require.NoError(t, err) defer stopOutputs(nil) - require.Empty(t, runner.defaultGroup.Groups) - stopEmission, err := execScheduler.Init(runCtx, samples) require.NoError(t, err) @@ -416,8 +396,6 @@ func TestDataIsolation(t *testing.T) { require.NoError(t, err) waitForMetricsFlushed() } - require.Contains(t, runner.defaultGroup.Groups, "setup") - require.Contains(t, runner.defaultGroup.Groups, "teardown") var count int for _, s := range mockOutput.Samples { if s.Metric.Name == "mycounter" { @@ -672,7 +650,6 @@ func TestVURunContext(t *testing.T) { assert.Equal(t, null.IntFrom(10), state.Options.VUs) assert.Equal(t, null.BoolFrom(true), state.Options.Throw) assert.NotNil(t, state.Logger) - assert.Equal(t, r.GetDefaultGroup(), state.Group) assert.Equal(t, vu.Transport, state.Transport) })) @@ -774,74 +751,6 @@ func TestVURunInterruptDoesntPanic(t *testing.T) { } } -func TestVUIntegrationGroups(t *testing.T) { - t.Parallel() - r1, err := getSimpleRunner(t, "/script.js", ` - var group = require("k6").group; - exports.default = function() { - fnOuter(); - group("my group", function() { - fnInner(); - group("nested group", function() { - fnNested(); - }) - }); - } - `) - require.NoError(t, err) - - registry := metrics.NewRegistry() - builtinMetrics := metrics.RegisterBuiltinMetrics(registry) - r2, err := NewFromArchive( - &lib.TestPreInitState{ - Logger: testutils.NewLogger(t), - BuiltinMetrics: builtinMetrics, - Registry: registry, - }, r1.MakeArchive()) - require.NoError(t, err) - - testdata := map[string]*Runner{"Source": r1, "Archive": r2} - for name, r := range testdata { - r := r - t.Run(name, func(t *testing.T) { - t.Parallel() - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - vu, err := r.newVU(ctx, 1, 1, make(chan metrics.SampleContainer, 100)) - require.NoError(t, err) - - fnOuterCalled := false - fnInnerCalled := false - fnNestedCalled := false - require.NoError(t, vu.Runtime.Set("fnOuter", func() { - fnOuterCalled = true - assert.Equal(t, r.GetDefaultGroup(), vu.state.Group) - })) - require.NoError(t, vu.Runtime.Set("fnInner", func() { - fnInnerCalled = true - g := vu.state.Group - assert.Equal(t, "my group", g.Name) - assert.Equal(t, r.GetDefaultGroup(), g.Parent) - })) - require.NoError(t, vu.Runtime.Set("fnNested", func() { - fnNestedCalled = true - g := vu.state.Group - assert.Equal(t, "nested group", g.Name) - assert.Equal(t, "my group", g.Parent.Name) - assert.Equal(t, r.GetDefaultGroup(), g.Parent.Parent) - })) - - activeVU := vu.Activate(&lib.VUActivationParams{RunContext: ctx}) - err = activeVU.RunOnce() - require.NoError(t, err) - assert.True(t, fnOuterCalled, "fnOuter() not called") - assert.True(t, fnInnerCalled, "fnInner() not called") - assert.True(t, fnNestedCalled, "fnNested() not called") - }) - } -} - func TestVUIntegrationMetrics(t *testing.T) { t.Parallel() testdata := make(map[string]*Runner, 2) diff --git a/lib/models.go b/lib/models.go index aff29a70c43..b83a3433e39 100644 --- a/lib/models.go +++ b/lib/models.go @@ -18,8 +18,11 @@ import ( // GroupSeparator for group IDs. const GroupSeparator = "::" +// RootGroupPath is the id of the root group +const RootGroupPath = "" + // ErrNameContainsGroupSeparator is emitted if you attempt to instantiate a Group or Check that contains the separator. -var ErrNameContainsGroupSeparator = errors.New("group and check names may not contain '::'") +var ErrNameContainsGroupSeparator = errors.New("group and check names may not contain '" + GroupSeparator + "'") // StageFields defines the fields used for a Stage; this is a dumb hack to make the JSON code // cleaner. pls fix. @@ -107,13 +110,13 @@ type Group struct { // The root group must be created with the name "" and parent set to nil; this is the only case // where a nil parent or empty name is allowed. func NewGroup(name string, parent *Group) (*Group, error) { - if strings.Contains(name, GroupSeparator) { - return nil, ErrNameContainsGroupSeparator - } - - path := name + old := RootGroupPath if parent != nil { - path = parent.Path + GroupSeparator + path + old = parent.Path + } + path, err := NewGroupPath(old, name) + if err != nil { + return nil, err } hash := md5.Sum([]byte(path)) //nolint:gosec @@ -147,6 +150,17 @@ func (g *Group) Group(name string) (*Group, error) { return group, nil } +// NewGroupPath ... +func NewGroupPath(old, path string) (string, error) { + if strings.Contains(path, GroupSeparator) { + return "", ErrNameContainsGroupSeparator + } + if old == RootGroupPath && path == RootGroupPath { + return RootGroupPath, nil + } + return old + GroupSeparator + path, nil +} + // Check creates a child check belonging to this group. // This is safe to call from multiple goroutines simultaneously. func (g *Group) Check(name string) (*Check, error) { diff --git a/lib/runner.go b/lib/runner.go index 92bad4a22f2..3d840f22b79 100644 --- a/lib/runner.go +++ b/lib/runner.go @@ -49,8 +49,6 @@ type VUActivationParams struct { // // interfacebloat: We may evaluate in the future to move out some methods; // but considering how central it is, it would require a huge effort. -// -//nolint:interfacebloat type Runner interface { // Creates an Archive of the runner. There should be a corresponding NewFromArchive() function // that will restore the runner from the archive. @@ -73,9 +71,6 @@ type Runner interface { // Runs post-test teardown, if applicable. Teardown(ctx context.Context, out chan<- metrics.SampleContainer) error - // Returns the default (root) Group. - GetDefaultGroup() *Group - // Get and set options. The initial value will be whatever the script specifies (for JS, // `export let options = {}`); cmd/run.go will mix this in with CLI-, config- and env-provided // values and write it back to the runner. diff --git a/lib/test_state.go b/lib/test_state.go index dee70eaae12..9bc5e3a79e7 100644 --- a/lib/test_state.go +++ b/lib/test_state.go @@ -2,6 +2,8 @@ package lib import ( "io" + "strings" + "sync/atomic" "github.com/sirupsen/logrus" "go.k6.io/k6/event" @@ -31,6 +33,118 @@ type TestRunState struct { Runner Runner // TODO: rename to something better, see type comment RunTags *metrics.TagSet + GroupSummary *GroupSummary // TODO(@mstoykov): move and rename + // TODO: add other properties that are computed or derived after init, e.g. // thresholds? } + +// GroupSummaryDescription is the description of the GroupSummary used to identify and ignore it +// for the purposes of the cli descriptions. +const GroupSummaryDescription = "Internal Group Summary output" + +// GroupSummary is an internal output implementation that facilitates the aggregation of +// group and check metrics for the purposes of the end of test summary and the REST API +type GroupSummary struct { + group *Group // TODO(@mstoykov): move the whole type outside of lib later + ch chan []metrics.SampleContainer + stopped chan struct{} + logger logrus.FieldLogger +} + +// NewGroupSummary returns new GroupSummary ready to be started. +func NewGroupSummary(logger logrus.FieldLogger) *GroupSummary { + group, _ := NewGroup(RootGroupPath, nil) + return &GroupSummary{ + group: group, + ch: make(chan []metrics.SampleContainer, 1000), + stopped: make(chan struct{}), + logger: logger, + } +} + +// Group returns the underlying group that has been aggregated +func (gs *GroupSummary) Group() *Group { + return gs.group +} + +// Description is part of the output.Output interface +func (gs *GroupSummary) Description() string { + return GroupSummaryDescription +} + +func buildGroup(rootGroup *Group, groupName string) (*Group, error) { + group := rootGroup + groups := strings.Split(groupName, "::") + var err error + for _, groupName := range groups[1:] { + group, err = group.Group(groupName) + if err != nil { + return nil, err + } + } + return group, nil +} + +func (gs *GroupSummary) handleSample(sample metrics.Sample) error { + switch sample.Metric.Name { + case "group_duration", "checks": + groupName, ok := sample.Tags.Get(metrics.TagGroup.String()) + if !ok { + return nil + } + group, err := buildGroup(gs.group, groupName) + if err != nil { + return err + } + + if sample.Metric.Name != "checks" { + return nil + } + + checkName, ok := sample.Tags.Get(metrics.TagCheck.String()) + if !ok { + return nil + } + check, err := group.Check(checkName) + if err != nil { + return err + } + if sample.Value == 0 { + atomic.AddInt64(&check.Fails, 1) + } else { + atomic.AddInt64(&check.Passes, 1) + } + } + return nil +} + +// Start is part of the output.Output interface +func (gs *GroupSummary) Start() error { + go func() { + defer close(gs.stopped) + for containers := range gs.ch { + for _, container := range containers { + for _, sample := range container.GetSamples() { + err := gs.handleSample(sample) + if err != nil { + gs.logger.WithError(err).Warn("couldn't handle a sample as part of group summary") + } + } + } + } + }() + return nil +} + +// Stop is part of the output.Output interface +func (gs *GroupSummary) Stop() error { + close(gs.ch) + <-gs.stopped + return nil +} + +// AddMetricSamples is part of the output.Output interface +func (gs *GroupSummary) AddMetricSamples(samples []metrics.SampleContainer) { + gs.ch <- samples +} diff --git a/lib/testutils/minirunner/minirunner.go b/lib/testutils/minirunner/minirunner.go index 47e4457e6d1..d5fc66fe050 100644 --- a/lib/testutils/minirunner/minirunner.go +++ b/lib/testutils/minirunner/minirunner.go @@ -28,7 +28,6 @@ type MiniRunner struct { SetupData []byte - Group *lib.Group Options lib.Options PreInitState *lib.TestPreInitState @@ -86,14 +85,6 @@ func (r MiniRunner) Teardown(ctx context.Context, out chan<- metrics.SampleConta return nil } -// GetDefaultGroup returns the default group. -func (r MiniRunner) GetDefaultGroup() *lib.Group { - if r.Group == nil { - r.Group = &lib.Group{} - } - return r.Group -} - // IsExecutable satisfies lib.Runner, but is mocked for MiniRunner since // it doesn't deal with JS. func (r MiniRunner) IsExecutable(_ string) bool { diff --git a/lib/vu_state.go b/lib/vu_state.go index 1b70c54c1ee..cb66a8a840e 100644 --- a/lib/vu_state.go +++ b/lib/vu_state.go @@ -42,9 +42,6 @@ type State struct { // Logger instance for every VU. Logger logrus.FieldLogger - // Current group; all emitted metrics are tagged with this. - Group *Group - // Networking equipment. Dialer DialContexter From 8321b07ea1e3855c83760eaa03a1812014d3cd2f Mon Sep 17 00:00:00 2001 From: Mihail Stoykov <312246+mstoykov@users.noreply.github.com> Date: Thu, 23 May 2024 16:30:57 +0300 Subject: [PATCH 2/4] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Joan López de la Franca Beltran <5459617+joanlopez@users.noreply.github.com> --- api/v1/group_routes_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/v1/group_routes_test.go b/api/v1/group_routes_test.go index 92d8b1b4502..89167bbb5fa 100644 --- a/api/v1/group_routes_test.go +++ b/api/v1/group_routes_test.go @@ -66,7 +66,7 @@ func TestGetGroups(t *testing.T) { t.Parallel() cs := getControlSurface(t, getTestRunState(t, lib.Options{}, &minirunner.MiniRunner{})) - _ = cs.RunState.GroupSummary.Start() + require.NoError(t, cs.RunState.GroupSummary.Start()) cs.RunState.GroupSummary.AddMetricSamples([]metrics.SampleContainer{ metrics.Sample{ TimeSeries: metrics.TimeSeries{ @@ -87,7 +87,7 @@ func TestGetGroups(t *testing.T) { }, }, }) - _ = cs.RunState.GroupSummary.Stop() + require.NoError(t, cs.RunState.GroupSummary.Stop()) g0, err := lib.NewGroup("", nil) assert.NoError(t, err) From e82c53e6c2c921623e10ed1cd441920a43644bbe Mon Sep 17 00:00:00 2001 From: Mihail Stoykov Date: Mon, 27 May 2024 08:41:12 +0300 Subject: [PATCH 3/4] fixup! Apply suggestions from code review --- api/v1/group_routes_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/v1/group_routes_test.go b/api/v1/group_routes_test.go index 89167bbb5fa..88a082d2525 100644 --- a/api/v1/group_routes_test.go +++ b/api/v1/group_routes_test.go @@ -66,7 +66,7 @@ func TestGetGroups(t *testing.T) { t.Parallel() cs := getControlSurface(t, getTestRunState(t, lib.Options{}, &minirunner.MiniRunner{})) - require.NoError(t, cs.RunState.GroupSummary.Start()) + require.NoError(t, cs.RunState.GroupSummary.Start()) cs.RunState.GroupSummary.AddMetricSamples([]metrics.SampleContainer{ metrics.Sample{ TimeSeries: metrics.TimeSeries{ From ba596ca3f65932c99809321b55ad02aa78c84393 Mon Sep 17 00:00:00 2001 From: Mihail Stoykov <312246+mstoykov@users.noreply.github.com> Date: Tue, 28 May 2024 17:31:57 +0300 Subject: [PATCH 4/4] Update lib/models.go --- lib/models.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/models.go b/lib/models.go index b83a3433e39..30cb6b13b72 100644 --- a/lib/models.go +++ b/lib/models.go @@ -19,6 +19,9 @@ import ( const GroupSeparator = "::" // RootGroupPath is the id of the root group +// +// Note(@mstoykov): the constant shouldn't be used in all tests in order to not couple the tests too much with it. +// Changing this will be a breaking change and in this way it will be more obvious. const RootGroupPath = "" // ErrNameContainsGroupSeparator is emitted if you attempt to instantiate a Group or Check that contains the separator.