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

Negotiate OM v1.0.0 #475

Merged
merged 2 commits into from May 2, 2023
Merged

Conversation

gouthamve
Copy link
Member

Also return OM v1.0.0, and not v0.0.1

Fixes #456

Also return OM v1.0.0, and not v0.0.1

Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
Copy link
Member

@jesusvazquez jesusvazquez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM. Lets merge to solve the fix and see if this can get in the next release.

@roidelapluie
Copy link
Member

It feels like we should return 0.0.1 if no version is specified to be backwards compatible

@gouthamve
Copy link
Member Author

gouthamve commented Apr 27, 2023

It kinda goes against the spec as mentioned here: https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#overall-structure

The content type MUST be: application/openmetrics-text; version=1.0.0; charset=utf-8

@beorn7
Copy link
Member

beorn7 commented Apr 27, 2023

If the content type really MUST be application/openmetrics-text; version=1.0.0; charset=utf-8, what does that mean for the content negotiation? Wouldn't it already be against the spec if the Accept header contains any content type different from the above? And if so, then we surely can respond to a non-spec-conforming Accept header with non-spec-conforming content type header, can't we?

Or less formally argued: If I announce that I accept OM of any version, then I cannot expect to get 1.0.0 back. If I want 1.0.0, then I have to put it into the accept header in exactly that way.

@jonatan-ivanov
Copy link

jonatan-ivanov commented Apr 27, 2023

If the content type really MUST be [...]

The Content-Type really must be that, please see the OpenMetrics specs:
"The content type MUST be: application/openmetrics-text; version=1.0.0; charset=utf-8"

Wouldn't it already be against the spec if the Accept header contains any content type different from the above?

I guess the specification does not specify the Accept header. One can send application/json but they might not get it. Also, one can send */* but in that case they should not be surprised if they get back a result in BSON. :) As far as I know the current fallback is plain/text which this PR is not changing. I think returning 0.0.1 is already against the spec since it must be 1.0.0.

If I announce that I accept OM of any version, then I cannot expect to get 1.0.0 back. If I want 1.0.0, then I have to put it into the accept header in exactly that way.

I think this is a very valid argument and I think the spec-conforming solution would be always returning 1.0.0 if any or no version of application/openmetrics-text is requested.

To think about existing users maybe this could be an ok solution: return 0.0.1 if it is explicitly requested (this violates the spec) but return 1.0.0 if any other or no version of application/openmetrics-text is requested (fall back to plain/text in other, non-openmetrics-text cases).

Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
@gouthamve
Copy link
Member Author

@roidelapluie @beorn7 Now returning 0.0.1 with no version for backwards compat.

@beorn7
Copy link
Member

beorn7 commented Apr 30, 2023

@jonatan-ivanov I actually tried to argue that returning v0.0.1 when no version is requested is OK. :)

Note that whatever content-type we return, the actual content is the same. Plus, the Go implementation of OpenMetrics isn't even complete yet. And second-order plus, there is a discussion if we should go for v1.1 or even v2.0 instead of trying to fully implement v1.0. So the discussion is a bit academic at this point.

I think with this PR, we should fix the current behavior of not even returning v1.0.0 even when requested, but we should try to change as little as possible to minimize breakages at this point.

@jonatan-ivanov and @bboreham I see your points, and this should be discussed. But by not changing the behavior now, we still keep the door open for changes in the future. While changing more than needed right now might create breakage noise that we might not need to create, e.g. if "the real thing" will be OM v1.1 or v2.0 anyway.

@gouthamve gouthamve merged commit b933a95 into prometheus:main May 2, 2023
7 checks passed
@gouthamve gouthamve deleted the negotiate-om-1.0 branch May 2, 2023 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Content negotiation is broken for expfmt and for promhttp
6 participants