Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

metrics.Sample.Metadata: Data structure for high cardinality tags #2726

Merged
merged 3 commits into from
Oct 17, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/xk6-tests/xk6-js-test/jstest.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func (j *JSTest) Foo(arg float64) (bool, error) {

ctx := j.vu.Context()

tags := state.Tags.GetCurrentValues().With("foo", "bar")
tags := state.Tags.GetCurrentValues().Tags.With("foo", "bar")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems bad .... Maybe just rename Tags to TagsAndMetadata for now?

I feel like it will be better if they are separate fields in state but this might take longer to do and I can see benefits for both, so I am for renaming and opening an issue to discuss the internal API and maybe make changes for the next release.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the big reasons for TagsAndMetadata to be packaged together like this was because, in the original PR, we treated them as 2 flavors of tags on the user-facing side, but internally we were saving them in completely different data structures.

We needed to lock them at the same time and we needed to remove corresponding tags when adding metadata and vice-versa:

k6/metrics/tags.go

Lines 176 to 180 in cd1e21d

func (tm *TagsAndMeta) SetTag(key, value string) {
tm.Tags = tm.Tags.With(key, value)
if tm.Metadata != nil {
delete(tm.Metadata, key)
}

k6/metrics/tags.go

Lines 185 to 186 in cd1e21d

func (tm *TagsAndMeta) SetMetadata(key, value string) {
tm.Tags = tm.Tags.Without(key)

Now that it's most likely we won't need to have differently-flavored (indexed / non-indexed) tags, a lot of this feels unnecessary 🤔 I guess the common locking and helper methods are still good to have, as well as the internal "accumulator" for the final atlas Tags value, and we don't have a lot of time to rewrite this now... 😞 But 👍 for renaming state.Tags to state.TagsAndMeta

Copy link
Contributor Author

@codebien codebien Oct 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering we are going to open an issue I'm for not renaming now, and including the refactor as the first thing in the next release (v0.42.0). @na-- WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I am also fine with tackling this in next release, with maybe just a TODO in lib.State for now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added #2733 for tracking this

metrics.PushIfNotDone(ctx, state.Samples, metrics.Sample{
Time: time.Now(),
TimeSeries: metrics.TimeSeries{Metric: j.foos, Tags: tags},
Expand Down
22 changes: 12 additions & 10 deletions cmd/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -296,16 +296,15 @@ func testSSLKEYLOGFILE(t *testing.T, ts *globalTestState, filePath string) {
func TestThresholdDeprecationWarnings(t *testing.T) {
t.Parallel()

// TODO: adjust this test after we actually make url, error, iter and vu non-indexable

ts := newGlobalTestState(t)
ts.args = []string{"k6", "run", "--system-tags", "url,error,vu,iter", "-"}
ts.args = []string{"k6", "run", "--system-tags", "url,error,vu,iter,scenario", "-"}
ts.stdIn = bytes.NewReader([]byte(`
export const options = {
thresholds: {
'http_req_duration{url:https://test.k6.io}': ['p(95)<500', 'p(99)<1000'],
'http_req_duration{error:foo}': ['p(99)<1000'],
'iterations{vu:1,iter:0}': ['count == 1'],
'iterations{scenario:default}': ['count == 1'],
'iterations{vu:1,iter:0}': ['count == 0'], // iter and vu are now unindexable
},
};

Expand All @@ -315,16 +314,15 @@ func TestThresholdDeprecationWarnings(t *testing.T) {
newRootCommand(ts.globalState).execute()

logs := ts.loggerHook.Drain()
assert.False(t, testutils.LogContains(logs, logrus.WarnLevel, "http_req_duration{url:https://test.k6.io}"))

// We no longer warn about this
assert.False(t, testutils.LogContains(logs, logrus.WarnLevel, "http_req_duration{url:https://test.k6.io}"))
assert.False(t, testutils.LogContains(logs, logrus.WarnLevel, "http_req_duration{error:foo}"))
assert.True(t, testutils.LogContains(logs, logrus.WarnLevel,
"Thresholds like 'http_req_duration{error:foo}', based on the high-cardinality 'error' metric tag, are deprecated",
))
assert.True(t, testutils.LogContains(logs, logrus.WarnLevel,
"Thresholds like 'iterations{vu:1,iter:0}', based on the high-cardinality 'vu' metric tag, are deprecated",
"The high-cardinality 'vu' metric tag was made non-indexable in k6 v0.41.0, so thresholds like 'iterations{vu:1,iter:0}'",
))
assert.True(t, testutils.LogContains(logs, logrus.WarnLevel,
"Thresholds like 'iterations{vu:1,iter:0}', based on the high-cardinality 'iter' metric tag, are deprecated",
"The high-cardinality 'iter' metric tag was made non-indexable in k6 v0.41.0, so thresholds like 'iterations{vu:1,iter:0}'",
))
}

Expand Down Expand Up @@ -478,3 +476,7 @@ func TestThresholdsFailed(t *testing.T) {
require.Contains(t, stdOut, ` ✗ { scenario:sc2 }...: 2`)
require.Contains(t, stdOut, ` ✓ { scenario:sc3 }...: 0 0/s`)
}

// TODO: add an integration test that verifies that unindexable tags work as
// expected and that VU tags from different scenarios don't cross between
// scenarios and pollute other metrics.
8 changes: 5 additions & 3 deletions core/local/local_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1240,11 +1240,13 @@ func TestRealTimeAndSetupTeardownMetrics(t *testing.T) {
getDummyTrail := func(group string, emitIterations bool, addExpTags ...string) metrics.SampleContainer {
expTags := []string{"group", group}
expTags = append(expTags, addExpTags...)
return netext.NewDialer(
dialer := netext.NewDialer(
net.Dialer{},
netext.NewResolver(net.LookupIP, 0, types.DNSfirst, types.DNSpreferIPv4),
).GetTrail(time.Now(), time.Now(),
true, emitIterations, getTags(piState.Registry, expTags...), piState.BuiltinMetrics)
)

ctm := metrics.TagsAndMeta{Tags: getTags(piState.Registry, expTags...)}
return dialer.GetTrail(time.Now(), time.Now(), true, emitIterations, ctm, piState.BuiltinMetrics)
}

// Initially give a long time (5s) for the execScheduler to start
Expand Down
3 changes: 2 additions & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ require (
github.com/gorilla/websocket v1.5.0
github.com/grafana/xk6-redis v0.1.1
github.com/grafana/xk6-timers v0.1.2
github.com/grafana/xk6-websockets v0.1.4
github.com/grafana/xk6-websockets v0.1.5
github.com/influxdata/influxdb1-client v0.0.0-20190402204710-8ff2fc3824fc
github.com/jhump/protoreflect v1.13.0
github.com/klauspost/compress v1.15.11
Expand Down Expand Up @@ -50,6 +50,7 @@ require (
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/dgryski/go-rendezvous v0.0.0-20200823014737-9f7001d12a5f // indirect
github.com/dlclark/regexp2 v1.7.0 // indirect
github.com/fsnotify/fsnotify v1.5.4 // indirect
github.com/go-redis/redis/v8 v8.11.5 // indirect
github.com/go-sourcemap/sourcemap v2.1.4-0.20211119122758-180fcef48034+incompatible
github.com/inconshreveable/mousetrap v1.0.0 // indirect
Expand Down
6 changes: 3 additions & 3 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,8 @@ github.com/grafana/xk6-redis v0.1.1 h1:rvWnLanRB2qzDwuY6NMBe6PXei3wJ3kjYvfCwRJ+q
github.com/grafana/xk6-redis v0.1.1/go.mod h1:z7el1Tz8advY+ex419KfLbENzSQYgaA2lQYwMlt9yMM=
github.com/grafana/xk6-timers v0.1.2 h1:YVM6hPDgvy4SkdZQpd+/r9M0kDi1g+QdbSxW5ClfwDk=
github.com/grafana/xk6-timers v0.1.2/go.mod h1:XHmDIXAKe30NJMXrxKIKMFXx98etsCl0jBYktjsSURc=
github.com/grafana/xk6-websockets v0.1.4 h1:OVqNZ3rIXFjY+MEeBf2lhOYJMfIbKT1Bppm4XnDQQWU=
github.com/grafana/xk6-websockets v0.1.4/go.mod h1:MAvkRFMyyRaxn1AG0sU33dujRlWdlNm++qpTblQESkg=
github.com/grafana/xk6-websockets v0.1.5 h1:jOAkQqB88m+osXagOr7TwP+ZB/u637UpysxCZd7B84E=
github.com/grafana/xk6-websockets v0.1.5/go.mod h1:+vlArhLMFFvNFgs5GUsw3RtzlDxli1G5SFMdC/QEUxU=
github.com/grpc-ecosystem/grpc-gateway v1.16.0/go.mod h1:BDjrQk3hbvj6Nolgz8mAMFbcEtjT1g+wF4CSlocrBnw=
github.com/hpcloud/tail v1.0.0/go.mod h1:ab1qPbhIpdTxEkNHXyeSf5vhxWSCs/tWer42PpOxQnU=
github.com/ianlancetaylor/demangle v0.0.0-20200824232613-28f6c0f3b639/go.mod h1:aSSvb/t6k1mPoxDqO4vJh6VOCGPwU4O0C2/Eqndh1Sc=
Expand Down Expand Up @@ -204,7 +204,7 @@ github.com/yuin/goldmark v1.2.1/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9dec
github.com/yuin/goldmark v1.3.5/go.mod h1:mwnBkeHKe2W/ZEtQ+71ViKU8L12m81fl3OWwC1Zlc8k=
go.k6.io/k6 v0.37.1-0.20220426072701-d105f5474bc3/go.mod h1:1bTdDsXTT2V3in3ZgdR15MDW6SQQh5nWni59tirqNB8=
go.k6.io/k6 v0.38.2/go.mod h1:1bTdDsXTT2V3in3ZgdR15MDW6SQQh5nWni59tirqNB8=
go.k6.io/k6 v0.40.1-0.20221004102444-50bc9af81f3a/go.mod h1:/NkhZeFQazmfRxuD05zoqG2pUd5Wxa1D78IiSEq4Huk=
go.k6.io/k6 v0.40.1-0.20221017105932-3c97ec7d1231/go.mod h1:vaSQ1A2rnC+wrKRJ4ExZCT6kKLfAEYoaIY9UR0uAjjk=
go.opentelemetry.io/proto/otlp v0.7.0/go.mod h1:PqfVotwruBrMGOCsRd/89rSnXhoiJIqeYNgFYFoEGnI=
golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=
Expand Down
27 changes: 13 additions & 14 deletions js/common/tags.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,31 +8,29 @@ import (
"go.k6.io/k6/metrics"
)

// ApplyCustomUserTags modifies the given metrics.TagSet object with the
// user specified custom tags.
// ApplyCustomUserTags modifies the given metrics.TagsAndMeta object with the
// user specified custom tags and metadata.
// It expects to receive the `keyValues` object in the `{key1: value1, key2:
// value2, ...}` format.
func ApplyCustomUserTags(rt *goja.Runtime, tags *metrics.TagSet, keyValues goja.Value) (*metrics.TagSet, error) {
func ApplyCustomUserTags(rt *goja.Runtime, tagsAndMeta *metrics.TagsAndMeta, keyValues goja.Value) error {
if keyValues == nil || goja.IsNull(keyValues) || goja.IsUndefined(keyValues) {
return tags, nil
return nil
}

keyValuesObj := keyValues.ToObject(rt)

newTags := tags
var err error
for _, key := range keyValuesObj.Keys() {
if newTags, err = ApplyCustomUserTag(rt, newTags, key, keyValuesObj.Get(key)); err != nil {
return nil, err
if err := ApplyCustomUserTag(rt, tagsAndMeta, key, keyValuesObj.Get(key)); err != nil {
return err
}
}

return newTags, nil
return nil
}

// ApplyCustomUserTag modifies the given metrics.TagSet object with the
// given custom tag and its value.
func ApplyCustomUserTag(rt *goja.Runtime, tags *metrics.TagSet, key string, val goja.Value) (*metrics.TagSet, error) {
// ApplyCustomUserTag modifies the given metrics.TagsAndMeta object with the
// given custom tag or metadata and theirs value.
func ApplyCustomUserTag(rt *goja.Runtime, tagsAndMeta *metrics.TagsAndMeta, key string, val goja.Value) error {
kind := reflect.Invalid
if typ := val.ExportType(); typ != nil {
kind = typ.Kind()
Expand All @@ -45,10 +43,11 @@ func ApplyCustomUserTag(rt *goja.Runtime, tags *metrics.TagSet, key string, val
reflect.Int64,
reflect.Float64:

return tags.With(key, val.String()), nil
tagsAndMeta.SetTag(key, val.String())
return nil

default:
return nil, fmt.Errorf(
return fmt.Errorf(
"invalid value for metric tag '%s': "+
"only String, Boolean and Number types are accepted as a metric tag values",
key,
Expand Down
37 changes: 20 additions & 17 deletions js/modules/k6/execution/execution.go
Original file line number Diff line number Diff line change
Expand Up @@ -303,9 +303,12 @@ type tagsDynamicObject struct {
// Get a property value for the key. May return nil if the property does not exist.
func (o *tagsDynamicObject) Get(key string) goja.Value {
tcv := o.state.Tags.GetCurrentValues()
if tag, ok := tcv.Get(key); ok {
if tag, ok := tcv.Tags.Get(key); ok {
return o.runtime.ToValue(tag)
}
if metadatum, ok := tcv.Metadata[key]; ok {
return o.runtime.ToValue(metadatum)
}
return nil
}

Expand All @@ -314,24 +317,22 @@ func (o *tagsDynamicObject) Get(key string) goja.Value {
// representation. An exception is raised in case a denied type is provided.
func (o *tagsDynamicObject) Set(key string, val goja.Value) bool {
var err error
o.state.Tags.Modify(func(tags *metrics.TagSet) *metrics.TagSet {
newTags, applyErr := common.ApplyCustomUserTag(o.runtime, tags, key, val)
if applyErr != nil {
err = applyErr
return tags
o.state.Tags.Modify(func(tagsAndMeta *metrics.TagsAndMeta) {
err = common.ApplyCustomUserTag(o.runtime, tagsAndMeta, key, val)
na-- marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
panic(o.runtime.NewTypeError(err.Error()))
}
return newTags
})
if err == nil {
return true
}
panic(o.runtime.NewTypeError(err.Error()))
return true
}

// Has returns true if the property exists.
func (o *tagsDynamicObject) Has(key string) bool {
ctv := o.state.Tags.GetCurrentValues()
if _, ok := ctv.Get(key); ok {
if _, ok := ctv.Tags.Get(key); ok {
return true
}
if _, ok := ctv.Metadata[key]; ok {
return true
}
return false
Expand All @@ -340,10 +341,9 @@ func (o *tagsDynamicObject) Has(key string) bool {
// Delete deletes the property for the key. It returns true on success (note,
// that includes missing property).
func (o *tagsDynamicObject) Delete(key string) bool {
o.state.Tags.Modify(func(tags *metrics.TagSet) *metrics.TagSet {
return tags.Without(key)
o.state.Tags.Modify(func(tagsAndMeta *metrics.TagsAndMeta) {
tagsAndMeta.DeleteTag(key)
})

return true
}

Expand All @@ -352,10 +352,13 @@ func (o *tagsDynamicObject) Delete(key string) bool {
func (o *tagsDynamicObject) Keys() []string {
ctv := o.state.Tags.GetCurrentValues()

tagsMap := ctv.Map()
keys := make([]string, 0, len(tagsMap))
tagsMap := ctv.Tags.Map()
keys := make([]string, 0, len(tagsMap)+len(ctv.Metadata))
for k := range tagsMap {
keys = append(keys, k)
}
for k := range ctv.Metadata {
keys = append(keys, k)
}
return keys
}
6 changes: 3 additions & 3 deletions js/modules/k6/execution/execution_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ func TestVUTagsJSONEncoding(t *testing.T) {
},
Tags: lib.NewVUStateTags(metrics.NewRegistry().RootTagSet().With("vu", "42")),
})
tenv.VU.State().Tags.Modify(func(tags *metrics.TagSet) *metrics.TagSet {
return tags.With("custom-tag", "mytag1")
tenv.VU.State().Tags.Modify(func(tagsAndMeta *metrics.TagsAndMeta) {
tagsAndMeta.SetTag("custom-tag", "mytag1")
})

encoded, err := tenv.VU.Runtime().RunString(`JSON.stringify(exec.vu.tags)`)
Expand Down Expand Up @@ -304,7 +304,7 @@ func TestOptionsTestFull(t *testing.T) {
SummaryTrendStats: []string{"avg", "min", "max"},
SummaryTimeUnit: null.StringFrom("ms"),
SystemTags: func() *metrics.SystemTagSet {
sysm := metrics.TagIter | metrics.TagVU
sysm := metrics.SystemTagSet(metrics.TagIter | metrics.TagVU)
return &sysm
}(),
RunTags: map[string]string{"runtag-key": "runtag-value"},
Expand Down
30 changes: 12 additions & 18 deletions js/modules/k6/grpc/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -216,25 +216,21 @@ func (c *Client) Invoke(
defer cancel()

if state.Options.SystemTags.Has(metrics.TagURL) {
p.Tags = p.Tags.With("url", fmt.Sprintf("%s%s", c.addr, method))
p.TagsAndMeta.SetSystemTagOrMeta(metrics.TagURL, fmt.Sprintf("%s%s", c.addr, method))
}
parts := strings.Split(method[1:], "/")
if state.Options.SystemTags.Has(metrics.TagService) {
p.Tags = p.Tags.With("service", parts[0])
}
if state.Options.SystemTags.Has(metrics.TagMethod) {
p.Tags = p.Tags.With("method", parts[1])
}
p.TagsAndMeta.SetSystemTagOrMetaIfEnabled(state.Options.SystemTags, metrics.TagService, parts[0])
p.TagsAndMeta.SetSystemTagOrMetaIfEnabled(state.Options.SystemTags, metrics.TagMethod, parts[1])

// Only set the name system tag if the user didn't explicitly set it beforehand
if _, ok := p.Tags.Get("name"); !ok && state.Options.SystemTags.Has(metrics.TagName) {
p.Tags = p.Tags.With("name", method)
if _, ok := p.TagsAndMeta.Tags.Get("name"); !ok {
p.TagsAndMeta.SetSystemTagOrMetaIfEnabled(state.Options.SystemTags, metrics.TagName, method)
}

reqmsg := grpcext.Request{
MethodDescriptor: methodDesc,
Message: b,
Tags: p.Tags,
TagsAndMeta: &p.TagsAndMeta,
}

return c.conn.Invoke(ctx, method, md, reqmsg)
Expand Down Expand Up @@ -318,15 +314,15 @@ func (c *Client) convertToMethodInfo(fdset *descriptorpb.FileDescriptorSet) ([]M
}

type invokeParams struct {
Metadata map[string]string
Tags *metrics.TagSet
Timeout time.Duration
Metadata map[string]string
TagsAndMeta metrics.TagsAndMeta
Timeout time.Duration
}

func (c *Client) parseInvokeParams(paramsVal goja.Value) (*invokeParams, error) {
result := &invokeParams{
Timeout: 1 * time.Minute,
Tags: c.vu.State().Tags.GetCurrentValues(),
Timeout: 1 * time.Minute,
TagsAndMeta: c.vu.State().Tags.GetCurrentValues(),
}
if paramsVal == nil || goja.IsUndefined(paramsVal) || goja.IsNull(paramsVal) {
return result, nil
Expand Down Expand Up @@ -354,11 +350,9 @@ func (c *Client) parseInvokeParams(paramsVal goja.Value) (*invokeParams, error)
result.Metadata[hk] = strval
}
case "tags":
newTags, err := common.ApplyCustomUserTags(rt, result.Tags, params.Get(k))
if err != nil {
if err := common.ApplyCustomUserTags(rt, &result.TagsAndMeta, params.Get(k)); err != nil {
return result, fmt.Errorf("metric tags: %w", err)
}
result.Tags = newTags
case "timeout":
var err error
v := params.Get(k).Export()
Expand Down
6 changes: 2 additions & 4 deletions js/modules/k6/http/request.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ func (c *Client) parseRequest(
Redirects: state.Options.MaxRedirects,
Cookies: make(map[string]*httpext.HTTPRequestCookie),
ResponseCallback: c.responseCallback,
Tags: c.moduleInstance.vu.State().Tags.GetCurrentValues(),
TagsAndMeta: c.moduleInstance.vu.State().Tags.GetCurrentValues(),
}

if state.Options.DiscardResponseBodies.Bool {
Expand Down Expand Up @@ -308,11 +308,9 @@ func (c *Client) parseRequest(
case "redirects":
result.Redirects = null.IntFrom(params.Get(k).ToInteger())
case "tags":
newTags, err := common.ApplyCustomUserTags(rt, result.Tags, params.Get(k))
if err != nil {
if err := common.ApplyCustomUserTags(rt, &result.TagsAndMeta, params.Get(k)); err != nil {
return nil, fmt.Errorf("invalid HTTP request metric tags: %w", err)
}
result.Tags = newTags
case "auth":
result.Auth = params.Get(k).String()
case "timeout":
Expand Down
8 changes: 4 additions & 4 deletions js/modules/k6/http/response_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,13 +148,13 @@ func TestResponse(t *testing.T) {
if assert.NoError(t, err) {
old := state.Group
state.Group = g
state.Tags.Modify(func(currentTags *metrics.TagSet) *metrics.TagSet {
return currentTags.With("group", g.Path)
state.Tags.Modify(func(tagsAndMeta *metrics.TagsAndMeta) {
tagsAndMeta.SetTag("group", g.Path)
})
defer func() {
state.Group = old
state.Tags.Modify(func(currentTags *metrics.TagSet) *metrics.TagSet {
return currentTags.With("group", old.Path)
state.Tags.Modify(func(tagsAndMeta *metrics.TagsAndMeta) {
tagsAndMeta.SetTag("group", old.Path)
})
}()
}
Expand Down
Loading