Bump future metric name length to 128 #3335
Conversation
mstoykov
force-pushed
the
bumpMetricNameLenRestriction
branch
from
b8fa5f7 to
4ecb7e7
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #3335 +/- ##
==========================================
+ Coverage 72.99% 73.16% +0.17%
==========================================
Files 261 258 -3
Lines 19993 19912 -81
==========================================
- Hits 14593 14568 -25
+ Misses 4476 4420 -56
Partials 924 924
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
|
Apparently, there is also:
Are we going to include it? |
|
That is not allowed in prometheus - so no. |
| @@ -46,8 +46,8 @@ type Bundle struct { | |||
| // https://github.com/grafana/k6/issues/3065 | |||
| func (b *Bundle) checkMetricNamesForPrometheusCompatibility() { | |||
| const ( | |||
| nameRegexString = "^[a-zA-Z_][a-zA-Z0-9_]{1,63}$" | |||
| badNameWarning = "Metric name should only include ASCII letters, numbers and underscores. " + | |||
| nameRegexString = "^[a-zA-Z_][a-zA-Z0-9_]{1,128}$" | |||
There was a problem hiding this comment.
| nameRegexString = "^[a-zA-Z_][a-zA-Z0-9_]{1,128}$" | |
| nameRegexString = "^[a-zA-Z_][a-zA-Z0-9_]{1,255}$" |
Is there a specific reason behind it? If not then I would prefer if we could keep it aligned with OTel.
There was a problem hiding this comment.
Yes that is the current limit in the registry. The place where this limit will go after this becomes an error instead of a warning.
There was a problem hiding this comment.
My suggestion was about the size limit s/128/255/
There was a problem hiding this comment.
Yes this is hte current limit - 128
Line 29 in 981b770
There was a problem hiding this comment.
Sorry, but I'm not still getting why we are not just bumping everything to 255. What is the reason to keep as 128? If there is one technical then I would like us adding a comment on the registry's regex explaining it. Prometheus doesn't have any limit and the OpenTelemetry limit is set to 255 so it sounds to me we should just match the OTel limit.
olegbespalov
added
the
documentation-needed
This is inline with the current restriction. The previous 63 character one came from Otel, but with open-telemetry/opentelemetry-specification#3648 This has been bumped to 255. Updates #3065
mstoykov
force-pushed
the
bumpMetricNameLenRestriction
branch
from
cde37e3 to
8aa1c8f
Compare
Changes in code done in grafana/k6#3335
|
Docs updates in grafana/k6-docs#1331 |
Changes in code done in grafana/k6#3335
This is inline with the current restriction.
The previous 63 character one came from Otel, but with open-telemetry/opentelemetry-specification#3648 This has been bumped to 255.
Updates #3065
Changelog
breaking: future metric names will be limited to 128 instead of 63 characters