[query] Allow lookback duration to be set in query parameters #1793
Conversation
| @@ -52,6 +52,7 @@ const ( | |||
| debugParam = "debug" | |||
| endExclusiveParam = "end-exclusive" | |||
| blockTypeParam = "block-type" | |||
| lookbackParam = "lookback" | |||
There was a problem hiding this comment.
Maybe 'lookbackDur' or something similar so it's a little easier to grok what it is?
| return 0, false, nil | ||
| } | ||
|
|
||
| if lookback == stepParam { |
There was a problem hiding this comment.
Curious if lookbackDuration is > stepSize should it default to stepSize?
There was a problem hiding this comment.
I think this is for the special case of &lookback=step; although it reads a little weird since it's being compared to a variable named xxxParam
| return 0, false, err | ||
| } | ||
|
|
||
| return value, true, nil |
There was a problem hiding this comment.
Might have to check that step > 0 otherwise you may have a bad time (although that case is handled elsewhere so it shouldn't panic, but might be better to catch it early)
There was a problem hiding this comment.
Sure thing will do.
| return value, nil | ||
| } | ||
|
|
||
| // Try parsing as a float value specifying seconds, the Prometheus default |
There was a problem hiding this comment.
meganit: . at end of comment
| seconds, floatErr := strconv.ParseFloat(str, 64) | ||
| if floatErr == nil { | ||
| ts := seconds * float64(time.Second) | ||
| if ts > maxInt64 || ts < minInt64 { |
There was a problem hiding this comment.
Should the lower bound just check ts <= 0? Negative seems invalid for duration parsing
There was a problem hiding this comment.
This was existing code, I could update though.
|
|
||
| if lookback == StepParam { | ||
| // Use the step size as lookback. | ||
| step, err := parseStep(r, StepParam) |
There was a problem hiding this comment.
Maybe should explicitly add a sanity check for step > 0? It should fail already on ParseStep but sanity check here may be useful
There was a problem hiding this comment.
Yup so I added that inside parseStep
| }, | ||
| { | ||
| name: "can set lookback duration based on step", | ||
| query: "lookback=step&step=10s", |
There was a problem hiding this comment.
May be worth adding in lookback=step, lookback=step&step=invalid , lookback=step&step=-1 cases here?
| @@ -206,6 +209,9 @@ func TestEncodeDecodeFetchQuery(t *testing.T) { | |||
| MetricsType: storage.AggregatedMetricsType, | |||
| StoragePolicy: policy.MustParseStoragePolicy("1m:14d"), | |||
| } | |||
| lookback := time.Minute | |||
| fetchOpts.LookbackDuration = &lookback | |||
There was a problem hiding this comment.
Any worth in adding one without a specified lookback to ensure it uses the config one correctly?
| return e.opts.Store().Fetch(ctx, query, fetchOpts) | ||
| } | ||
|
|
||
| func (e *engine) ExecuteExpr( | ||
| ctx context.Context, | ||
| parser parser.Parser, | ||
| opts *QueryOptions, | ||
| fetchOpts *storage.FetchOptions, |
There was a problem hiding this comment.
nit; set opts as last param here
What this PR does / why we need it:
This allows callers to set lookback as a request param for per-request specification of the lookback for a query.
Special notes for your reviewer:
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: