Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make label name character set validation optional #2835

Merged

Conversation

replay
Copy link
Contributor

@replay replay commented Jul 3, 2020

This removes the SkipLabelValidation distributor config property again and introduces a new flag called -validation.enforce-label-name-charset. That config flag can be used to disable the enforcement of the prometheus character set restrictions when validating label names, to allow for a wider set of characters in label names.
The advantage of doing it this way is that the enforcement of the label name charset can be disabled specifically, while all the other label limits (-validation.max-length-label-name, -validation.max-length-label-value, etc) remain in place.

Signed-off-by: replay <mauro.stettler@gmail.com>
@replay replay changed the title Make label name validation optional Make label name character set validation optional Jul 3, 2020
@@ -38,6 +38,7 @@ type Limits struct {
CreationGracePeriod time.Duration `yaml:"creation_grace_period"`
EnforceMetadataMetricName bool `yaml:"enforce_metadata_metric_name"`
EnforceMetricName bool `yaml:"enforce_metric_name"`
EnforceLabelNameCharset bool `yaml:enforce_label_name_charset"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to expose this option to Cortex users? Or is it supposed to be only internal flag used by extension you’re writing? (I would prefer later, but if there is good reason to expose, we can keep it)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same concern here. I would prefer to avoid making it configurable to Cortex users unless there's a legit use case for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, no problem, i'll remove the flag so that the setting isn't exposed to the user. I'll add a parameter to validation.ValidateLabels() then, to skip the label name validation. I'll also extend the unit tests to test that flag

Signed-off-by: replay <mauro.stettler@gmail.com>
@replay replay force-pushed the make_label_name_validation_optional branch from b48ab32 to c74b572 Compare July 6, 2020 16:20
@pull-request-size pull-request-size bot added size/M and removed size/S labels Jul 6, 2020
@replay
Copy link
Contributor Author

replay commented Jul 6, 2020

One test has failed in the CI, but when I run it locally it passes fine. I can't trigger a re-run without write permissions, but is it possible that this test is unstable (TestSweepImmediateDropsSamples)?

@pracucci
Copy link
Contributor

pracucci commented Jul 7, 2020

One test has failed in the CI, but when I run it locally it passes fine. I can't trigger a re-run without write permissions, but is it possible that this test is unstable (TestSweepImmediateDropsSamples)?

Yes, this test is flaky. I've re-opened the issue about it #2825. Definitely not something you have to fix.

Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! (modulo a nit)

pkg/distributor/distributor.go Outdated Show resolved Hide resolved
if cfg.EnforceMetricName(userID) {
metricName, err := extract.MetricNameFromLabelAdapters(ls)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this (moving it inside the branch)!

Signed-off-by: replay <mauro.stettler@gmail.com>
@pracucci pracucci merged commit d2f37d3 into cortexproject:master Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants