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

codebien · 2022-10-13T08:51:51Z

This is built on top of #2654 and #2594 with a more limited scope and it closes #2584.

One of the main use cases for this support of high-cardinality metric tags/metadata is to allow k6 to support traces and to export the trace_id and span_id values it generated to something external like Tempo and Jaeger. The goal is to eventually have this with the new k6 HTTP API. However, this PR does not add a direct JavaScript API so the direct use of this feature will be only available from an extension.

One change from the original issue description has been the decision to not have a raw_url non-indexable url metadata and have a separate issue for tracking feedback and ideas.

js/modules/k6/execution/execution.go

lib/netext/httpext/transport.go

metrics/tags.go

mstoykov

LGTM!

I did a really fast pass over it, so might've have missed something.

Some things that we might want to fix (later):

It is a bit inconsistant between using metadata and non-indexable. Both in the code and in the messaging to the user. Some of those likely can be mass changed now.
I find state.Tags (and if we rename it to state.TagsAndMetadata) a bit ... strange. As it is combined in the state, but then we edit the separate parts and put it separately in the final samples.
Emitting samples with the current tags never was ... nice as you needed to get the tags from one place add yours and then set the whole thing in the samples. I guess now that you need to get tags object from somewhere it will guide users to actuallyt get it from the lib.State. But it still clunky IMO. This is even more clunky with metadata - I would expect in most cases code will jsut not set metadata. Arguably having it combined in state.TagsAndMetadata guides to this as well 🤷 . I just find the whole thing not nice and not intuitive and not so hard to get wrong.

All of those though should likely be done in separate PRs ... likely after we have used the new code for a bit. And have discussed the js API for editing metadata/non-indexable nontags

mstoykov · 2022-10-13T09:42:25Z

metrics/engine/engine.go

-		me.logger.Warnf("Thresholds like '%s', based on the high-cardinality 'vu' metric tag, "+
-			"are deprecated and will not be supported in future k6 releases.", name,
+		me.logger.Warnf(
+			"The high-cardinality 'vu' metric tag was made non-indexable in k6 v0.41.0, so thresholds"+


nit; maybe change this message to not use non-indexable - I am not certain we should use metadata yet 🤷

maybe non-indexable is correct enough 🤷

I don't think replacing non-indexable with metadata is better. If we can find something clearer I'm ok with it, otherwise, I would prefer to let as-is. I haven't a better suggestion at the moment.

mstoykov · 2022-10-13T09:44:34Z

.github/workflows/xk6-tests/xk6-js-test/jstest.go

@@ -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")


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.

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

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?

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

Added #2733 for tracking this

metrics/tags.go

na-- · 2022-10-16T19:36:40Z

lib/netext/httpext/request.go

+				combinedLogFields[k] = v
+			}
+		}
+		for k, v := range preq.TagsAndMeta.Metadata {


Just in case, we should probably reverse the order here - first add the Metadata entries and then the Tags. That way, if there is a duplicate key, the tag value will be shown (which is probably more important 🤔

Should we split them into two sub-json-objects? I think could be relevant to see both, especially in debugging phases.

{ // ... "iter": 10, "tags": {"key1":"val1",...}, "meta": {"meta1":"val1"} }

I don't think this is possible, and even if it is, it won't look nice with logrus. Since it's such a minor concern and only matters for --http-debug, I think we'd be fine with the current solution, just flipping the tags.

na--

Thank you! 🙇

olegbespalov

In general, LGTM 👍 💪 Left a small non-blocking comment.

olegbespalov · 2022-10-17T08:47:35Z

metrics/tags_test.go

@@ -109,6 +109,8 @@ func TestTagSetContains(t *testing.T) {
 	assert.False(t, st.Contains(outer))
 }

+// TODO: Add TagsAndMeta tests


👀 should we add them with this PR?

codebien · 2022-10-17T10:34:06Z

@olegbespalov @na-- I added the tests and reversed the order of inserting tagsAndMeta for the logs.
If you will approve I will rebase and go through the process of pinning the right experimental version for the xk6-websockets.

Remove for a temporary commit the experimental module to allow bumping the k6 dependency in the extension's repository.

Uses the [email protected] version that is now migrated to the new metadata for non-indexable tags.

See grafana#2950 and grafana#2726 (comment) for more information. Signed-off-by: Siddhesh Mhadnak <[email protected]>

codebien self-assigned this Oct 13, 2022

codebien requested a review from na-- October 13, 2022 08:52

github-actions bot requested a review from oleiade October 13, 2022 08:52

codebien changed the title ~~metrics: Sample.Metadata - Data structure for high cardinality tags~~ metrics.Sample.Metadata: Data structure for high cardinality tags Oct 13, 2022

olegbespalov reviewed Oct 13, 2022

View reviewed changes

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

lib/netext/httpext/transport.go Outdated Show resolved Hide resolved

metrics/tags.go Show resolved Hide resolved

olegbespalov mentioned this pull request Oct 13, 2022

Support non-indexable high-cardinality metric tags/metadata #2654

Closed

mstoykov previously approved these changes Oct 13, 2022

View reviewed changes

na-- reviewed Oct 13, 2022

View reviewed changes

metrics/tags.go Outdated Show resolved Hide resolved

na-- reviewed Oct 13, 2022

View reviewed changes

metrics/tags.go Outdated Show resolved Hide resolved

na-- reviewed Oct 13, 2022

View reviewed changes

metrics/tags.go Outdated Show resolved Hide resolved

codebien dismissed mstoykov’s stale review via 45b0181 October 13, 2022 10:55

codebien requested review from na-- and olegbespalov October 13, 2022 11:05

na-- reviewed Oct 16, 2022

View reviewed changes

na-- previously approved these changes Oct 16, 2022

View reviewed changes

olegbespalov previously approved these changes Oct 17, 2022

View reviewed changes

codebien mentioned this pull request Oct 17, 2022

lib.State: Split or rename the Tags field #2733

Open

mstoykov previously approved these changes Oct 17, 2022

View reviewed changes

codebien dismissed stale reviews from mstoykov, olegbespalov, and na-- via 0b07133 October 17, 2022 10:27

codebien force-pushed the metrics-metadata branch from 45b0181 to 0b07133 Compare October 17, 2022 10:27

na-- previously approved these changes Oct 17, 2022

View reviewed changes

codebien dismissed na--’s stale review via 1ce73e9 October 17, 2022 10:31

codebien force-pushed the metrics-metadata branch from 0b07133 to 1ce73e9 Compare October 17, 2022 10:31

na-- previously approved these changes Oct 17, 2022

View reviewed changes

mstoykov previously approved these changes Oct 17, 2022

View reviewed changes

na-- and others added 2 commits October 17, 2022 12:44

Support non-indexable high-cardinality metric tags/metadata

6663efe

Remove experimental xk6-websockets

3c97ec7

Remove for a temporary commit the experimental module to allow bumping the k6 dependency in the extension's repository.

codebien dismissed stale reviews from mstoykov and na-- via 3c97ec7 October 17, 2022 11:00

codebien force-pushed the metrics-metadata branch from 1ce73e9 to 3c97ec7 Compare October 17, 2022 11:00

Bump xk6-websockets version

776c476

Uses the [email protected] version that is now migrated to the new metadata for non-indexable tags.

na-- mentioned this pull request Oct 17, 2022

Support non-indexable tags (Metadata) grafana/xk6-websockets#14

Merged

mstoykov approved these changes Oct 17, 2022

View reviewed changes

olegbespalov approved these changes Oct 17, 2022

View reviewed changes

na-- approved these changes Oct 17, 2022

View reviewed changes

codebien merged commit 0568f27 into master Oct 17, 2022

codebien deleted the metrics-metadata branch October 17, 2022 13:04

codebien added this to the v0.41.0 milestone Oct 17, 2022

na-- added the breaking change for PRs that need to be mentioned in the breaking changes section of the release notes label Oct 28, 2022

na-- mentioned this pull request Nov 10, 2022

JS API for user-specified high-cardinality metrics metadata #2766

Closed

immavalls mentioned this pull request Feb 3, 2023

Document the metadata column in the CSV output grafana/k6-docs#925

Open

TomRiddle01 mentioned this pull request Mar 1, 2023

Option to use separate name and url tags (Possibly via raw_url) #2950

Open

sid-maddy pushed a commit to sid-maddy/k6 that referenced this pull request Oct 23, 2023

feat: reinstate the raw_url non-indexable tag

903fd32

See grafana#2950 and grafana#2726 (comment) for more information. Signed-off-by: Siddhesh Mhadnak <[email protected]>

sid-maddy added a commit to sid-maddy/k6 that referenced this pull request Oct 25, 2023

feat: reinstate the raw_url non-indexable tag

bb23d04

See grafana#2950 and grafana#2726 (comment) for more information. Signed-off-by: Siddhesh Mhadnak <[email protected]>

mstoykov mentioned this pull request Mar 14, 2024

sse: support Server Sent Event #3639

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

codebien commented Oct 13, 2022 •

edited

Loading

mstoykov left a comment

mstoykov Oct 13, 2022

codebien Oct 13, 2022

mstoykov Oct 13, 2022

na-- Oct 13, 2022

codebien Oct 13, 2022 •

edited

Loading

na-- Oct 13, 2022

codebien Oct 17, 2022

na-- Oct 16, 2022

codebien Oct 17, 2022

na-- Oct 17, 2022

na-- left a comment

olegbespalov left a comment

olegbespalov Oct 17, 2022

codebien commented Oct 17, 2022 •

edited

Loading

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

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

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

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

Conversation

codebien commented Oct 13, 2022 • edited Loading

mstoykov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codebien Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

na-- left a comment

Choose a reason for hiding this comment

olegbespalov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codebien commented Oct 17, 2022 • edited Loading

codebien commented Oct 13, 2022 •

edited

Loading

codebien Oct 13, 2022 •

edited

Loading

codebien commented Oct 17, 2022 •

edited

Loading