limit get series API call time range when start param is not specified

[yeya24]

Signed-off-by: Ben Ye benye@amazon.com

What this PR does:

1. We ensure select start time is always a non-negative value (we set it to 0 if it is < 0) to avoid int64 overflow issue.
2. For /api/v1/series API and start time of 0, we just fallback to query ingesters only to avoid OOM kills. This should work well for those clients who don't support specifying start on series API.

Which issue(s) this PR fixes:

/api/v1/series API supports optional start and end parameters. Since Cortex uses the Prometheus API code, right now if start is not specified, it will use the min value here https://github.com/prometheus/prometheus/blob/main/web/api/v1/api.go#L725. This happens for some clients that don't configure these params.

The issue here is that if start time is too small, we will end up querying all the data for the tenant. For series API this usually means we need to fetch a lot of data and do a lot of series duplications. It is really easy to get OOM killed.

https://github.com/cortexproject/cortex/blob/master/pkg/querier/querier.go#L344 Here we checks the max query length. However, it is broken because the endTime.Sub(startTime) overflows int64.

Other notes:

1. Why maxQueryLookback is not enough?
   maxQueryLookback could limit the start time. But since maxQueryLookback is a limit which applies to both query, query range and series queries, We might not want to use the same range for all those 3 queries. For example, you may want to query 3 months data for query range but querying 3 months data using Series might not make sense in terms of performance. And if maxQueryLookback is not set then we don't have a fallback mechanism to protect ourselves.

2. What if the user specifies start but start is a really small value?
   We ignore this case for now because this usually is not a valid use case. If the user intentionally does this, it is better to use maxQueryLookback.

Checklist

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

[alanprot]

I'm ok with this change.. but we are changing the behavior a bit (before no arguments = everything and now we only return the data stored on ingesters).

@alvinlin123 WDYT?

[alvinlin123]

So according to https://cortexmetrics.io/docs/api/#get-series-by-label-matchers Cortex already ignore start/end parameter by default. If -querier.query-store-for-labels-enabled then it will use it, but it's experimental feature. So I think we can move forward with this.

Let's update https://cortexmetrics.io/docs/api/#get-series-by-label-matchers with the default value.

[alvinlin123]

I think it's better if we merge this to line 307 above. Because what they are doing are pretty much the same.

[yeya24]

I see. I wanted to merge the two but to do this we need to do L318 ~ L336 first before L307.
This is probably a behavior change because we will do the time range validation first and then send to the metadata querier.

[alvinlin123]

I was thinking we can do something along the line

userID, err := tenant.TenantID(ctx)
if err != nil {
  return storage.ErrSeriesSet(err)
}
startMs, endMs, err := validateQueryTimeRange(ctx, userID, sp.Start, sp.End, q.limits, q.maxQueryIntoFuture)

if sp == nil {
  sp = &storage.SelectHints{Start: q.mint, End: q.maxt}
} else if sp.Func == "series" && (!q.queryStoreForLabels ||  (err != nil && startMs == 0)) {	
   return q.metadataQuerier.Select(true, sp, matchers...)
} 

if err == errEmptyTimeRange {
    return storage.NoopSeriesSet()
} else if err != nil {
   return storage.ErrSeriesSet(err)
}

Essentially if start is 0 we treat it as if queryStoreForLabels is false. I also think that validateQueryTimeRange should return -1 for startMs and endMs when err != nil to make our life easier.

There is an edge case where user actually specifies "0" for "start", and Cortex would still return last 24 hours data, but I think this behaviour is ok for the edge case.

[yeya24]

OK, I think this could work...
But is it worth? For readability I prefer to separate them. It is just 1 duplicated line.
This is not very clear tbh.
if sp.Func == "series" && (!q.queryStoreForLabels || (err != nil && startMs == 0))

[alvinlin123]

Yea that's why I was thinking maybe validateQueryTimeRange should return -1s when err is not nil. But this is fine, I don't think we need to do it change anything for now; it's up to you.

I think this PR is ok, but I will let @alanprot have a chance to voice any other concerns based on what I said :)