Fix issue where cached entry size keeps increasing when making tiny query repeatedly

[alvinlin123]

What this PR does:
See my comment at #3920 (comment)

Which issue(s) this PR fixes:
This issue was found as part of #3920, not sure if this PR fixes the issue though.

Checklist

• Tests updated
• Documentation added
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[pracucci]

Thanks for working on it! I reviewed the code: good job spotting the issue, but I'm not sure this is correct.

The problem is that the extends returned by handleHit() overwrite the cached response. Now think about running a query over "last 24h", then running the same query over "last 1h". With the changes proposed in this PR, the second query on "last 1h" would overwrite the cached extends for the "previous 23h" while we don't want to loose them.

[alvinlin123]

Thanks for working on it! I reviewed the code: good job spotting the issue, but I'm not sure this is correct.

The problem is that the extends returned by handleHit() overwrite the cached response. Now think about running a query over "last 24h", then running the same query over "last 1h". With the changes proposed in this PR, the second query on "last 1h" would overwrite the cached extends for the "previous 23h" while we don't want to loose them.

Spot on, I have another idea, will submit a an update soon.

[alvinlin123]

I decided to go with different approach to fix the issue; instead of modifying the []Extent in partition, I made queryrange.handleHit to be able to handle overlapping Extent and queryrange.matrixMerge() be able to deal with overlapping sample points. This approach is probably more resilient to future change in partition and reduces amount of slice allocation.

One thing I am not quite certain is the following code in queryrange.MergeResponses

for _, res := range responses {
		promResponses = append(promResponses, res.(*PrometheusResponse))
		resultsCacheGenNumberHeaderValues = append(resultsCacheGenNumberHeaderValues, getHeaderValuesWithName(res, ResultsCacheGenNumberHeaderName)...)
	}

Is there any special handling needed if the given responses contains overlapping sample points?

[pracucci]

Thanks for iterating on my feedback! Your change looks good to me. I left few nits I would be glad if you could take a look. In the meanwhile I will ping another person expert on the query-frontend for a 2nd look, cause this stuff is tricky :)

[pracucci]

Should never happen with the current logic but, to be on the safe side, I would handle the case sort.Search() returns -1 (in this cause I believe we should the input samples).

[alvinlin123]

Agreed, better to be more defensive. But I took a look at the doc https://golang.org/pkg/sort/#Search sort.Search() never returns -1 it returns [0, n], where if return value is n it means not found.

func main() {

	x := []int{3, 5, 6, 7}
	
        // both prints "4"
	fmt.Println(sort.Search(len(x), func(i int) bool {return x[i] > 100}))  
	fmt.Println(sort.Search(len(x), func(i int) bool {return x[i] < 0})) 
}

https://play.golang.org/p/HBtNWh3_MsR

But I think I get your point, I will add special handling if minTs is smaller than samples[0].

[owen-d]

Nice investigation and good job on the PR. This code is very tricky (took me a while to reacquaint myself with the code). The fix looks good to me once you address @pracucci's comments. I'm looking forward to revendoring this into Loki :)

[alvinlin123]

Thanks for review and the comments @pracucci and @owen-d. Yea, this is definitely tricky stuff; I took a trip down memory lane back in my college algorithm classes (10+ years ago) trying to understand these stuff, cool stuff :-)

I address the PR comments, let me know if they look okay.

[pracucci]

LGTM, thanks!

[pstibrany]

🙈