js/k6/exec: Set VU Tags #2172

codebien · 2021-10-11T09:40:35Z

Extended the k6/execution API to allow the get or set of a Tag in the VU's state.

This part of the Stage tagging work, but it can be considered as an independent feature.

import http from 'k6/http';
import exec from 'k6/execution';

export const options = {
  stages: [
    { duration: '1m', target: 10 },
    { duration: '1m', target: 20 },
    { duration: '1m', target: 0 },
  ],
};

export default function () {
  exec.vu.tags['stage'] = 'stage#2'
  console.log(exec.vu.tags['stage']) // returns stage#2
}

mstoykov · 2021-10-11T14:36:18Z

js/modules/k6/execution/execution.go

+	if enabled, ok := o.SystemTags[key]; ok && enabled {
+		return false
+	}


The way systemTags.Map is implemented enabled is always true if it's in the map (ok is true). Was your idea that you should not be able to set a tag with the same name as a system tag if it's not enabled ... because I don't think this is needed if someone wants to set a tag ... they probably should be able to, even if it collides with some of ours ... if anything for most cases unless it's disabled we will likely overwrite it somewhere down the codepath, for at least some of them

js/modules/k6/execution/execution.go

js/modules/k6/execution/execution_test.go

codebien · 2021-10-21T17:04:07Z

Pushed some basic code for supporting the lock. WDYT? I will wait for a consensus about the API before polishing more.

js/modules/k6/ws/ws.go

mstoykov

The basic TagMap seems fine to me, so we can continue polishing it.

It seems because of the usage of lib.State.CloneTags() in most places it won't be as big of a change as I expected 😅

codebien · 2021-10-26T16:47:02Z

Added the full test coverage for the TagMap API and removed the useless check for system tags. Ready for a new review.

mstoykov

LGTM in general, I don't see how this will break.

I might've tried sync.Map but I don't think it matters enough and we can always redo it in the future

js/modules/k6/execution/execution.go

js/modules/k6/execution/execution_test.go

codebien · 2021-10-28T09:20:33Z

I might've tried sync.Map but I don't think it matters enough and we can always redo it in the future

I thought about it, but the case of this PR doesn't seem to fit very well for it.

From the sync.Map doc:

The Map type is specialized. Most code should use a plain Go map instead, with separate locking or coordination, for better type safety and to make it easier to maintain other invariants along with the map content.

The Map type is optimized for two common use cases: (1) when the entry for a given key is only ever written once but read many times, as in caches that only grow, or (2) when multiple goroutines read, write, and overwrite entries for disjoint sets of keys. In these two cases, use of a Map may significantly reduce lock contention compared to a Go map paired with a separate Mutex or RWMutex.

It would require k6 specific benchmarks. Maybe, can we can consider to open an issue and iterate on it if we will identify a requirement?

codebien · 2021-10-28T15:02:34Z

@mstoykov Added the json test but we should be sure that we are fine with #2172 (comment)

js/modules/k6/execution/execution.go

js/modules/k6/execution/execution_test.go

yorugac · 2021-10-29T09:17:34Z

Just a note on sync.Map for tags experiments: it probably falls within the scope of #1831

na--

Mostly LGTM and I'm fine with merging it if you make a v0.36.0 issue about https://github.com/grafana/k6/pull/2172/files#r740787612

Though fixing that should probably be easy, so it might be a bit better if you do it in this PR 🤷‍♂️

codebien · 2021-11-02T08:16:25Z

Though fixing that should probably be easy, so it might be a bit better if you do it in this PR 🤷‍♂️

@na-- I prefer to address it now, I will try it today 👍

codebien · 2021-11-02T17:22:44Z

Pushed changes as defined in #2172 (comment)

js/modules/k6/execution/execution.go

mstoykov · 2021-11-03T09:28:42Z

js/modules/k6/execution/execution.go

