scrape: set validation and escaping defaults in default config vars #16751
Conversation
|
What about the following approach? diff --git a/config/config.go b/config/config.go
index a15bcfcf7..87568e876 100644
--- a/config/config.go
+++ b/config/config.go
@@ -172,9 +172,8 @@ var (
ScrapeProtocols: DefaultScrapeProtocols,
ConvertClassicHistogramsToNHCB: false,
AlwaysScrapeClassicHistograms: false,
- // Matching the behavior in Validate(), default to UTF-8.
- MetricNameValidationScheme: UTF8ValidationConfig,
- MetricNameEscapingScheme: model.AllowUTF8,
+ MetricNameValidationScheme: UTF8ValidationConfig,
+ MetricNameEscapingScheme: model.AllowUTF8,
}
DefaultRuntimeConfig = RuntimeConfig{
@@ -191,9 +190,6 @@ var (
HonorTimestamps: true,
HTTPClientConfig: config.DefaultHTTPClientConfig,
EnableCompression: true,
- // Matching the behavior in Validate(), default to UTF-8.
- MetricNameValidationScheme: UTF8ValidationConfig,
- MetricNameEscapingScheme: model.AllowUTF8,
}
// DefaultAlertmanagerConfig is the default alertmanager configuration.
@@ -784,10 +780,6 @@ func (c *ScrapeConfig) SetDirectory(dir string) {
// UnmarshalYAML implements the yaml.Unmarshaler interface.
func (c *ScrapeConfig) UnmarshalYAML(unmarshal func(interface{}) error) error {
*c = DefaultScrapeConfig
- // Unset default validation and escaping scheme values so they can inherit
- // from the global config if they are blank in the file.
- c.MetricNameValidationScheme = ""
- c.MetricNameEscapingScheme = ""
if err := discovery.UnmarshalYAMLWithInlineConfigs(c, unmarshal); err != nil {
return err
}
@@ -950,6 +942,13 @@ func (c *ScrapeConfig) MarshalYAML() (interface{}, error) {
// ToValidationScheme returns the validation scheme for the given string config value.
func ToValidationScheme(s string) (validationScheme model.ValidationScheme, err error) {
switch s {
+ case "":
+ // This is a workaround for third party exporters that don't set the validation scheme.
+ if DefaultGlobalConfig.MetricNameValidationScheme == "" {
+ // Prevents infinite recursion.
+ panic("global metric name validation scheme is not set")
+ }
+ return ToValidationScheme(DefaultGlobalConfig.MetricNameValidationScheme)
case UTF8ValidationConfig:
validationScheme = model.UTF8Validation
case LegacyValidationConfig: |
|
yeah I think that can work -- I'll try it |
|
looking good, and actually the wrapper for fixing escaping scheme works very nicely. Where is the right place to add a comment that people need to call Validate? |
|
Maybe expand this comment to say that defaulting is done during validate? Line 184 in bdada23 Or add it to the type comment above DefaultScrapeConfig. |
|
I think the best is probably in the type comment above DefaultScrapeConfig ; as Validate does more than just this. |
I'm tempted to unexport DefaultScrapeConfig (after a deprecation window) and instead create NewDefaultScrapeConfig() that creates a valid object with those defaults. We can do that in a subsequent PR though. |
7ac65dd to
4033c3c
Compare
4033c3c to
76f5cea
Compare
Fixes #16750 Signed-off-by: Owen Williams <owen.williams@grafana.com>
76f5cea to
1ce7d48
Compare
| } | ||
|
|
||
| DefaultRuntimeConfig = RuntimeConfig{ | ||
| // Go runtime tuning. | ||
| GoGC: getGoGC(), | ||
| } | ||
|
|
||
| // DefaultScrapeConfig is the default scrape configuration. | ||
| // DefaultScrapeConfig is the default scrape configuration. Users of this | ||
| // default MUST call Validate() on the config after creation, even if it's |
There was a problem hiding this comment.
This is unclear to me. What is "the config"? I can't find a call to Validate() on this object in Prometheus.
Maybe you meant something like "MUST call Validate() on any ScrapeConfig object, even a copy of DefaultScrapeConfig" ?
the most important thing is the linked bug which in turn links to an otel bug that happened because of this: open-telemetry/opentelemetry-collector-contrib#40788 Because the required fields were not set, they got a panic when the code executed on the invalid config object. |
|
So yes, this should be part of 3.5 |
|
I don't understand how a bug in a different project means we need this change in Prometheus 3.5. |
|
the effect of the bug was in another project, but the cause was a prometheus regression preventing a user from upgrading their collector. I recommend reading the upstream bug for the full context. |
Fixes #16750
Updated tests to rely on the existence of those values