fix(prometheusreceiver): invalid metric name validation error in scrape start from target allocator #40803
Conversation
This test checks target allocator + scrape e2e. The test fails if: - Prometheus logged something above the WARN level - Prometheus reports failed scrape pools in metrics - No metrics are in the result Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Set sane defaults if target allocator didn't return value for metric_name_validation_scheme metric_name_escaping_scheme The defaults are "legacy" and "underscores" Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
krajorama
changed the title
| if scrapeConfig.MetricNameValidationScheme == "" { | ||
| scrapeConfig.MetricNameValidationScheme = promconfig.LegacyValidationConfig | ||
| } | ||
|
|
||
| if scrapeConfig.MetricNameEscapingScheme == "" { | ||
| scrapeConfig.MetricNameEscapingScheme = model.EscapeUnderscores | ||
| } | ||
|
|
There was a problem hiding this comment.
Sorry, the default behavior is a little more complex: default validation scheme is actually UTF8. And then the escaping scheme is "allow-utf-8" if the validation scheme is UTF8, and is "underscores" if the validation scheme is "legacy"
https://github.com/prometheus/prometheus/blob/main/config/config.go#L874-L920
There was a problem hiding this comment.
I've reused the Validation code from Prometheus to set these instead, so that the code is not duplicated here.
dashpole
dismissed
their stale review
We should update the default configuration in prometheus instead
|
We've discussed this in the OTEL Prometheus WG meeting. The real problem is that Prometheus doesn't set the correct defaults itself. It's ok to make this workaround for now, linked to a new issue in Prometheus, however I need to update the test so that it's creating the base scrape configuration from Prometheus defaults and not manually as in my test code. |
in the target allocator when we receive a scrape config. This makes sure that globals are applied and defaults are set. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
|
I think I found a nice solution to not duplicate logic from Prometheus , however I don't have a testbed for the target allocator operator, so it's very hard for me to tell if I'm causing some other problem. I imagine it's fair to validate the scrape config that the target allocator provides, but I'm not 100% sure. @dashpole @ywwg |
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
| } | ||
|
|
||
| // Validate the scrape config and also fill in the defaults from the global config as needed. | ||
| err = scrapeConfig.Validate(m.promCfg.GlobalConfig) |
There was a problem hiding this comment.
yes this is the correct approach
| // https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/35459 | ||
| // is implemented and is default. | ||
| if m.promCfg.GlobalConfig.MetricNameValidationScheme == "" { | ||
| m.promCfg.GlobalConfig.MetricNameValidationScheme = promconfig.LegacyValidationConfig |
There was a problem hiding this comment.
note that upstream, the default will be UTF-8. If otel collector wants a different default they will have to override it somewhere.
|
I couldn't find time to review but I'm trusting david and owen on this one :) I'm adding the ready to merge label |
ArthurSens
added
the
ready to merge
Description
Fix invalid metric name validation error in scrape start from target allocator.
Prometheus made setting metric_name_validation_scheme, metric_name_escaping_scheme mandatory mandatory, use sane defaults ("legacy", "undescores").
Link to tracking issue
Fixes #40788
Testing
Added unit test for target allocator + scrape e2e.
The test fails if:
Documentation
N/A