Normalize Prometheus metric names for OTLP exporter and update tests#2262
Normalize Prometheus metric names for OTLP exporter and update tests#2262rafaelwestphal wants to merge 13 commits into
Conversation
quentinmit
left a comment
quentinmit
left a comment
There was a problem hiding this comment.
What is our migration plan for cleaning up this tech debt in the future? What will we do when customers ask us to support utf-8 metrics or when GMP starts supporting them by default?
| pipeline.Processors["metrics"] = append(pipeline.Processors["metrics"], otel.MetricUnknownCounter()) | ||
| pipeline.Processors["metrics"] = append(pipeline.Processors["metrics"], otel.PrometheusMetricNormalize()) | ||
| // If a metric already has a domain, it will not be considered a prometheus metric by the UTR endpoint unless we add the prefix. | ||
| // This behavior is the same as the GCM/GMP exporters. | ||
| pipeline.Processors["metrics"] = append(pipeline.Processors["metrics"], otel.MetricsTransform(otel.AddPrefix("prometheus.googleapis.com"))) |
There was a problem hiding this comment.
Nit: These three can all be a single call to append with four arguments.
Added a toggle to allow Cx to enable the utf-8 metrics. Default is off, but we can switch when we want to change the behavior. |
| // 1. Replace all non-compliant characters (except :) with underscore | ||
| `replace_pattern(metric.name, "[^a-zA-Z0-9:]", "_")`, | ||
| // 2. Collapse multiple consecutive underscores | ||
| `replace_pattern(metric.name, "_+", "_")`, |
There was a problem hiding this comment.
AFAICT from reading the upstream code, multiple consecutive underscores are only collapsed when WithMetricSuffixes is true. That's set to the AddMetricSuffixes config from the exporter, which we set to false.
So instead, I think we're in this case:
// Simple case (no full normalization, no units, etc.).
metricName := strings.Join(strings.FieldsFunc(name, func(r rune) bool {
return !isValidCompliantMetricChar(r) && r != '_'
}), "_")
// Namespace?
if mn.Namespace != "" {
namespace := strings.Join(strings.FieldsFunc(mn.Namespace, func(r rune) bool {
return !isValidCompliantMetricChar(r) && r != '_'
}), "_")
normalizedName = namespace + "_" + metricName
return
}
// Metric name starts with a digit? Prefix it with an underscore.
if metricName != "" && unicode.IsDigit(rune(metricName[0])) {
metricName = "_" + metricName
}
normalizedName = metricName
returnThis does not collapse multiple underscores, nor does it remove a leading or trailing underscore.
I think you want, instead of 1-4,
replace_pattern(metric.name, "[^a-zA-Z0-9:_]+", "_").
which replaces consecutive invalid characters with a single underscore, without touching any existing underscores.
| converter = confgenerator.ConvertPrometheusExporterToOtlpExporter | ||
| } | ||
| return []otel.ReceiverPipeline{converter(otel.ReceiverPipeline{ | ||
| AllowUTF8: (r.AllowUTF8 != nil && *r.AllowUTF8), |
There was a problem hiding this comment.
Why is this a separate field in the ReceiverPipeline struct instead of the logic just being added to metricsProcessors?
|
|
||
| GRPCEndpoint string `yaml:"grpc_endpoint" validate:"omitempty,hostname_port" tracking:"endpoint"` | ||
| MetricsMode string `yaml:"metrics_mode" validate:"omitempty,oneof=googlecloudmonitoring googlemanagedprometheus" tracking:""` | ||
| AllowUTF8 *bool `yaml:"allow_utf8" validate:"omitempty"` |
There was a problem hiding this comment.
I believe this option also applies to the Prometheus receiver. But either way, we should think hard about whether and how we want to add a config knob for this. (If we want to eventually have UTF-8 on for everyone, any config knob should probably be experimental.)
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
2 participants
Description
The original behavior of prometheus metric name is to replace dots and slashes with _. Currently otlp_exporter implementation breaks that behavior. Adding a processor to fix the breaking change.
Related issue
b/476109839
How has this been tested?
Checklist: