querier: option to skip sending queries to long term storage

[thorfour]

What this PR does: Moves the MinChunkAge option from chunk store into the querier to allow that behaviour regardless of store backend.

Which issue(s) this PR fixes:
N/A

Checklist

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

[jtlisi]

One nit, otherwise LGTM

[pracucci]

Thanks @thorfour for the PR! I've the feeling this change is unsafe. There's no guarantee that the alive ingesters hold all the timeseries within the -querier.query-ingesters-within period. For example, in case of scale down, ingesters chunks are flushed to the storage: with this new option enabled, you would end up querying a subset of the series.

In the case of TSDB, we're still not handling the scale down (it's TBD) because we're currently loosing the WAL if no joining ingester is found by the leaving one, but once will be fixed we'll have the same issue there as well.

In case we'll have good arguments to have this kind of change merged, I would review the PR (I've seen some inconsistencies here and there, but haven't commented cause I first would like to understand if this kind of change is feasible).

[bboreham]

The chunk store already has this feature here:

cortex/pkg/chunk/chunk_store.go

Lines 272 to 275 in fcbc186

	if from.After(now.Add(-c.cfg.MinChunkAge)) {
	// no data relevant to this query will have arrived at the store yet
	return true, nil
	}

so you shouldn't need to add it at the querier level.

[thorfour]

The chunk store already has this feature here

That's true, but I would then need to add the feature to the block querier, it made sense to have it as a top level feature instead of backend specific.

Is it preferred to be implemented in each backend or on the top level @bboreham?

[bboreham]

I guess you can have a go at moving the feature, but don't add something that duplicates it.

[gouthamve]

Can we have a more descriptive name for the flag? The old one is not well named either and I think we can do better when changing it :P

[thorfour]

@gouthamve Do you have a better name for the flag in mind? min-ingester-lookback-time?
skip-store-before-time ?

[thorfour]

Setting has been renamed to StoreMinQueryLookback

[pracucci]

Thanks Thor for addressing my feedback. I just realized a couple of more small things, I would be glad if you could take a look at. A part from this, LGTM.

[pracucci]

Thanks Thor. Changes LGTM.

[gouthamve]

@bboreham This is a breaking change, how do we want to proceed here? Do you want make this flag a deprecated flag or break deployments that upgrade w/o changing this?

Other than that, LGTM!

[pracucci]

LGTM with few nit to fix, please.

Too old now

[pracucci]

LGTM (just left few minor comments). @bboreham What's your take about this? Given it introduce a breaking change in the config, if we want it would be great to have it in the 0.6.0.

[jtlisi]

LGTM

[pracucci]

May you also rebase master so that we can move on merging it, please?

[pracucci]

@thorfour Sorry to bother you. We've merge the release 0.6.0 and you would need to move your CHANGELOG.md entry to the top of the file (remember to keep entries grouped by type).

[thorfour]

Yea forgot we had cut 0.6.0 and didn't really look at the merge conflicts when I rebased it.