Parameterize metric/label name validation scheme #11848

juliusmh · 2025-06-24T18:11:18Z

What this PR does

Direct and indirect references to the global name validation scheme were removed in favor of a per-tenant override.

Distributors have a validation middleware that ensures metric and label names are valid.
Rulers validate rule(group)s using this naming scheme.
Queriers use UTF8 validation everywhere.

TODO:

Reach consensus on whether streamingpromqlcompat.NameValidatingEngine is the right approach: Decided to do UTF8 validation in query path.
Get Alertmanager updated with fix, stop using our own fork

Which issue(s) this PR fixes or relates to

Depends on

Fixes: #11503

Checklist

Tests updated.
Documentation added.
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]. If changelog entry is not needed, please add the changelog-not-needed label to the PR.
about-versioning.md updated with experimental features.

juliusmh · 2025-06-24T18:36:20Z

pkg/mimirtool/rules/rules.go

@@ -255,7 +256,8 @@ func (r RuleNamespace) Validate(groupNodes []rulefmt.RuleGroupNode) []error {
 func ValidateRuleGroup(g rwrulefmt.RuleGroup, node rulefmt.RuleGroupNode) []error {
 	var errs []error
 	for i, r := range g.Rules {
-		for _, err := range r.Validate(node.Rules[i]) {
+		// TODO(juliusmh):
+		for _, err := range r.Validate(node.Rules[i], validation.LegacyNamingScheme) {


Q: How should we handle validation checks in mimirtool?

I can think about it after the initial review :D

aknuds1

OK, gave it a quick review. Nice work! Please check my questions :)

pkg/cardinality/request.go

pkg/cardinality/request_test.go

pkg/distributor/validate_test.go

pkg/mimir/modules.go

pkg/querier/querier.go

pkg/streamingpromql/compat/name_validating_engine.go

aknuds1 · 2025-06-25T16:57:25Z

pkg/streamingpromql/compat/name_validating_engine.go

+		if e.limits.NameValidationScheme(tenantID) == prom_validation.LegacyNamingScheme {
+			return prom_validation.LegacyNamingScheme, nil
+		}


Isn't it bad to enforce the legacy name validation scheme on the query path? All it will do is prevent the user from querying any metrics they ingested in the past with the UTF-8 name validation scheme?

This PR tries to maintain the current behavior, and currently, we validate label names in label_join, label_replace and count_values with legacy scheme. Queries that don't use those functions support UTF8, today. This can be seen in the tests I did.

The question we have to answer: is there any scenario in which data returned by a querier or frontend is fed back to mimir without validation (e.g. to ingesters directly, or with label validation disabled)? If the answer is no, we can safely remove the validation. I'm assuming the answer is yes though, based on this comment.

@charleskorn WDYT?

I believe that data will always be validated when written back by rulers. They don't skip validation.

However, I believe we should validate names in at least label_replace and label_join, as not validating may lead to unexpected results:

functions that consume the results of label_replace and label_join may have undefined behaviour with invalid label names

a user testing a recording or alerting rule as a query may not realise that their rule will later fail to write samples when evaluated by rulers

pkg/streamingpromql/operators/aggregations/count_values_test.go

pkg/util/validation/limits.go

charleskorn · 2025-06-30T02:19:44Z

pkg/cardinality/request.go

@@ -262,10 +263,10 @@ func extractLabelNames(values url.Values) ([]model.LabelName, error) {

 	labelNames := make([]model.LabelName, 0, len(labelNamesParams))
 	for _, labelNameParam := range labelNamesParams {
-		labelName := model.LabelName(labelNameParam)
-		if !labelName.IsValid() {
+		if !validation.UTF8NamingScheme.IsValidLabelName(labelNameParam) {


Should the naming scheme used here be configurable?

I am wondering in this case whether this code should use a parameter (instead of hard coding), but not let it be user configurable since, as I explain above, I'm not sure we want to enforce the legacy naming scheme in queries. WDYT @charleskorn?

I don't know enough about this code path to know what makes sense here. Does Prometheus apply validation to the label names here as well?

Does Prometheus apply validation to the label names here as well?

@juliusmh Did you check?

In any case, this is an example of read path isn't it, where we don't want to enforce the "legacy" naming scheme? If so, I think it could a non-configurable parameter/argument, just to avoid hard coding.

Afaik, the api/v1/cardinality/* endpoints are Mimir specific.

The closest is probably db stats in prometheus admin API (calculated here), where you can get some analysis on label name/value cardinality, which, as far as I can see, doesn't involve any validation. But I'm not super confident in that analysis.

pkg/mimir/modules.go

pkg/streamingpromql/operators/functions/factories.go

aknuds1

I think this is starting to look good. We should keep that test case though.

pkg/querier/cardinality_analysis_handler_test.go

pkg/streamingpromql/planning.go

pkg/streamingpromql/query.go

juliusmh · 2025-07-21T10:37:30Z

As discussed @aknuds1 :

Synced with WIP: Distributor: Refactor metric/label name validation to not use global variable #12109
Fixed failing tests

@charleskorn :

Could you elaborate? It seems like adding it to promql.QueryOpts would be a more natural fit rather than creating a wrapper like this (at least for Mimir, not sure about Prometheus).

For promql, the main problem is that query opts are directly constructed and passed to query engine in prometheus' v1.NewApi. Unless we want to rewrite that handler we need to somehow "modify" promql.QueryOptions, IMO the QueryEngine interface is the only viable place.

You're right, for MQE this can be implemented differently, see this commit. The wrapper, is/was a way of unifying this logic for promql/mimir query engine paths.

dimitarvdimitrov · 2025-07-23T14:42:16Z

oh, also the mimirtool changes i think need a changelog entry because they're a change in behaviour

@dimitarvdimitrov I think there may have been a misunderstanding here? We are postponing mimirtool (and other) behavioral changes until a follow-up PR.

you're right 👍

aknuds1

@dimitarvdimitrov requests a test case for an invalid label name in the cardinality API. Could you add that @juliusmh?

Also, could you add a CHANGE type changelog entry about how the read path now supports all valid UTF-8 strings at least for the label value cardinality API (/api/v1/cardinality/label_values)? Please see Slack discussion regarding the latter.

pkg/frontend/querymiddleware/request_validation_test.go

juliusmh · 2025-07-23T17:01:00Z

Given this was a bit chaotic, quick summary of what was addressed:

refactored distributor's validation tests
added more tests to MetricsQueryRequestValidationRoundTripper

and what is still open:

Here, I don't see which case is missing.
Here, should I add labelMatcher/selector validation to LabelsQueryRequestValidationRoundTripper? We didn't do that previously.
Here, Move this relabel config modification to a better place. Do you have any ideas where that could be?
I added some Changelog messages, but needs refining

CHANGELOG.md

dimitarvdimitrov · 2025-07-24T09:52:36Z

thanks @juliusmh . i think only the last thread still needs resolving. i left a suggestion

tacole02 · 2025-07-24T19:18:44Z

CHANGELOG.md

+* [CHANGE] Query-frontend: Add support for utf8 label/metric names in `/api/v1/cardinality/{label_values|label_values|active_series}` endpoints. #11848.
+* [CHANGE] Querier: Add support for utf8 metric names, support utf8 label names in `label_join`, `label_replace` and `count_values` PromQL functions. #11848.


Suggested change

* [CHANGE] Query-frontend: Add support for utf8 label/metric names in `/api/v1/cardinality/{label_values|label_values|active_series}` endpoints. #11848.

* [CHANGE] Querier: Add support for utf8 metric names, support utf8 label names in `label_join`, `label_replace` and `count_values` PromQL functions. #11848.

* [CHANGE] Query-frontend: Add support for UTF-8 metric and label names in `/api/v1/cardinality/{label_values|label_values|active_series}` endpoints. #11848.

* [CHANGE] Querier: Add support for UTF-8 metric and label names in `label_join`, `label_replace`, and `count_values` PromQL functions. #11848.

aknuds1

I see what looks like an accidental test change. Also please review my suggestions.

pkg/frontend/querymiddleware/request_validation_test.go

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

…(rather than NameValidatingEngine)

…oundTripper

aknuds1

You need to update mimir-prometheus.

go.mod

aknuds1

LGTM!

juliusmh force-pushed the juliusmh/remove_global_name_validation branch 3 times, most recently from e47a7e5 to 78ba72c Compare June 24, 2025 18:33

juliusmh commented Jun 24, 2025

View reviewed changes

juliusmh force-pushed the juliusmh/remove_global_name_validation branch from 78ba72c to 549cafd Compare June 24, 2025 22:05

juliusmh requested a review from aknuds1 June 25, 2025 08:33

aknuds1 requested a review from Copilot June 25, 2025 16:29

This comment was marked as outdated.

Sign in to view

aknuds1 reviewed Jun 25, 2025

View reviewed changes

juliusmh force-pushed the juliusmh/remove_global_name_validation branch 3 times, most recently from c1310a3 to 3888883 Compare June 26, 2025 16:22

charleskorn reviewed Jun 30, 2025

View reviewed changes

aknuds1 self-requested a review June 30, 2025 07:21

juliusmh force-pushed the juliusmh/remove_global_name_validation branch 5 times, most recently from 756ef29 to 448dde2 Compare July 4, 2025 11:47

aknuds1 previously requested changes Jul 4, 2025

View reviewed changes

pkg/querier/cardinality_analysis_handler_test.go Outdated Show resolved Hide resolved

juliusmh force-pushed the juliusmh/remove_global_name_validation branch 2 times, most recently from 35aeae6 to cc723b6 Compare July 7, 2025 09:17

juliusmh mentioned this pull request Jul 17, 2025

Move validation into Mimir (remove global name validation) #12113

Closed

4 tasks

juliusmh force-pushed the juliusmh/remove_global_name_validation branch from cc723b6 to 936bd5c Compare July 21, 2025 10:00

juliusmh changed the base branch from main to r352 July 21, 2025 10:02

juliusmh commented Jul 21, 2025

View reviewed changes

pkg/streamingpromql/planning.go Outdated Show resolved Hide resolved

juliusmh commented Jul 21, 2025

View reviewed changes

pkg/streamingpromql/query.go Outdated Show resolved Hide resolved

juliusmh force-pushed the juliusmh/remove_global_name_validation branch from 936bd5c to 0522664 Compare July 21, 2025 10:41

juliusmh changed the base branch from r352 to main July 21, 2025 11:50

aknuds1 reviewed Jul 23, 2025

View reviewed changes

pkg/frontend/querymiddleware/request_validation_test.go Show resolved Hide resolved

juliusmh force-pushed the juliusmh/remove_global_name_validation branch from 6fc01ca to b83fffe Compare July 23, 2025 16:27

dimitarvdimitrov reviewed Jul 24, 2025

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

juliusmh force-pushed the juliusmh/remove_global_name_validation branch 2 times, most recently from 72ecc48 to 25e0428 Compare July 24, 2025 12:58

tacole02 approved these changes Jul 24, 2025

View reviewed changes

aknuds1 reviewed Jul 25, 2025

View reviewed changes

juliusmh force-pushed the juliusmh/remove_global_name_validation branch from 25e0428 to 5cc2c4b Compare July 25, 2025 09:49

dimitarvdimitrov approved these changes Jul 25, 2025

View reviewed changes

juliusmh changed the title ~~Remove global name validation scheme~~ Parameterize metric/label name validation scheme Jul 25, 2025

juliusmh and others added 10 commits July 25, 2025 14:37

chore: remove global name validation scheme

d958d17

Don't change prometheus/common API

ed9101a

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

chore: fix failing tests

2347f89

streamingpromql: use label/metric name validation scheme from limits …

e8ab43d

…(rather than NameValidatingEngine)

deps: update alertmanager; prometheus-mimir

a37b3be

streamingpromql: always use UTF8Validation

f47e6c9

distributor: refactor legacy/utf validation tests

a600d4d

querymiddleware: utf8/legacy tests for MetricsQueryRequestValidationR…

b079740

…oundTripper

chore: update changelog

63c1a4e

util/validation: set relabel config validation scheme

65bc6f8

aknuds1 reviewed Jul 25, 2025

View reviewed changes

go.mod Outdated Show resolved Hide resolved

juliusmh force-pushed the juliusmh/remove_global_name_validation branch from 5cc2c4b to 65bc6f8 Compare July 25, 2025 12:44

aknuds1 approved these changes Jul 25, 2025

View reviewed changes

aknuds1 added enhancement component/query-frontend labels Jul 25, 2025

juliusmh merged commit c4dc705 into main Jul 25, 2025
35 checks passed

juliusmh deleted the juliusmh/remove_global_name_validation branch July 25, 2025 13:14

		* [CHANGE] Query-frontend: Add support for utf8 label/metric names in `/api/v1/cardinality/{label_values\|label_values\|active_series}` endpoints. #11848.
		* [CHANGE] Querier: Add support for utf8 metric names, support utf8 label names in `label_join`, `label_replace` and `count_values` PromQL functions. #11848.

Parameterize metric/label name validation scheme #11848

Parameterize metric/label name validation scheme #11848

Uh oh!

Conversation

juliusmh commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

aknuds1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juliusmh Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aknuds1 Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juliusmh Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aknuds1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

juliusmh commented Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dimitarvdimitrov commented Jul 23, 2025

Uh oh!

aknuds1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

juliusmh commented Jul 23, 2025

Uh oh!

Uh oh!

dimitarvdimitrov commented Jul 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aknuds1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

juliusmh commented Jun 24, 2025 •

edited

Loading

juliusmh Jun 26, 2025 •

edited

Loading

aknuds1 Jul 1, 2025 •

edited

Loading

juliusmh Jul 1, 2025 •

edited

Loading

juliusmh commented Jul 21, 2025 •

edited

Loading