k6/metrics: Warn on metric names not compatible with Prometheus #3066

mstoykov · 2023-05-11T11:23:39Z

This adds a warning for custom metrics that are not Prometheus compatible.

The warning is in the k6/metrics as the alternative was threading a logger in the registry specifically for this and then removing it.

The major downside of this is that extension can still register incompatible metric names without any warnings.

Updates #3065

mstoykov · 2023-05-11T11:26:02Z

While I was writing this I started wondering if an alternative approach of checking this after the initial init context run might not be more beneficial.

The major problem is the registry can't currently return all the metric names, but it will let us catch extensions registering "bad" names

codecov-commenter · 2023-05-11T11:33:02Z

Codecov Report

❗ No coverage uploaded for pull request base (registry-fetch-all@74a2e13). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head c4047a6 differs from pull request most recent head 34e263e. Consider uploading reports for the commit 34e263e to get more accurate results

@@                  Coverage Diff                  @@
##             registry-fetch-all    #3066   +/-   ##
=====================================================
  Coverage                      ?   76.73%           
=====================================================
  Files                         ?      229           
  Lines                         ?    17130           
  Branches                      ?        0           
=====================================================
  Hits                          ?    13145           
  Misses                        ?     3141           
  Partials                      ?      844

Flag	Coverage Δ
ubuntu	`76.73% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

codebien · 2023-05-11T13:12:57Z

but it will let us catch extensions registering "bad" names.

Yeah, I see value into this. I prefer an approach that includes extensions so we don't create shadow zones.

the registry can't currently return all the metric names

Do you mean something like this? b485b22

mstoykov · 2023-05-11T13:19:21Z

@codebien yeah that seems like a thing I can use.

Is that PR likely to be merged soon ™️ . If not can we pull this function out and then rebase both of our PRs on top of it?

codebien · 2023-05-11T13:20:35Z

Is that PR likely to be merged soon tm .

Not really

If not can we pull this function out and then rebase both of our PRs on top of it?

If you want to use it, sure. Creating the PR.

codebien

LGTM (with test fixed), just a minor comment

js/bundle.go

codebien

LGTM

imiric

Compiling the regex outside of newBundle is the only blocker for me.

But I have a general concern about this change. Shouldn't we only limit metric names if the Prometheus or OTel outputs are used? That is, wouldn't it make more sense to have this warning and eventual failure in the outputs themselves? It seems unnecessary to break metric names for all k6 users, even if they're not using Prometheus/OTel.

If we want to suggest people stick to this restriction anyway, then just a warning would be enough, without the breaking change in v0.48.0. But even a general warning would be annoying if someone is not using Prometheus/OTel.

imiric · 2023-05-12T08:34:39Z

js/bundle.go

+// TODO: this is to be removed once this is not a warning and it can be moved to the registry
+func (b *Bundle) checkMetricNamesForPrometheusCompatibility() {
+	const (
+		nameRegexString = "^[a-zA-Z_][a-zA-Z0-9_]{1,128}$"


I didn't see a length limit on Prometheus' metric names in their documentation, but OTel limits it to 63 chars.

I'm not sure if we should follow whatever Prometheus' limit is, or OTel's, but 63 chars is way too restricting. So... neither?

To be honest I did copy our length restriction mostly as this change is supposed to tighten the name in a way to be usable by as many outside systems as possible. And it still needs to actually be subset of what we currently support.

but OTel limits it to 63 chars.

I didn't see that 🤦

To be honest 63 characters are quite a lot for a metric name to be honest.

MyReallyCoolCounterThatCountsRedEyedPuppies is only 43.

So to be honest I am okay with us going even down to 63 characters 🤷

cc @dgzlopes any thoughts?

True, 63 chars ought to be enough for anybody.:tm: :smile:

It's probably a safe limit to impose. Even the browser's long Web Vital metrics like webvital_first_contentful_paint_good is 36 chars, and going much above that would be messy to show in our CLI and Cloud UIs.

So if we want to be strict with OTel compatibility, we might also limit it to 63.

By default, Prometheus has no limits (I think) (related: prometheus/prometheus#1235 (comment))

Is there any technical reason for us to set a limit?

js/bundle.go

mstoykov · 2023-05-12T12:40:01Z

Compiling the regex outside of newBundle is the only blocker for me.

I will change that if my comment there is not enough - I really have no problem with that.

But I have a general concern about this change. Shouldn't we only limit metric names if the Prometheus or OTel outputs are used? That is, wouldn't it make more sense to have this warning and eventual failure in the outputs themselves? It seems unnecessary to break metric names for all k6 users, even if they're not using Prometheus/OTel.

If we want to suggest people stick to this restriction anyway, then just a warning would be enough, without the breaking change in v0.48.0. But even a general warning would be annoying if someone is not using Prometheus/OTel.

I feel like I have not explained the change very well. Most systems working with metrics have some metric name restrictions. Of all of the Prometheus and Otel seem to be the one that get the most traction.

Anyone who uses k6 and custom metrics and output to one of those systems will have some problems if k6 allows characters that are not supported by the other system.

AFAIK the current k6 name restriction is mostly what it is because that is what we came up with.

So this PR tries to make it so that k6 metric names will be supported by any output system irregardless of whether the users use them or not.

As the issues says we will pivot in case there is a lot of user feedback that they want spaces (for example, also the character that started this).

In practice this warning is a way to ask users for feedback in case they really want to keep something, until we actually deprecate it.

cc @dgzlopes if you want to add more.

codebien · 2023-05-12T12:51:47Z

In practice this warning is a way to ask users for feedback in case they really want to keep something, until we actually deprecate it.

If this is the attempt, then I would like to insert a call to action in the message explicitly requesting negative feedback.

Something like:
if this does not work for your business, let we know here (the link to the issue with the v0.48 milestone for final removal)

dgzlopes · 2023-05-12T13:09:25Z

cc @dgzlopes if you want to add more.

I think you covered all 👍

imiric · 2023-05-12T13:11:13Z

@mstoykov Yeah, I see the benefit of restricting this to some common set of characters that would be compatible with most metric backends. I was just wondering whether it made sense to force the change on users who don't use outputs that would be impacted by this.

But OK, no blockers from me. Let's just agree on the length.

mstoykov · 2023-05-12T13:26:30Z

@codebien I think the message is already long enough. And if users see a message that tells them that something that they need will not work in 2-3 versions, I would expect that:

they will change it if it is easy for them, and they do not care - WIN
actually come to tell us as in 2-3 versions they will not be able to use us.

We have been getting reports of this kind for a while without a big message, so I prefer to not make the message even longer.

js/bundle.go

sniku · 2023-05-12T14:20:15Z

Eventually, we will store k6-cloud time-series data in Mimir, so we should enforce the metric name restrictions for all k6 tests (not only for those that use Prometheus output).

👍 from me.

js/bundle.go

This adds a warning for custom metrics that are not Prometheus compatible. The warning is in the js/bundle mostly for convenience. It is a place we already have a registry and a logger and we go through only once. As this is temporary warning until we enforce stricter metric names I don't think it is good idea to refactor the registry to support this for a little while. Updates #3065

mstoykov · 2023-06-08T07:52:43Z

Documentation added in grafana/k6-docs#1205

mstoykov added this to the v0.45.0 milestone May 11, 2023

github-actions bot requested review from imiric and oleiade May 11, 2023 11:23

mstoykov requested review from codebien and removed request for oleiade May 11, 2023 11:26

codebien mentioned this pull request May 11, 2023

metrics: Return all registered metrics #3067

Merged

mstoykov force-pushed the warnOnMetricNames branch from db92213 to b6b2214 Compare May 11, 2023 15:29

mstoykov changed the base branch from master to registry-fetch-all May 11, 2023 15:29

codebien reviewed May 11, 2023

View reviewed changes

js/bundle.go Outdated Show resolved Hide resolved

codebien previously approved these changes May 12, 2023

View reviewed changes

imiric suggested changes May 12, 2023

View reviewed changes

mstoykov commented May 12, 2023

View reviewed changes

js/bundle.go Outdated Show resolved Hide resolved

olegbespalov previously approved these changes May 12, 2023

View reviewed changes

mstoykov requested a review from imiric May 12, 2023 15:58

imiric previously approved these changes May 12, 2023

View reviewed changes

Base automatically changed from registry-fetch-all to master May 15, 2023 08:01

codebien dismissed stale reviews from olegbespalov and themself via 34e263e May 15, 2023 08:01

mstoykov requested a review from codebien May 15, 2023 08:06

codebien reviewed May 15, 2023

View reviewed changes

js/bundle.go Show resolved Hide resolved

mstoykov dismissed imiric’s stale review via 1d9c60c May 15, 2023 08:21

mstoykov force-pushed the warnOnMetricNames branch from 34e263e to 1d9c60c Compare May 15, 2023 08:21

mstoykov requested review from codebien, imiric and olegbespalov May 15, 2023 08:30

olegbespalov approved these changes May 15, 2023

View reviewed changes

codebien approved these changes May 15, 2023

View reviewed changes

mstoykov merged commit 218a173 into master May 15, 2023
22 checks passed

mstoykov deleted the warnOnMetricNames branch May 15, 2023 11:50

olegbespalov added the documentation-needed A PR which will need a separate PR for documentation label Jun 6, 2023

mstoykov mentioned this pull request Sep 18, 2023

Restrict metric names to what is supported by Prometheus #3065

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

k6/metrics: Warn on metric names not compatible with Prometheus #3066

k6/metrics: Warn on metric names not compatible with Prometheus #3066

mstoykov commented May 11, 2023

mstoykov commented May 11, 2023

codecov-commenter commented May 11, 2023 •

edited

codebien commented May 11, 2023

mstoykov commented May 11, 2023

codebien commented May 11, 2023 •

edited

codebien left a comment •

edited

codebien left a comment

imiric left a comment •

edited

imiric May 12, 2023

mstoykov May 12, 2023

imiric May 12, 2023

dgzlopes May 12, 2023

mstoykov commented May 12, 2023

codebien commented May 12, 2023

dgzlopes commented May 12, 2023 •

edited

imiric commented May 12, 2023

mstoykov commented May 12, 2023

sniku commented May 12, 2023 •

edited

mstoykov commented Jun 8, 2023

k6/metrics: Warn on metric names not compatible with Prometheus #3066

k6/metrics: Warn on metric names not compatible with Prometheus #3066

Conversation

mstoykov commented May 11, 2023

mstoykov commented May 11, 2023

codecov-commenter commented May 11, 2023 • edited

Codecov Report

codebien commented May 11, 2023

mstoykov commented May 11, 2023

codebien commented May 11, 2023 • edited

codebien left a comment • edited

Choose a reason for hiding this comment

codebien left a comment

Choose a reason for hiding this comment

imiric left a comment • edited

Choose a reason for hiding this comment

imiric May 12, 2023

Choose a reason for hiding this comment

mstoykov May 12, 2023

Choose a reason for hiding this comment

imiric May 12, 2023

Choose a reason for hiding this comment

dgzlopes May 12, 2023

Choose a reason for hiding this comment

mstoykov commented May 12, 2023

codebien commented May 12, 2023

dgzlopes commented May 12, 2023 • edited

imiric commented May 12, 2023

mstoykov commented May 12, 2023

sniku commented May 12, 2023 • edited

mstoykov commented Jun 8, 2023

codecov-commenter commented May 11, 2023 •

edited

codebien commented May 11, 2023 •

edited

codebien left a comment •

edited

imiric left a comment •

edited

dgzlopes commented May 12, 2023 •

edited

sniku commented May 12, 2023 •

edited