-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Should prometheus metric name normalization be exposed as a configuration option? #21743
Comments
Pinging code owners:
See Adding Labels via Comments if you do not have permissions to add labels yourself. |
We have a working group meeting in two weeks if you would like to discuss it there. If not, I'll do so. |
Which working group meeting are you referring to specifically? |
Collector Sig Discussion on 10 May 2023
Please add anything that I may have missed. |
This change is effectively breaking any existing Grafana dashboard you get from various projects and vendors giving you ultimately 2 options
It will also probably go for any AlertingRule you get from upstream |
From the perspective of a user wanting to use the prometheus receiver as a drop-in replacement for Prometheus, this is a very disruptive change. The Prometheus -> Otel normalization is very simple, as it only trims unit and I support making this a configuration option. And if we actually want users to migrate, we should consider providing a more gradual migration path as well. |
This also affects us in the googlemanagedprometheus exporter, both on the read side from the receiver and on export because we rely on the normalization functions in the Collector. |
Summary from prometheus wg this morning:
I will break down this summary into a list of action items. After doing that we can determine action item owners and start moving forward. |
@swiatekm-sumo are you saying that prometheus -> otel translation converts |
To be exact, Prometheus receiver calls |
Thanks for the detailed info. This may be worthy of an issue in itself, I'll take a deeper look, maybe we are missing some context. I'll add it to the list of action items that I will build around metric name normalization. |
Sorry I haven't been part of relevant discussions, as i'm still on leave. The intent of the feature gate was to be transparent to users when using a prometheus receiver and exporter. Translation from prometheus naming to otel naming is done in the receiver, and the reverse is done in prometheus exporters. It should not be breaking for anyone using a prometheus receiver and exporter as a drop-in replacement for the prometheus server. The transition is intended to be gradual, and the feature gate was in alpha (disabled by default) for 11 months. It can still be disabled with |
This would mean that there is a guarantee that metric pipelines with The prometheus receiver readme says:
What does minimal drop in mean here? Does performing normalization by default in the receiver push it further from that goal? I believe @swiatekm-sumo brought up a good use case where this would not be desired. The dropping of the |
I agree that this should be a configuration option. There are many scenarios (not prom->otel->prom) when the full normalization can be confusing. Another concern is that some standard Prometheus metrics potentially can be mapped 1:1 to standard OTel metrics defined in the spec. Would covering those cases be complete normalization? I'm a bit skeptical about going further that route and would vote for making simple normalization a default option.
I submitted a PR to document the Prometheus->Otel transition #23209. Also, filed an issue about the limited translations on the receiver side #23208
I don't think we should complicate it like this as long as the Prometheus exporter produces the same result for metrics both normalized and not normalized on the receiver side, which is the case AFAIK, e.g., it doesn't add the |
Hi, I have a lot more experience with the exporter side of things, than the receiver side of things. And in the exporter, not having full normalization is quite bad imo. For example, take the case of a metric being exported via OTLP Recv --> PRW today (full normalisation OFF): For this reason, I think we should enable full normalisation by default on the exporter as soon as possible. It is an inevitable change, and something that is more painful the longer it takes as it will break more users. Coming to the normalisation on the receiver end, it is a tricky question. Lets us consider the following usecases:
Whether recevier does full normalisation or not doesn't matter here as long as exporter does the full normalisation.
With normalisation it becomes:
Some names are unfortunate though:
So I am kinda split on the receiver normalisation. It makes sense for a lot of the metrics, but there is also a set of metrics where its a bit unfortunate. |
Just found this: prometheus/prometheus#12152 If this is merged, people could see warnings for OTel metrics ( |
…4256) **Description:** Adds a new configuration option: `trim_metric_suffixes` to the prometheus receiver. When set to true, suffixes will be trimmed from metrics. This replaces use of the`pkg.translator.prometheus.NormalizeName` feature-gate in the prometheus receiver, but leaves it for exporters. The first commit simplifies the usage of the feature-gate. The tests and implementation were passing around an entire registry when it wasn't necessary. **Link to tracking Issue:** Part of #21743 Part of #8950
…24260) **Description:** Add add_metric_suffixes configuration option, which can disable the addition of type and unit suffixes. This is backwards-compatible, since the default is true. This is recommended by the specification for sum suffixes in https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/compatibility/prometheus_and_openmetrics.md#sums and allowed in metadata https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/compatibility/prometheus_and_openmetrics.md#metric-metadata-1: `Exporters SHOULD provide a configuration option to disable the addition of _total suffixes.` `The resulting unit SHOULD be added to the metric as OpenMetrics UNIT metadata and as a suffix to the metric name unless the metric name already contains the unit, or the unit MUST be omitted` This deprecates the BuildPromCompliantName function in-favor of BuildCompliantName, which includes the additional argument for configuring suffixes. **Link to tracking Issue:** Fixes #21743 Part of #8950 --------- Co-authored-by: Dmitrii Anoshin <anoshindx@gmail.com>
Component(s)
exporter/prometheus, exporter/prometheusremotewrite, receiver/prometheus
Describe the issue you're reporting
In #20519 the
pkg.translator.prometheus.NormalizeName
was enabled by default causing full prometheus metric name normalization to be used instead of simple. The intent of feature gates is that they will eventually be removed and thus the option to revert back to simple name normalization will be lost.I believe that simple vs full name normalization should be a choice by the operator of the collector. It is not clear to me if full name normalization makes sense in all use cases.
Specifically I am wondering if the permanent enabling of this flag causes the OpenTelemetry Prometheus receiver to move further away from a
drop in replacement
for the prometheus server. Does the prometheus server perform this same type offull metric name normalization
by default?The text was updated successfully, but these errors were encountered: