k6 icon indicating copy to clipboard operation
k6 copied to clipboard

Support non-indexable high-cardinality metric tags/metadata

Open na-- opened this issue 2 years ago • 1 comments

This is built on top of https://github.com/grafana/k6/pull/2594 and should close https://github.com/grafana/k6/issues/2584 and fix https://github.com/grafana/k6/issues/2653 when it's done.

A bunch of tests still remain to be ported or written, and some of the outputs need to be adjusted, but it should be suitable for initial review even now. Just keep in mind this: https://github.com/grafana/k6/issues/2584#issuecomment-1219618111

na-- avatar Aug 18 '22 15:08 na--

Actually, besides addressing PR comments and maybe a few more tests, I am not sure if anything else is needed in this PR here :thinking: So I'll remove the draft status of this PR, it's officially ready for reviews :tada:

na-- avatar Aug 19 '22 12:08 na--

For anyone watching, after some internal discussions, we are likely not going to merge this PR as it is.

We are cannibalizing it for parts and trying to merge the parts nobody objects to (most of the Go internals and refactoring, and the Metadata map in metrics.Sample) in k6 v0.41.0, while leaving the rest (any JS API changes that are not purely refactoring) for further consideration in the future. See https://github.com/grafana/k6/issues/2584#issuecomment-1269775631 for more details.

na-- avatar Oct 06 '22 10:10 na--

I rebased the code with the new changes from master. There is a backup branch in case you would check the original version.

Hmm :thinking: it would have probably been simpler and safer to keep the old branch in this PR and just close it (without merging) and open a new PR with the new version. Because, when you have a PR with a branch, you can safely delete and then restore the branch without any fears. But if the free-standing backup-non-indexable-tags branch is deleted, it won't be recoverable, so it will just clutter the branches list.

I also removed the previously added metrics.TagRawURL after an internal discussion. We will open a dedicated issue for it. There is also a small change in the latest fixup from the original version.

The reason the original code was like that was to optimize it slightly. In your code, strconv.Itoa() and strconv.FormatBool() will always be called, even when they are not needed (e.g. when the tags are disabled). It's a very minor optimization, since these tags are enabled by default, so the readability improvements are probably more important.

na-- avatar Oct 13 '22 06:10 na--

@olegbespalov can you please comment on the new PR?

codebien avatar Oct 13 '22 08:10 codebien

@codebien, just for my understanding, will this PR be closed? Shouldn't I check them anymore? :thinking:

olegbespalov avatar Oct 13 '22 08:10 olegbespalov

Yes, we are going to rollback this PR to the previous history and close then move in two more defined PRs.

codebien avatar Oct 13 '22 09:10 codebien

Closing this for the reasons stated in https://github.com/grafana/k6/pull/2654#issuecomment-1269779558 and https://github.com/grafana/k6/issues/2584#issuecomment-1269775631

In short, we merged the internal parts of this PR in other PRs (https://github.com/grafana/k6/pull/2716, https://github.com/grafana/k6/pull/2726, https://github.com/grafana/k6/pull/2727) and we will continue iterating on a better solution for the JS APIs that expose the high-cardinality metrics.Sample.Metadata for user scripts.

na-- avatar Oct 19 '22 13:10 na--