align metric queries by step and other queries by split interval #5181
Conversation
| if !newEnd.Before(end) { | ||
| newEnd = end | ||
| } else if endTimeInclusive { | ||
| newEnd = newEnd.Add(-time.Millisecond) | ||
| } |
There was a problem hiding this comment.
I don't quite understand this.
There was a problem hiding this comment.
When the end time is inclusive, we do not want the current query's end time to be the same as the start time of the next query. The only queries that have both start and end time inclusive are metadata queries, and without this change, we would still end up querying the same data as the next query.
I will add a comment to make it clear.
| @@ -298,6 +318,9 @@ func splitMetricByTime(r queryrangebase.Request, interval time.Duration) []query | |||
| EndTs: end, | |||
| }) | |||
| } | |||
|
|
|||
| // change the start time to original time | |||
| reqs[0] = reqs[0].WithStartEnd(lokiReq.GetStart(), reqs[0].GetEnd()) | |||
There was a problem hiding this comment.
can you add a check if len(reqs) != 0
|
Should we deprecate the |
While there is not much value in keeping it, I was thinking about changing it to round down the start time and round up the end time to make all the queries cacheable. |
What this PR does / why we need it:
It changes the way query splits are built based on the query type:
stepin query.Note: Start of the first query and end time of the last query is kept the same as the original query.
This change is being done for two reasons:
24h(split interval). Testing this internally reduced 3x the number of index queries and improved query performance by 2x for long-range/labelcalls.While there is no visible impact on the results cache hit rate, I think we should also change the step alignment code to round down the start time and round up the end time, which would anyways only align the start time of the first query and end time of the last query.
Checklist