utf8: enable utf-8 support by default #14705
Conversation
5d0dcde to
91f20f9
Compare
91f20f9 to
d640d3f
Compare
|
need to fix some tests |
cmd/prometheus/main.go
Outdated
| case "utf8-names": | ||
| model.NameValidationScheme = model.UTF8Validation | ||
| level.Info(logger).Log("msg", "Experimental UTF-8 support enabled") | ||
| case "legacy-names": |
There was a problem hiding this comment.
Does this mean 2.x has --enable-feature=utf8-names and 3.x will have --enable-feature=legacy-names with utf8 on by default?
There was a problem hiding this comment.
2.x will have a feature called utf8-names available but it will not be set by default. 3.x will not have a feature flag for it, utf8 will be default-on and it can be disabled through the global or scrapeconfig flags
cmd/prometheus/main.go
Outdated
| @@ -461,7 +463,7 @@ func main() { | |||
| a.Flag("scrape.discovery-reload-interval", "Interval used by scrape manager to throttle target groups updates."). | |||
| Hidden().Default("5s").SetValue(&cfg.scrape.DiscoveryReloadInterval) | |||
|
|
|||
| a.Flag("enable-feature", "Comma separated feature names to enable. Valid options: agent, auto-gomemlimit, exemplar-storage, expand-external-labels, memory-snapshot-on-shutdown, promql-per-step-stats, promql-experimental-functions, extra-scrape-metrics, new-service-discovery-manager, auto-gomaxprocs, no-default-scrape-port, native-histograms, otlp-write-receiver, created-timestamp-zero-ingestion, concurrent-rule-eval. See https://prometheus.io/docs/prometheus/latest/feature_flags/ for more details."). | |||
| a.Flag("enable-feature", "Comma separated feature names to enable. Valid options: agent, auto-gomemlimit, exemplar-storage, expand-external-labels, memory-snapshot-on-shutdown, promql-per-step-stats, promql-experimental-functions, extra-scrape-metrics, new-service-discovery-manager, auto-gomaxprocs, no-default-scrape-port, native-histograms, otlp-write-receiver, created-timestamp-zero-ingestion, concurrent-rule-eval, legacy-names. See https://prometheus.io/docs/prometheus/latest/feature_flags/ for more details."). | |||
There was a problem hiding this comment.
enable-feature is for experimental features. legacy-names is neither breaking nor experimental. I think this should be a flag of its own or something else. Another argument can be made that in the future we want to always have utf8 enabled in Prometheus and remove the legacy-names, in which case feature flags are better. @beorn7 WDYT?
There was a problem hiding this comment.
See the below comment; we figured we don't need the flag afterall
There was a problem hiding this comment.
After some offline chat with @ywwg we figured we don't need the flag at all. The config file has two levels to manage the utf8 - global and at scrape level. Global config already controls the utf8 for all the scrapes, so the flag is redundant. utf8 will be on by default, and users can simply set the global config to legacy when migrating to 3.0.
We need tests to check the scrape level config overriding global config when set.
cbfc63a to
fd6c2ee
Compare
config/config.go
Outdated
| if model.NameValidationScheme != model.UTF8Validation { | ||
| return fmt.Errorf("utf8 name validation requested but feature not enabled via --enable-feature=utf8-names") | ||
| return fmt.Errorf("utf8 name validation requested but validation mode is not set to UTF8") |
There was a problem hiding this comment.
That's not what we want, is it?
We want the user to be able to specify legacy validation globally, but then this error would fire.
I think we only need to check the case that an invalid validation scheme is provided.
This has implications for the tests below.
There was a problem hiding this comment.
see @codesome 's comment: #14705 (review)
Legacy mode should only be set in the global config or scrape config, while the common library itself always needs to be set to validate UTF-8 by default or those settings won't work. This conditional is a check that there is not a programming error.
There was a problem hiding this comment.
I see. Of course, you are not comparing to the config setting here.
In that case, let's just panic. That's fine (or actually most appropriate) for a programming error. Returning an error from the config validation certainly suggests there is an error in the config (not in the code).
6e4311d to
7b408ad
Compare
This change causes Prometheus to allow all UTF-8 characters in metric and label names. This means that names that were previously invalid and would have been previously rejected will be allowed through. Signed-off-by: Owen Williams <owen.williams@grafana.com>
7b408ad to
88bb05c
Compare
This change causes Prometheus to allow all UTF-8 characters in metric and label names. This means that names that were previously invalid and would have been previously rejected will be allowed through.
Updates a number of tests to make sure we are still testing legacy behavior where needed.
part of #13095
fixes #14674