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

Make Query requests idempotent #1022

Merged
merged 3 commits into from Apr 13, 2022
Merged

Conversation

dohnto
Copy link
Contributor

@dohnto dohnto commented Apr 7, 2022

Address #1020.

Signed-off-by: Tomáš Dohnálek dohnto@gmail.com

This was challenging to test. I used toxiproxy to inject connectivity issues. I build a custom Toxic which counts HTTP requests a closes every even request.

Without the header set, the conversation was following and my range query ended with

Error querying Prometheus: Post "http://localhost:8080/api/v1/query_range": EOF

Screenshot from 2022-04-07 13-51-44

When I set the idempotency header, the conversation was equal to the case when GET is used - the connection is reopen and the request is retried.

Screenshot from 2022-04-07 13-53-40

Address prometheus#1020.

Signed-off-by: Tomáš Dohnálek <dohnto@gmail.com>
@dohnto dohnto marked this pull request as draft Apr 7, 2022
api/prometheus/v1/api.go Outdated Show resolved Hide resolved
Signed-off-by: Tomáš Dohnálek <dohnto@gmail.com>
@dohnto dohnto force-pushed the dohnto/post-idempotent branch from a1b532e to 7016fd3 Compare Apr 8, 2022
@dohnto dohnto marked this pull request as ready for review Apr 8, 2022
Copy link
Member

@kakkoyun kakkoyun left a comment

This looks reasonable. Let's document more, and I'm happy to accept it.

@@ -1138,6 +1138,9 @@ func (h *apiClientImpl) DoGetFallback(ctx context.Context, u *url.URL, args url.
return nil, nil, nil, err
}
req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
// Underlying `net.http` library automatically retries` idempotent requests when connectivity issues are hit.
// POST requests are not considered idempotent by default, so we need to explicitly mark them as such.
req.Header["Idempotency-Key"] = nil
Copy link
Member

@kakkoyun kakkoyun Apr 12, 2022

Choose a reason for hiding this comment

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

Could you please at the docs directly here?

Transport only retries a request upon encountering a network error if the request is idempotent and either has no body or has its Request.GetBody defined. HTTP requests are considered idempotent if they have HTTP methods GET, HEAD, OPTIONS, or TRACE; or if their Header map contains an "Idempotency-Key" or "X-Idempotency-Key" entry. If the idempotency key value is a zero-length slice, the request is treated as idempotent but the header is not sent on the wire.

And a link to the docs?

Copy link
Contributor Author

@dohnto dohnto Apr 12, 2022

Choose a reason for hiding this comment

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

PTAL.

I think we should squash before merging. Not sure if I am suppose to do that, or the Github's UI is used for it?

@dohnto dohnto force-pushed the dohnto/post-idempotent branch from 66ac0ad to ebe4b5b Compare Apr 12, 2022
Signed-off-by: Tomáš Dohnálek <dohnto@gmail.com>
@dohnto dohnto force-pushed the dohnto/post-idempotent branch from ebe4b5b to 5588f73 Compare Apr 12, 2022
@kakkoyun kakkoyun merged commit cc7991d into prometheus:main Apr 13, 2022
8 checks passed
@kakkoyun
Copy link
Member

kakkoyun commented Apr 13, 2022

@dohnto Amazing work. Thanks for taking care of it.

@bboreham
Copy link
Contributor

bboreham commented May 23, 2022

Looking forward to having this change in a release.

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.

None yet

3 participants