Alerting: Make time range query parameters not required when querying Loki #62985
Conversation
alexweav
added
area/alerting
| if start != 0 { | ||
| values.Set("start", fmt.Sprintf("%d", start)) | ||
| } | ||
| if end != 0 { | ||
| values.Set("end", fmt.Sprintf("%d", end)) | ||
| } |
There was a problem hiding this comment.
I'd document the behaviour of range query defaults here - When no start or end timestamps are sent, Loki will default to 1h for the start timestamp and now for the end time.
One more thing that I don't like is that start and end are kind-of like options parameters but you must specify something at the function call - can we at least document the function, it so that users of the method understand that behaviour?
There was a problem hiding this comment.
Perhaps it might make sense to use a QueryOptions struct instead?
type QueryOptions struct {
Selectors []Selector
Start int64 // an optional start time
End int64 // an optional end time
}There was a problem hiding this comment.
Looking at this with fresh eyes, I took a new approach that I think addresses all these.
Now, we move this up a layer - the client still requires timestamps, but we calculate our own defaults instead.
- The layer above already uses a query options struct like in @grobinson-grafana's suggestion.
- No need for docs to explain weird edge case behavior at the client level. Code is more clear.
- 1 hour is honestly not a good default for state history. It's too short. In the new approach, we can add our own default.
- Using Grafana's
time.Now()rather than Loki's is generally more correct in the event that system clocks do not align. - Better single responsibility principle - let's keep
loki_http.gofocused on the actual HTTP communication, the default range is app logic that's agnostic of transport.
There was a problem hiding this comment.
I'm with George - QueryOptions is what I would expect. Aligns with open/close and SRP.
There was a problem hiding this comment.
Ah, nvm - I see what you mean with "moving up a layer". LGTM.
… Loki (#62985) * Make from and to not required * Move default range calculation up to loki.go
What is this feature?
Now we only forward the
from/toquery parameters if they are supplied, rather than all the time. It will fall back to Loki's default time range, which is usually the last hour. Although most of our clients will need to specify a longer time range, this still improves basic usability of the API. Also, such clients only need to specifyfromand can omittomost of the time.Also renames the client method
querytorangeQueryto be clear as to which query type we're using. For some queries, it might be advantageous to use an instant query instead. We can evaluate that in the future.Why do we need this feature?
Nice quality-of-life for clients. UI can omit the
toparameter when it rolls out. Doesn't affect datasource-based clients. Not compat breaking.Which issue(s) does this PR fix?:
contrib #48359
Special notes for your reviewer: