Parameterized metric/label name validation scheme #946

juliusmh · 2025-07-22T08:49:33Z

This PR introduces explicit APIs to set validation scheme for various components of prometheus.

ywwg

this looks like a good approach. I have not looked at the relabel code before, it does seem like we will want to backport the new config to upstream prometheus. Does that sound right?

ywwg · 2025-07-22T17:50:58Z

model/relabel/relabel.go

 	// Action is the action to be performed for the relabeling.
 	Action Action `yaml:"action,omitempty" json:"action,omitempty"`
+	// MetricNameValidationScheme to use when validating labels.
+	MetricNameValidationScheme model.ValidationScheme `yaml:"-" json:"-"`


Oo I hadn't noticed this. If this is needed here then we probably want to do this in upstream prometheus as well, right?

The idea is to upstream changes after we've battle tested them a bit in mimir-prometheus.

model/relabel/relabel.go

juliusmh · 2025-07-23T11:39:12Z

model/relabel/relabel.go

 	}
-	if c.Action == Replace && !varInRegexTemplate(c.TargetLabel) && !model.LabelName(c.TargetLabel).IsValid() {
+
+	if c.MetricNameValidationScheme == model.UnsetValidation {


See above. This check might not be necessary. If scheme is Unset, then IsValidLabelName below uses NameValidationScheme.

It's better to be robust I think. IsValidLabelName could always change to be less lenient.

aknuds1

I think it looks good at this point. Awaiting @dimitarvdimitrov's review though.

dimitarvdimitrov · 2025-07-23T14:45:54Z

model/rulefmt/rulefmt.go

 				node: &node.Record,
 			})
 		}
