-
Notifications
You must be signed in to change notification settings - Fork 9.1k
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
bug: Fix quality value in accept header #13313
bug: Fix quality value in accept header #13313
Conversation
30b55ec
to
d2cc0d2
Compare
scrape/scrape.go
Outdated
@@ -675,7 +675,7 @@ func acceptHeader(sps []config.ScrapeProtocol) string { | |||
weight-- | |||
} | |||
// Default match anything. | |||
vals = append(vals, fmt.Sprintf("*/*;q=%d", weight)) | |||
vals = append(vals, fmt.Sprintf("*/*;q=%d", weight-1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current quality value of */*
is 2.0 -- subtracting 1 will result in a quality value of 1.0 which is a higher precedence than application/openmetrics-text
(at 0.5
or 0.4
depending on version) and text/plain
(0.3
). Wouldn't it be better to match the old behavior of Prometheus which assigned 0.1
to */*
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @kalpadiptyaroy @rocketraman,
I am a former ticket reporter.
When I first discovered the problem we thought it would be appropriate to fix it as follows:
vals = append(vals, fmt.Sprintf("*/*;q=0.%d", weight))
By using this code, qvalue becomes 0.2
and the problem is avoided.
However, we also question whether this is the correct way to calculate weight.
I think you should ask the person involved in #12738 to check the specifications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yu-shiba. I have gone through the issue #12738. This issue is more about bringing in flexibility to the Accept header definitions, such that it is not hardcoded. So let's not consider it as of now.
@rocketraman your suggestion too is good, but my perspective to this is as follows:
If we hardcode the q
value for text/plain
as 0.1
then later on their is a probability of confusion for the maintainers, that why text/plain
is getting a special preference, when as per https://www.rfc-editor.org/rfc/rfc9110.html#name-accept specification, q
value must be relative and calculated via normalisation
func. , hence I would like to go with @yu-shiba 's solution, since that will preserve the relative factor and avoid confusion.
vals = append(vals, fmt.Sprintf("*/*;q=%d", weight-1)) | |
vals = append(vals, fmt.Sprintf("*/*;q=0.%d", weight)) |
Still I would like everyone to agree on a common point before merging this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
per https://www.rfc-editor.org/rfc/rfc9110.html#name-accept specification, q value must be relative and calculated via normalisation
RFC says:
- It is relative to other q values
- It is normalized to a value between 0 and 1
It does not say it needs to be calculated. For example, if all other values are tenths (as they are), */*
could be hard-coded to a hundredths value, like 0.01
and this would guarantee that both conditions from the RFC are met.
All that being said, the code of 0.%d
should in fact result in 0.1
for */*
anyway if I'm reading this code right (not 0.2
as mentioned above by @yu-shiba ), so that approach is fine and makes sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note also this code fails to produce a sensible result if there are 9 or more scrape protocols passed in! Probably not something we need to worry about now. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rocketraman Noted and generalised for any number of scrape protocols.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kalpadiptyaroy Perhaps not the intended result?
application/openmetrics-text;version=1.0.0;q=0.500000,application/openmetrics-text;version=0.0.1;q=0.500000,text/plain;version=0.0.4;q=0.500000,*/*;q=0.500000
Also, if we are serious about the calculation, %f
is dangerous in light of the RFC.
weight = OWS ";" OWS "q=" qvalue qvalue = ( "0" [ "." 0*3DIGIT ] ) / ( "1" [ "." 0*3("0") ] )
It is better to specify the width.
"%.3g"
@rocketraman I thought it was 0.1 too, but the initial value of weight is the number of ScrapeProtocolsHeaders.
This is why the qvalue results seem to differ from intuition.
1a1ae5b
to
e779f3d
Compare
Signed-off-by: Kumar Kalpadiptya Roy <kalpadiptya.roy@outlook.com>
e779f3d
to
b012366
Compare
@yu-shiba Thanks for clarifying the |
Thanks! |
@kalpadiptyaroy, |
Fixed #13268 - Now 'q' value will be strictly within the range of 0 to 1.