+// In any other case, if the Throw option is set then an error is raised
+// otherwise just a Warning is written.
+func (o *tagsDynamicObject) Set(key string, val goja.Value) bool {
+	switch val.ExportType().Kind() { //nolint:exhaustive


Doesn't the default case make this linter compliment?

apparently not, seems it prefer explicit cases

nishanths/exhaustive#1

codebien · 2021-11-03T09:38:29Z

js/modules/k6/execution/execution_test.go

+func TestVUTags(t *testing.T) {
+	t.Parallel()
+
+	setupExecModule := func(t *testing.T) (*ModuleInstance, *goja.Runtime, *testutils.SimpleLogrusHook) { // nolint:unparam


@mstoykov regarding linter, this also is not totally clear to me, maybe it's spotting a bug that I'm not seeing

~~You are not using t I think~~ I just saw where you do .... 🤔

from my local experiments ... it seems like unparam can't find that you actually use the hook(the last result) in one of the cases and that is what is being.

If you add it to first subcase and assert that it's empty it fixed itself 🤷

Also maybe (not necessary in this PR you can move this to the outside in a function and not actually have sub tests with but just a bunch of single tests as this just adds indentation currently.

I also have started to make these functions to return a struct with the things so that you don't have to constantly add 1 more returned thing whenever we add one more thing we need.

js/modules/k6/execution/execution_test.go

mstoykov

LGTM 👏

I would just like to point out that for some system tags setting them through this will have effects.

Like for example setting name will work for HTTP requests, but setting url (and having it still enabled in system tags) will not, and probably most others in such a situation. But for example vu or scenario will always be overwritten.

This definitely needs some documentation and likely at least a discussion (in an issue) on what needs to be done if anything

js/modules/k6/execution/execution.go

Extended the k6/execution API to allow the get or set of a Tag in the VU state.

codebien · 2021-11-03T10:57:40Z

rebased
refactor of the helper
added a float case

This definitely needs some documentation and likely at least a discussion (in an issue) on what needs to be done if anything

I will add it to the documentation, @mstoykov can you open an issue for it, please? (I'm not sure to know all the possible cases so I will be for sure not exhaustive in describing the issue, and probably you already have a better vision about possible implementations).

yorugac

This version looks much more neat. Nice work 😄

codebien self-assigned this Oct 11, 2021

codebien force-pushed the vu-tags branch from a951e63 to 5aa44b1 Compare October 11, 2021 13:36

codebien requested review from mstoykov, na--, imiric, inancgumus and yorugac October 11, 2021 13:49

mstoykov reviewed Oct 11, 2021

View reviewed changes

yorugac requested changes Oct 12, 2021

View reviewed changes

js/modules/k6/execution/execution_test.go Outdated Show resolved Hide resolved

mstoykov added this to the v0.35.0 milestone Oct 19, 2021

codebien force-pushed the vu-tags branch 3 times, most recently from 4023685 to 6404b50 Compare October 21, 2021 16:46

codebien requested a review from mstoykov October 25, 2021 14:47

mstoykov reviewed Oct 26, 2021

View reviewed changes

js/modules/k6/ws/ws.go Outdated Show resolved Hide resolved

mstoykov reviewed Oct 26, 2021

View reviewed changes

codebien force-pushed the vu-tags branch 2 times, most recently from 3790391 to d3e4d1f Compare October 26, 2021 16:01

codebien requested review from yorugac and mstoykov October 26, 2021 16:50

mstoykov previously approved these changes Oct 28, 2021

View reviewed changes

js/modules/k6/execution/execution.go Outdated Show resolved Hide resolved

js/modules/k6/execution/execution_test.go Outdated Show resolved Hide resolved

codebien dismissed mstoykov’s stale review via 85894f8 October 28, 2021 09:46

codebien force-pushed the vu-tags branch from d3e4d1f to 85894f8 Compare October 28, 2021 09:46

yorugac requested changes Oct 29, 2021

View reviewed changes

js/modules/k6/execution/execution.go Show resolved Hide resolved

js/modules/k6/execution/execution_test.go Outdated Show resolved Hide resolved

lib/state: concurrent-safe Tags map

62e4ac1

yorugac previously approved these changes Nov 1, 2021

View reviewed changes

This was referenced Nov 1, 2021

js/execution: Stage identifier #2215

Closed

Tag metrics from different stages #796

Closed

na-- reviewed Nov 2, 2021

View reviewed changes

codebien mentioned this pull request Nov 2, 2021

Types accepted for custom tags #2218

Open

codebien dismissed stale reviews from yorugac and mstoykov via 0c2a369 November 2, 2021 17:02

codebien requested review from mstoykov, na-- and yorugac November 2, 2021 17:22

na-- reviewed Nov 3, 2021

View reviewed changes

js/modules/k6/execution/execution.go Outdated Show resolved Hide resolved

na-- reviewed Nov 3, 2021

View reviewed changes

js/modules/k6/execution/execution.go Outdated Show resolved Hide resolved

mstoykov reviewed Nov 3, 2021

View reviewed changes

codebien commented Nov 3, 2021

View reviewed changes

mstoykov reviewed Nov 3, 2021

View reviewed changes

js/modules/k6/execution/execution_test.go Outdated Show resolved Hide resolved

mstoykov previously approved these changes Nov 3, 2021

View reviewed changes

mstoykov reviewed Nov 3, 2021

View reviewed changes

js/modules/k6/execution/execution.go Outdated Show resolved Hide resolved

js/k6/exec: Allow to set state's Tags

225b594

Extended the k6/execution API to allow the get or set of a Tag in the VU state.

codebien dismissed mstoykov’s stale review via 225b594 November 3, 2021 10:44

codebien force-pushed the vu-tags branch from 63e7fa1 to 225b594 Compare November 3, 2021 10:44

na-- approved these changes Nov 3, 2021

View reviewed changes

codebien requested a review from mstoykov November 3, 2021 11:03

yorugac approved these changes Nov 3, 2021

View reviewed changes

mstoykov mentioned this pull request Nov 3, 2021

exec.vu.tags: Tags not overwritten consistently by System Tags #2221

Closed

mstoykov approved these changes Nov 3, 2021

View reviewed changes

codebien merged commit e57ff82 into master Nov 3, 2021

codebien deleted the vu-tags branch November 3, 2021 12:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

js/k6/exec: Set VU Tags #2172

js/k6/exec: Set VU Tags #2172

codebien commented Oct 11, 2021 •

edited

Loading

mstoykov Oct 11, 2021

codebien commented Oct 21, 2021

mstoykov left a comment

codebien commented Oct 26, 2021

mstoykov left a comment

codebien commented Oct 28, 2021 •

edited

Loading

codebien commented Oct 28, 2021 •

edited

Loading

yorugac commented Oct 29, 2021

na-- left a comment

codebien commented Nov 2, 2021

codebien commented Nov 2, 2021

mstoykov Nov 3, 2021

codebien Nov 3, 2021

mstoykov Nov 3, 2021

codebien Nov 3, 2021

mstoykov Nov 3, 2021 •

edited

Loading

mstoykov Nov 3, 2021

mstoykov left a comment

codebien commented Nov 3, 2021 •

edited

Loading

yorugac left a comment

js/k6/exec: Set VU Tags #2172

js/k6/exec: Set VU Tags #2172

Conversation

codebien commented Oct 11, 2021 • edited Loading

mstoykov Oct 11, 2021

Choose a reason for hiding this comment

codebien commented Oct 21, 2021

mstoykov left a comment

Choose a reason for hiding this comment

codebien commented Oct 26, 2021

mstoykov left a comment

Choose a reason for hiding this comment

codebien commented Oct 28, 2021 • edited Loading

codebien commented Oct 28, 2021 • edited Loading

yorugac commented Oct 29, 2021

na-- left a comment

Choose a reason for hiding this comment

codebien commented Nov 2, 2021

codebien commented Nov 2, 2021

mstoykov Nov 3, 2021

Choose a reason for hiding this comment

codebien Nov 3, 2021

Choose a reason for hiding this comment

mstoykov Nov 3, 2021

Choose a reason for hiding this comment

codebien Nov 3, 2021

Choose a reason for hiding this comment

mstoykov Nov 3, 2021 • edited Loading

Choose a reason for hiding this comment

mstoykov Nov 3, 2021

Choose a reason for hiding this comment

mstoykov left a comment

Choose a reason for hiding this comment

codebien commented Nov 3, 2021 • edited Loading

yorugac left a comment

Choose a reason for hiding this comment

codebien commented Oct 11, 2021 •

edited

Loading

codebien commented Oct 28, 2021 •

edited

Loading

codebien commented Oct 28, 2021 •

edited

Loading

mstoykov Nov 3, 2021 •

edited

Loading

codebien commented Nov 3, 2021 •

edited

Loading