Skip to content
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

prometheus: Remove namespace on attributes #3634

Open
gouthamve opened this issue Aug 1, 2023 · 8 comments
Open

prometheus: Remove namespace on attributes #3634

gouthamve opened this issue Aug 1, 2023 · 8 comments
Labels
spec:metrics Related to the specification/metrics directory triage:deciding:community-feedback Open to community discussion. If the community can provide sufficient reasoning, it may be accepted

Comments

@gouthamve
Copy link
Member

gouthamve commented Aug 1, 2023

From @bertysentry at prometheus/prometheus#12571 (comment)

For example, for JVM metrics: the process.runtime.jvm.gc.duration metric will come with the process.runtime.jvm.gc.name
and process.runtime.jvm.gc.action attributes. This will end up as process_runtime_jvm_gc_duration_seconds_counter{process_runtime_jvm_gc_name="...", process_runtime_jvm_gc_action="..."} in PromQL.

We could decide to simplify process_runtime_jvm_gc_name and process_runtime_jvm_gc_action labels to name and action respectively by removing the matching namespaces in metric name and its labels.

The benefit is that we get metrics and labels that look much like "classic" Prometheus metrics and labels. The problem is that the behavior could be counter-intuitive and difficult to predict in some cases (which labels get simplified or not?).


It is a good point and I think also exporting the namespaces in every label makes it extremely cumbersome to work with and query the data. Could we make it part of the Proemtheus compatibility spec to not send the namespace in the labels to Prometheus?

@gouthamve gouthamve added the spec:metrics Related to the specification/metrics directory label Aug 1, 2023
@Oberon00
Copy link
Member

Oberon00 commented Aug 1, 2023

Have you seen open-telemetry/semantic-conventions#51? I think a decision on that topic was already made

@dashpole
Copy link
Contributor

dashpole commented Aug 1, 2023

I think the proposal is that prometheus exporters strip namespaces from metric labels if they match the namespace of the metric name

@gouthamve
Copy link
Member Author

Yup, this is for the Prometheus compatibility spec. I tried to make it slightly clear in the description as well. Sorry for the confusion.

@bertysentry
Copy link
Contributor

Or... we could set up a coup against convince 😅 the TC that the decision to prefix all attributes in metrics was non-optimal, and will imply a lot of CPU cycles just to remove these in Prometheus and all its clones, which means more energy consumed, and more carbon in the atmosphere, which is bad.

The decision was a close call (only a relative majority of 4/9 voted for prefixing all attributes with the metric's namespace). And nothing has actually been implemented yet. It's not too late to change our mind, nothing's set in stone, after all.

Note: At this point of my comment, I think I need to clearly say that I will abide by the decision of the TC on this. I'm just highlighting a new reason to discuss the decision made.

@gouthamve
Copy link
Member Author

Also, if we remove the prefix on export, do we need to reintroduce back during scraping? I don't know.

@jsuereth
Copy link
Contributor

@bertysentry We were aware of the prometheus concerns when we voted.

Regarding removing the prefix on export to prometheus:

Since we do have programatic access to all attributes and metric names in semconv, would it make sense to have some kind of codegen in semconv that could create an optimal metric-transform for Prometheus exporters that only does namespace transformation on OTEL metrics?

I imagine we could do some kind of lookup-table of transformations instead of string-prefix searches, in addition to limiting the "surprise" to users for their own metrics where they may not be using namespaces but accidentally fall afoul of namespace-stripping logic.

@jsuereth
Copy link
Contributor

jsuereth commented Aug 15, 2023

Another point I want to make is namespacing changes in otel. We're moving towards "reasonably unique" namespaces, so all your process.runtime.jvm examples become jvm. The underlying concern still exists, but it's slightly better:

jvm_gc_duration_seconds_counter{jvm_gc_name="...", jvm_gc_action="...", ...}

@bertysentry
Copy link
Contributor

@jsuereth The lookup table from the semconv is a good idea. However, we will still need to implement the logic for all metrics that are not covered by semconv (and there will always be plenty).

WRT "reasonably unique" namespaces, I think it's a good thing, I mean, it make things less horrible ;-)

@trask trask added the triage:deciding:community-feedback Open to community discussion. If the community can provide sufficient reasoning, it may be accepted label May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:metrics Related to the specification/metrics directory triage:deciding:community-feedback Open to community discussion. If the community can provide sufficient reasoning, it may be accepted
Projects
None yet
Development

No branches or pull requests

7 participants