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 exporter add duplicated suffix for counter metrics #5304

Closed
ShadowySpirits opened this issue Mar 15, 2023 · 10 comments · Fixed by #5308
Closed

Prometheus exporter add duplicated suffix for counter metrics #5304

ShadowySpirits opened this issue Mar 15, 2023 · 10 comments · Fixed by #5308
Labels
Bug Something isn't working

Comments

@ShadowySpirits
Copy link
Contributor

Describe the bug
Prometheus exporter will add duplicated suffix _total.

Additional context
Prometheus exporter will add a _total suffix for all counter metrics. I think it should have a check that the metrics name already ends at _total.

String headerName(String name, PrometheusType type) {
if (type == PrometheusType.COUNTER) {
return name + "_total";
}
return name;
}

If the community agrees that this is a bug, I'm willing to submit a patch to fix it.

@ShadowySpirits ShadowySpirits added the Bug Something isn't working label Mar 15, 2023
@jack-berg
Copy link
Member

@dashpole this seems reasonable but I don't see any mention of this scenario in the prometheus openmetrics compatibility document. What do you think? Should the compatibility doc be updated to say that "_total" should be appended if not already present?

@ShadowySpirits
Copy link
Contributor Author

@dashpole this seems reasonable but I don't see any mention of this scenario in the prometheus openmetrics compatibility document. What do you think? Should the compatibility doc be updated to say that "_total" should be appended if not already present?

Hi, @jack-berg. I notice that there is such a requirement in the specification:

Monotonic Sum metric points MUST have _total added as a suffix to the metric name.

@jack-berg
Copy link
Member

Sorry @ShadowySpirits for the delayed reply. I see that the spec says "Monotonic Sum metric points MUST have _total added as a suffix to the metric name". I think for the behavior to be what you suggest the text would need to read something like: "Monotonic Sum metric points MUST have _total suffix. If the metric name does not end with _total, it MUST be added."

@ShadowySpirits
Copy link
Contributor Author

Sorry @ShadowySpirits for the delayed reply. I see that the spec says "Monotonic Sum metric points MUST have _total added as a suffix to the metric name". I think for the behavior to be what you suggest the text would need to read something like: "Monotonic Sum metric points MUST have _total suffix. If the metric name does not end with _total, it MUST be added."

Your explanation makes sense, but repeating _total after metrics suffixed with _total is weird and causes different behavior from otlp-exporter. Is it possible to clarify the interpretation of the spec?

@jack-berg
Copy link
Member

jack-berg commented Mar 31, 2023

repeating _total after metrics suffixed with _total is weird and causes different behavior from otlp-exporter.

💯

@dashpole can you offer some insight here?

@ShadowySpirits
Copy link
Contributor Author

@jack-berg @dashpole Have there been any updates? I noticed that my pull request was closed due to being stale.

@jack-berg
Copy link
Member

Sorry about that - I thought we might get some feedback from the spec. Even though the spec doesn't explicitly describe the behavior of only adding "_total" if the name doesn't already end with it, I think its reasonable. I also think this is the behavior of the prometheus java http client. I believe this code strips the _total suffix if it exists, and this code appends it when serializing to the text format.

I re-opened and approved your PR. Thanks for you patience.

@ShadowySpirits
Copy link
Contributor Author

@jack-berg Thanks a lot!

@dashpole
Copy link
Contributor

Sorry, I was out on leave. Yes, please do not add a second _total if it is already the suffix of the metric. I can clarify that in the spec if you haven't already.

@jack-berg
Copy link
Member

Thanks @dashpole. I did not update the spec.

carlosalberto pushed a commit to open-telemetry/opentelemetry-specification that referenced this issue Jul 6, 2023
Clarifies that _total does not need to be added as a suffix to counters
if it already exists. There was confusion in
open-telemetry/opentelemetry-java#5304 (comment)
if that was correct or not.
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this issue Oct 31, 2024
…elemetry#3581)

Clarifies that _total does not need to be added as a suffix to counters
if it already exists. There was confusion in
open-telemetry/opentelemetry-java#5304 (comment)
if that was correct or not.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants