query-frontend: inject query cache keys for LabelValues/Cardinality requests

[dimitarvdimitrov]

This PR makes it possible for GEM to inject its own cache key generation. This required splitting up the existing genericQueryDelegate into two interfaces

type tenantCacheTTL interface {
	// ttl returns the cache TTL for the input userID.
	ttl(userID string) time.Duration
}

and extending CacheSplitter to include LabelValues and Cardinality cache items

// CacheKeyGenerator generates cache keys. This is a useful interface for downstream
// consumers who wish to implement their own strategies.
type CacheKeyGenerator interface {
	// QueryRequest should generate a cache key based on the tenant ID and Request.
	QueryRequest(ctx context.Context, tenantID string, r Request) string

	// LabelValues should return a cache key for a label values request. The cache key does not need to contain the tenant ID.
	LabelValues(ctx context.Context, path string, values url.Values) (*GenericQueryCacheKey, error)

	// LabelValuesCardinality should return a cache key for a label values cardinality request. The cache key does not need to contain the tenant ID.
	LabelValuesCardinality(ctx context.Context, path string, values url.Values) (*GenericQueryCacheKey, error)
}

[dimitarvdimitrov]

I find the Generate prefix unnecessary and repetitive. I went with it for consistency, but I would rather drop it. If reviewers agree, I can introduce new methods as LabelValuesCacheKey and LabelValuesCardinalityCacheKey and open a follow-up PR to rename GenerateCacheKey to CacheKey

Also: should we still call it CacheSplitter if it generates cache keys, and doesn't actually split anything?

[56quarters]

I find the Generate prefix unnecessary and repetitive

I don't feel strongly about it if you want to change it.

Also: should we still call it CacheSplitter if it generates cache keys, and doesn't actually split anything?

I definitely think this should change.

[56quarters]

LGTM, thanks!

[56quarters]

I find the Generate prefix unnecessary and repetitive

I don't feel strongly about it if you want to change it.

Also: should we still call it CacheSplitter if it generates cache keys, and doesn't actually split anything?

I definitely think this should change.

[dimitarvdimitrov]

thank you for the review @56quarters. I pushed a few more changes

1. Remove Generate prefix from CacheSplitter interface
2. Rename CacheSplitter to CacheKeyGenerator
3. Remove userID from signature of LabelValues and Cardinality methods. The tenant ID was already being taken into account in the genericQueryCache, so it was confusing to ask the key generator to account for it too