-
Notifications
You must be signed in to change notification settings - Fork 6
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
Reconsider the approach of applying the full normalization #72
Comments
Discussed this issue in the Prometheus WG meeting. Q 1: @gouthamve mentioned that Prometheus is going to support dots in metric names going forward, so, potentially, we can restore OTel metric names in the OTel -> Prom -> OTel scenario by applying the full normalization. I'm still not sure if it justifies changing all the Prometheus metrics in the Prometheus receiver because of the reasons mentioned above ( Q 2: Look like there are no objections to making the normalization on the Prometheus exporter side configurable and having it enabled by default. |
Sorry I missed the discussion yesterday; I'm just getting back from leave and am catching up now. The response to enabling the feature gate makes me think our approach might have to change. If we tried to make the change at a future point, we would likely run into the same pushback, and i'm not sure the value we are getting for the change is worth the pain it would cause users right now. My suggestion is that we enable normalization on prometheus exporters by default, but disable it on the prometheus receiver by default. It would essentially make only additive changes by default. Prom -> OTel -> Prom would still round trip successfully in that case, but OTel -> Prom -> OTel would end up with extra suffixes in addition to underscores. To round-trip OTel -> Prom -> OTel, a user would need to:
Since we want OTel SDK prometheus exporters to work with older prometheus servers, sanitization ( I'm not a fan of encoding/decoding information from/to the HELP text. It seems likely to break, and pollutes the help text with information users are unlikely to care about. |
I like it. Echoes what I said here: open-telemetry/opentelemetry-collector-contrib#21743 (comment) |
@dashpole I agree with this in general. But WDYT about the suggestion I mentioned in my previous comment about adding an OTel metric hint to the HELP section by the Prometheus exporter. For example, if any metric has |
I'm not a fan of encoding/decoding information from/to the HELP text. It seems likely to break, and pollutes the help text with information users are unlikely to care about. |
OK, let's proceed with the configuration options for the full normalization, which will be disabled on the receiver and enabled on the exporter. We probably don't need a feature gate for that, and the existing one can be deprecated. |
This has been completed in the collector |
We recently attempted to activate the pkg.translator.prometheus.NormalizeName feature gate in the collector. This feature gate applies full metrics normalization by default in both the Prometheus receiver and exporter. However, the process did not go smoothly and was seen as an unnecessary disruptive change. Two issues were reported regarding this matter:
pkg.translator.prometheus.NormalizeName
feature gate opentelemetry-collector-contrib#23208After discussing the topic in the Collector SIG on June 6th, a decision was made to revert the feature gate back to its alpha state until both issues are resolved.
To initiate the discussion towards resolving these problems, I have two questions:
1. Why do we need to change the original Prometheus metrics names in the Prometheus receiver?
While it is beneficial to populate as many fields as possible in an OTel metric object from the available information in a Prometheus endpoint, including the metric unit, changing the original metric name seems unnecessary. Unless we have predefined mappings, we cannot convert a Prometheus metric name into an OTel metric with a name compliant with the OTel spec. By simply removing the unit and
_total
suffixes, we end up with metric names that may be unfamiliar to users. For example, a commonly used Prometheus metric,process_cpu_seconds_total
, would becomeprocess_cpu
after the full normalization, which lacks references in both OTel and Prometheus contexts. However, if we retain the nameprocess_cpu_seconds_total
in the OTel metric name (withs
as the unit), it is evident where the metric originates.If we decide against changing the original name in the Prometheus receiver:
system.cpu.time
would become an OTel metric namedsystem_cpu_time_seconds_total
instead ofsystem_cpu_time
. I believe bothsystem_cpu_time_seconds_total
andsystem_cpu_time
metric names should be acceptable since it is not possible to revert back tosystem.cpu.time
.2. Do we want to provide an option for the end user to disable/enable the full normalization in the Prometheus exporter? If so, what would be the default setting?
Unlike the Prometheus receiver, the full normalization in the Prometheus exporter is important as it prevents the loss of the unit field value during conversion. However, there might be situations where users prefer to avoid additional suffixes and keep the metric name as close to the original OTel name as possible. Considering the feedback from open-telemetry/opentelemetry-collector-contrib#21743, it seems reasonable to provide this option. If we do include it, what should be the default behavior? It is plausible that the default setting should be the enabled full normalization, but alternative viewpoints are welcome.
The text was updated successfully, but these errors were encountered: