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

Update query API to support timeouts #1014

Merged
merged 6 commits into from Apr 21, 2022

Conversation

josephwoodward
Copy link
Contributor

@josephwoodward josephwoodward commented Mar 28, 2022

Fixes #999

@kakkoyun
Copy link
Member

Hey @josephwoodward, we plan to release soonish? Would you be able to have a look at this so that we can include it in the release?

@josephwoodward
Copy link
Contributor Author

@kakkoyun Yes, no problem. IIRC the change is almost there, I'll have a look tonight.

@josephwoodward josephwoodward force-pushed the enable_query_timeout branch 2 times, most recently from 60ceaab to 80099df Compare April 13, 2022 23:08
@josephwoodward josephwoodward marked this pull request as ready for review April 13, 2022 23:09
Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

Looking good. Thanks a lot.

Would you consider adding the timeout to QueryRange as well? It's the same API.
https://prometheus.io/docs/prometheus/latest/querying/api/#range-queries

api/prometheus/v1/api.go Outdated Show resolved Hide resolved
api/prometheus/v1/api.go Outdated Show resolved Hide resolved
@josephwoodward
Copy link
Contributor Author

Looking good. Thanks a lot.

Would you consider adding the timeout to QueryRange as well? It's the same API. https://prometheus.io/docs/prometheus/latest/querying/api/#range-queries

Ah yes, thanks. I missed that one.

josephwoodward and others added 6 commits April 16, 2022 03:11
Signed-off-by: Joseph Woodward <joseph.woodward@xeuse.com>
Co-authored-by: Kemal Akkoyun <kakkoyun@users.noreply.github.com>
Signed-off-by: Joseph Woodward <joseph.woodward@xeuse.com>
Co-authored-by: Kemal Akkoyun <kakkoyun@users.noreply.github.com>
Signed-off-by: Joseph Woodward <joseph.woodward@xeuse.com>
Signed-off-by: Joseph Woodward <joseph.woodward@xeuse.com>
Signed-off-by: Joseph Woodward <joseph.woodward@xeuse.com>
Signed-off-by: Joseph Woodward <joseph.woodward@xeuse.com>
@josephwoodward
Copy link
Contributor Author

josephwoodward commented Apr 16, 2022

@kakkoyun This is ready for another review when you have the time, I've added support for timeouts to the QueryRange API as per your comment.

Also, I noticed that the tests currently don't validate the request parameters the HTTP client sends, is there an issue to fix this or is there more context I'm unaware of here?

@kakkoyun
Copy link
Member

Also, I noticed that the tests currently don't validate the request parameters the HTTP client sends, is there an issue to fix this or is there more context I'm unaware of here?

I believe I don't have the context either. Feel free to open an issue so that we can discuss.

Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

LGTM.
Thanks for the contribution.

@kakkoyun kakkoyun merged commit 48a686a into prometheus:main Apr 21, 2022
@josephwoodward josephwoodward deleted the enable_query_timeout branch April 21, 2022 07:24
@bboreham
Copy link
Member

At first glance I thought this would set the context timeout. I think there should at least be a documentation comment saying what the intended behaviour is.

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.

Allow passing timeout parameter in Prometheus queries
3 participants