Implement query timeouts #498
Conversation
| var stalenessDelta = flag.Duration("query.staleness-delta", 300*time.Second, "Staleness delta allowance during expression evaluations.") | ||
| var ( | ||
| stalenessDelta = flag.Duration("query.staleness-delta", 300*time.Second, "Staleness delta allowance during expression evaluations.") | ||
| queryTimeout = flag.Duration("query.timeout", 30*time.Second, "Maximum time a query may take before being aborted.") |
There was a problem hiding this comment.
In the end this should be user-configurable via a query parameter. But for now, a flag with a default seems ok. Let's set the default to 2 minutes though (based on experience with heavy datasets and some rare, but useful, expensive queries).
|
@fabxc did you close this PR intentionally? |
|
Missed some stuff and wanted to make some changes before bothering anyone with reviewing - but you already commented ;) Relying on the TotalEvalTimer breaks a few tests and requires to manually start it before calling ast.EvalTo*. It's not exactly pretty, but works if I change the relevant parts. |
| @@ -40,6 +40,11 @@ func (t *Timer) Stop() { | |||
| t.duration += time.Since(t.start) | |||
| } | |||
|
|
|||
| // Return the elapsed time up to now | |||
There was a problem hiding this comment.
s/now/now./
(sentence-style comments with periods: https://code.google.com/p/go-wiki/wiki/CodeReviewComments#Comment_Sentences)
|
Things are fixed now, more transparency would be nice though. |
| var stalenessDelta = flag.Duration("query.staleness-delta", 300*time.Second, "Staleness delta allowance during expression evaluations.") | ||
| var ( | ||
| stalenessDelta = flag.Duration("query.staleness-delta", 300*time.Second, "Staleness delta allowance during expression evaluations.") | ||
| queryTimeout = flag.Duration("query.timeout", 2*time.Minute, "Maximum time a query may take before being aborted.") |
There was a problem hiding this comment.
Referring to @juliusv 's comment: We definitely want a timeout configurable per query later, but then we still need this flag as a default value if no timeout is requested in the query. We only have to change the docstring then..
|
Hi @fabxc , thanks for your contribution. I believe you have forgotten to add the changes of timer.go to this PR. |
|
Sorry, my bad, now I can see the changes. Was in the wrong repo... |
| @@ -40,6 +40,11 @@ func (t *Timer) Stop() { | |||
| t.duration += time.Since(t.start) | |||
| } | |||
|
|
|||
| // Return the elapsed time up to now. | |||
There was a problem hiding this comment.
Start doc comment with "ElapsedTIme ...". golint catches things like that.
|
@juliusv That comment is attached to an outdated diff... GH reviewing... meh... |
|
@fabxc So I looked over this again, and besides our musings about future possibilities, this looks great. I'll merge it unless you had anything you still wanted to add or change? Thanks! |
|
(oh, but please squash this into one or two meaningful commits before merging) |
This is related to prometheus#454. Queries now timeout after a duration set by the -query.timeout flag. The TotalEvalTimer is now started/stopped inside any of the ast.Eval* functions.
fabxc
force-pushed
the
feature/query_timeout
branch
from
bbfaca1 to
fa1e900
Compare
|
Done. |
|
Woot, thanks! |
Document stddev/stdvar_over_time
No description provided.