-		if !model.IsValidMetricName(model.LabelValue(r.Record)) {


why not change the implementation of model.IsValidMetricName instead of creating a new function with the same name in a separate package?

That would require changing prometheus/common/model.IsValidMetricName's signature wouldn't it? That would be a much more intrusive change, due to all the dependencies on prometheus/common/model.IsValidMetricName throughout the whole ecosystem.

That would require changing prometheus/common/model.IsValidMetricName's signature wouldn't it?

yes, but it would at least give us some safety. Right now nothing prevent something on the write path to use model.IsValidMetricName instead of labels.IsValidMetricName.

The alternative would be to ban the use of model.IsValidmetricName and model.LabelName.IsValid() in Prometheus, and always use the explicit functions instead together with UTF8Validation.

this sounds better. we should do the same in mimir too

Also it feels like labels.IsValidMetricName should be in model too, maybe named differently. Right now, it's a bit confusing which one you're supposed to use (or you have to wait for the linter to tell you you're using the wrong one)

yes, but it would at least give us some safety.

We started out by doing that, but it required forking a lot of dependencies etc. Not sure how it would affect the greater ecosystem on top of that.

model/relabel/relabel.go

dimitarvdimitrov · 2025-07-23T14:53:07Z

model/relabel/relabel.go

 		// UTF-8 allows ${} characters, so standard validation allow $variables by default.
 		// TODO(bwplotka): Relabelling users cannot put $ and ${<...>} characters in metric names or values.
 		// Design escaping mechanism to allow that, once valid use case appears.
-		return model.LabelName(value).IsValid()


what about uses of the old model.IsValidmetricName and model.LabelName.IsValid()? Are you going to parametarize those too?

IMO having both globally-set and parametarized behaviours is confusing and will likely lead to bugs

This is the compromise we came up with I'm afraid. The rationale is that only the write path is supposed to be configurable wrt. the name validation scheme, and we will otherwise always use the UTF-8 validation mode. The global variable is not supposed to be used, and it's deprecated (staticcheck catches if you use it).

The alternative would be to ban the use of model.IsValidmetricName and model.LabelName.IsValid() in Prometheus, and always use the explicit functions instead together with UTF8Validation.

model/relabel/relabel_test.go

model/rulefmt/rulefmt.go

model/rulefmt/rulefmt_test.go

notifier/manager.go

Copilot

Pull Request Overview

This PR introduces parameterized metric/label name validation schemes throughout the Prometheus codebase. The changes allow different components to explicitly specify and use either legacy validation or UTF-8 validation for metric and label names, moving away from a global default validation scheme.

Key changes include:

Addition of explicit MetricNameValidationScheme field to relabel configurations
New validation functions in the labels package that accept validation scheme parameters
Updates to rule parsing to support configurable validation schemes through options
Comprehensive test updates to specify validation schemes explicitly

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`model/labels/validate.go`	New validation functions that accept validation scheme parameters
`model/labels/validate_test.go`	Tests for the new parameterized validation functions
`model/relabel/relabel.go`	Added MetricNameValidationScheme field and updated validation logic
`model/relabel/relabel_test.go`	Updated tests to set validation schemes explicitly
`model/rulefmt/rulefmt.go`	Refactored parsing to accept validation scheme options
`model/rulefmt/rulefmt_test.go`	Updated tests to use new parsing API with options
`scrape/scrape.go`	Added validation scheme checks in scrape pool creation and reload
`scrape/scrape_test.go`	Set validation schemes in test configurations
`notifier/manager.go`	Added validation scheme handling for relabel configurations
`rules/manager.go`	Updated to use new rulefmt parsing API

scrape/scrape.go

model/relabel/relabel.go

notifier/manager.go

dimitarvdimitrov

lgtm, we agreed you guys will add some linting to prevent using the model versions in favour of the parametarized labels versions

aknuds1 · 2025-07-25T11:59:46Z

lgtm, we agreed you guys will add some linting to prevent using the model versions in favour of the parametarized labels versions

Thanks @dimitarvdimitrov, yes. Maybe as part of the upstreaming.

dimitarvdimitrov · 2025-07-25T12:07:21Z

Maybe as part of the upstreaming.
i thought you were starting to work on this? https://raintank-corp.slack.com/archives/C0909GTDAUV/p1753362194915909?thread_ts=1753358923.288489&cid=C0909GTDAUV

Signed-off-by: Arve Knudsen <[email protected]>

Co-authored-by: Julius Hinze <[email protected]> Signed-off-by: Arve Knudsen <[email protected]>

Signed-off-by: Arve Knudsen <[email protected]>

…n to NewManager/ApplyConfig Signed-off-by: Arve Knudsen <[email protected]>

Signed-off-by: Arve Knudsen <[email protected]>

aknuds1 · 2025-07-25T12:24:42Z

thought you were starting to work on this?

@dimitarvdimitrov Yes, making an upstreaming PR is next on my list. Already chatted a bit with @ywwg on potential improvements.

#### 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: - [x] Reach consensus on whether `streamingpromqlcompat.NameValidatingEngine` is the right approach: Decided to do UTF8 validation in query path. - [x] Get Alertmanager updated with fix, stop using our own fork #### Which issue(s) this PR fixes or relates to Depends on - grafana/mimir-prometheus#946 - grafana/prometheus-alertmanager#116 Fixes: #11503 #### Checklist - [x] Tests updated. - [ ] Documentation added. - [x] `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`](https://github.com/grafana/mimir/blob/main/docs/sources/mimir/configure/about-versioning.md) updated with experimental features. --------- Signed-off-by: Julius Hinze <[email protected]> Co-authored-by: Arve Knudsen <[email protected]>

…validation" This reverts commit c4bd4fa, reversing changes made to 6dd7af9.

juliusmh requested a review from aknuds1 July 22, 2025 08:49

This was referenced Jul 22, 2025

remove global name validation #892

Closed

Parameterize metric/label name validation scheme grafana/mimir#11848

Merged

ywwg reviewed Jul 22, 2025

View reviewed changes

juliusmh commented Jul 23, 2025

View reviewed changes

aknuds1 force-pushed the arve/parameterized-name-validation branch from a0a70e9 to 7fe8e08 Compare July 23, 2025 13:29

aknuds1 approved these changes Jul 23, 2025

View reviewed changes

aknuds1 requested review from Copilot and dimitarvdimitrov July 23, 2025 13:59

This comment was marked as outdated.

Sign in to view

dimitarvdimitrov reviewed Jul 23, 2025

View reviewed changes

aknuds1 force-pushed the arve/parameterized-name-validation branch from 79860ff to 0d9a872 Compare July 24, 2025 12:50

aknuds1 requested review from Copilot and dimitarvdimitrov July 24, 2025 13:48

Copilot AI reviewed Jul 24, 2025

View reviewed changes

scrape/scrape.go Outdated Show resolved Hide resolved

model/relabel/relabel.go Show resolved Hide resolved

aknuds1 force-pushed the arve/parameterized-name-validation branch from 5dd1258 to e835981 Compare July 24, 2025 13:49

ywwg reviewed Jul 24, 2025

View reviewed changes

model/relabel/relabel.go Show resolved Hide resolved

notifier/manager.go Show resolved Hide resolved

dimitarvdimitrov approved these changes Jul 25, 2025

View reviewed changes

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

feat(storage): add option to specify name scheme for remote read/write

c8f1b91

Signed-off-by: Arve Knudsen <[email protected]>

promql: Always use UTF8Validation

5c39c28

Signed-off-by: Arve Knudsen <[email protected]>

Update model/relabel/relabel.go

be87cfc

Co-authored-by: Julius Hinze <[email protected]> Signed-off-by: Arve Knudsen <[email protected]>

model/rulefmt: Default to model.NameValidationScheme

3e8bf70

Signed-off-by: Arve Knudsen <[email protected]>

notifier: Default to model.NameValidationScheme

1d1d136

Signed-off-by: Arve Knudsen <[email protected]>

notifier.Manager: Move RelabelConfigs validation scheme initializatio…

a441380

…n to NewManager/ApplyConfig Signed-off-by: Arve Knudsen <[email protected]>

Fix UnsetValidation checking

22e3084

Signed-off-by: Arve Knudsen <[email protected]>

Remove no-op change

0317525

Signed-off-by: Arve Knudsen <[email protected]>

Make ignoreUnknownFields an option

b6662cd

Signed-off-by: Arve Knudsen <[email protected]>

Add ParseFile legacy validation success case

3d26e64

Signed-off-by: Arve Knudsen <[email protected]>

Remove no-op change

45de439

Signed-off-by: Arve Knudsen <[email protected]>

aknuds1 force-pushed the arve/parameterized-name-validation branch from 7ef6515 to 45de439 Compare July 25, 2025 12:08

notifier.NewManager: Log warning when using default scheme

944844e

Signed-off-by: Arve Knudsen <[email protected]>

juliusmh merged commit c4bd4fa into main Jul 25, 2025
13 checks passed

juliusmh deleted the arve/parameterized-name-validation branch July 25, 2025 12:33

aknuds1 added a commit that referenced this pull request Jul 28, 2025

Revert "Merge pull request #946 from grafana/arve/parameterized-name-…

df24e70

…validation" This reverts commit c4bd4fa, reversing changes made to 6dd7af9.

aknuds1 added a commit that referenced this pull request Aug 4, 2025

Revert "Merge pull request #946 from grafana/arve/parameterized-name-…

8ad7765

…validation" This reverts commit c4bd4fa, reversing changes made to 6dd7af9.

Parameterized metric/label name validation scheme #946

Parameterized metric/label name validation scheme #946

Uh oh!

Conversation

juliusmh commented Jul 22, 2025

Uh oh!

ywwg left a comment

Choose a reason for hiding this comment

Uh oh!

ywwg Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

aknuds1 Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

juliusmh Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

aknuds1 Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

aknuds1 left a comment • 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!

This comment was marked as outdated.

Uh oh!

dimitarvdimitrov Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

aknuds1 Jul 23, 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!

dimitarvdimitrov Jul 23, 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!

aknuds1 Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dimitarvdimitrov Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

aknuds1 Jul 23, 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!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dimitarvdimitrov left a comment

Choose a reason for hiding this comment

Uh oh!

aknuds1 commented Jul 25, 2025

Uh oh!

dimitarvdimitrov commented Jul 25, 2025

Uh oh!

aknuds1 commented Jul 25, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

aknuds1 left a comment •

edited

Loading

aknuds1 Jul 23, 2025 •

edited

Loading

dimitarvdimitrov Jul 23, 2025 •

edited

Loading

aknuds1 Jul 23, 2025 •

edited

Loading