-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Update query API to support timeouts #1014
Conversation
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? |
@kakkoyun Yes, no problem. IIRC the change is almost there, I'll have a look tonight. |
60ceaab
to
80099df
Compare
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.
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. |
Signed-off-by: Joseph Woodward <[email protected]>
Co-authored-by: Kemal Akkoyun <[email protected]> Signed-off-by: Joseph Woodward <[email protected]>
Co-authored-by: Kemal Akkoyun <[email protected]> Signed-off-by: Joseph Woodward <[email protected]>
Signed-off-by: Joseph Woodward <[email protected]>
Signed-off-by: Joseph Woodward <[email protected]>
Signed-off-by: Joseph Woodward <[email protected]>
66b9777
to
f2f5b48
Compare
@kakkoyun This is ready for another review when you have the time, I've added support for timeouts to the 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. |
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.
LGTM.
Thanks for the contribution.
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. |
Fixes #999