Handle the case of large step causing single pt extents

[gouthamve]

We noticed that we were ignoring samples if the step is 24h and this
fixes it.

[bboreham]

I see that this improves the symptom, however it now does something undesirable: it makes two queries for parts of the range before and after the cached extent, with no gap between them since the extent is of zero width.
Can you special-case this so the cached value is added but only one query is made?

[gouthamve]

@bboreham Handled the special case differently, WDYT?

[bboreham]

Still looks like it will do the same 2 queries. Try changing the test so the cached value is at 101.

[bboreham]

Wait how does this even work? Your test is for a zero-length range (Start: 100, End: 100); does that happen in practice?

[gouthamve]

It happens when the step is more than 50% of the interval. Or basically large steps. In the case in our production it was 24hr step.

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

https://github.com/cortexproject/cortex/blob/master/pkg/querier/queryrange/split_by_interval.go#L63-L86

[bboreham]

OK can you have different handling for req.GetStart() == req.GetEnd() ?

AFAICS for a longer request your change will put back the conditions I was fixing in #3653

[gouthamve]

AFAICS for a longer request your change will put back the conditions I was fixing in #3653

Basically when you have req.GetStart() == req.GetEnd(), it is impossible to have the conditions of #3653. When start and end are the same, you basically have a single step in that range. If there is a hit, we pick that extent (no other extent can exist). We basically can never have many small requests.

[bboreham]

OK but this PR changes it for all requests.

[bboreham]

Ah, no it doesn't!

Sorry, I did not understand the comment at all, and didn't spot how the test had changed.

Still think it would be clearer to special-case req.GetStart() == req.GetEnd() outside of the loop.

[pracucci]

The fix LGTM. I left a couple of non-blocking comments.

[bboreham]

[sorry, posted this in the wrong place yesterday]

Wouldn't it look better if you add something like this at the top and don't change the loop?

  // special-case situation created by step bigger than range
  if req.GetStart() == req.GetEnd() {
    // do we have a cached extent that matches this zero-length request?
    if len(extents) == 1 && extents[0].Start == req.GetStart() && extents[0].End == req.GetEnd() {
      res, err := extent.toResponse()
      cachedResponses = append(cachedResponses, res)
    } else {
      // cache didn't match this request; pass it through to querier
      requests = append(requests, req)
    }
    return requests, cachedResponses, nil
  }

[gouthamve]

But that would mean queries with the same start and end time created directly by users will never be cached. Might not be an issue though.

[bboreham]

I'm not seeing that. All I meant was to pull the special case out of the loop so we don't have to stare really hard at the loop to see what it means.

[bboreham]

Oh, you mean if we have a zero-length request and a non-zero-length cache entry (but still over 5 minutes) that covers it, then we won't look at that. Fair point.

I think this helps to prove that this routine is too complicated to get right.