From 1dbcc05e3242beaf1b1c53040d185e66223aace9 Mon Sep 17 00:00:00 2001 From: Owen Diehl Date: Mon, 27 Jan 2020 15:50:01 -0500 Subject: [PATCH 1/9] deprecates frontend.cache-split-interval in favor of querier.split-queries-by-interval Signed-off-by: Owen Diehl --- pkg/querier/queryrange/results_cache.go | 43 ++++++++++++++++---- pkg/querier/queryrange/results_cache_test.go | 31 ++++++++------ pkg/querier/queryrange/roundtrip.go | 22 +++++++++- 3 files changed, 73 insertions(+), 23 deletions(-) diff --git a/pkg/querier/queryrange/results_cache.go b/pkg/querier/queryrange/results_cache.go index 802cb32fde..f3d327fcee 100644 --- a/pkg/querier/queryrange/results_cache.go +++ b/pkg/querier/queryrange/results_cache.go @@ -37,8 +37,14 @@ type ResultsCacheConfig struct { // RegisterFlags registers flags. func (cfg *ResultsCacheConfig) RegisterFlags(f *flag.FlagSet) { cfg.CacheConfig.RegisterFlagsWithPrefix("frontend.", "", f) + f.DurationVar(&cfg.MaxCacheFreshness, "frontend.max-cache-freshness", 1*time.Minute, "Most recent allowed cacheable result, to prevent caching very recent results that might still be in flux.") - f.DurationVar(&cfg.SplitInterval, "frontend.cache-split-interval", 24*time.Hour, "The maximum interval expected for each request, results will be cached per single interval.") + f.DurationVar(&cfg.SplitInterval, "frontend.cache-split-interval", 0, "Deprecated: The maximum interval expected for each request, results will be cached per single interval. This flag has been deprecated and this behavior is now determined by querier.split-queries-by-interval") +} + +// GenerateCacheKey impls CacheSplitter +func (cfg ResultsCacheConfig) GenerateCacheKey(userID string, r Request) string { + return generateKey(userID, r, cfg.SplitInterval) } // Extractor is used by the cache to extract a subset of a response from a cache entry. @@ -55,6 +61,18 @@ func (e ExtractorFunc) Extract(start, end int64, from Response) Response { return e(start, end, from) } +// CacheSplitter generates cache keys +type CacheSplitter interface { + GenerateCacheKey(userID string, r Request) string +} + +// constSplitter is a utility for using a constant split interval when determining cache keys +type constSplitter time.Duration + +func (t constSplitter) GenerateCacheKey(userID string, r Request) string { + return generateKey(userID, r, time.Duration(t)) +} + // PrometheusResponseExtractor is an `Extractor` for a Prometheus query range response. var PrometheusResponseExtractor = ExtractorFunc(func(start, end int64, from Response) Response { promRes := from.(*PrometheusResponse) @@ -69,11 +87,12 @@ var PrometheusResponseExtractor = ExtractorFunc(func(start, end int64, from Resp }) type resultsCache struct { - logger log.Logger - cfg ResultsCacheConfig - next Handler - cache cache.Cache - limits Limits + logger log.Logger + cfg ResultsCacheConfig + next Handler + cache cache.Cache + limits Limits + splitter CacheSplitter extractor Extractor merger Merger @@ -85,7 +104,14 @@ type resultsCache struct { // Each request starting from within the same interval will hit the same cache entry. // If the cache doesn't have the entire duration of the request cached, it will query the uncached parts and append them to the cache entries. // see `generateKey`. -func NewResultsCacheMiddleware(logger log.Logger, cfg ResultsCacheConfig, limits Limits, merger Merger, extractor Extractor) (Middleware, cache.Cache, error) { +func NewResultsCacheMiddleware( + logger log.Logger, + cfg ResultsCacheConfig, + splitter CacheSplitter, + limits Limits, + merger Merger, + extractor Extractor, +) (Middleware, cache.Cache, error) { c, err := cache.New(cfg.CacheConfig) if err != nil { return nil, nil, err @@ -100,6 +126,7 @@ func NewResultsCacheMiddleware(logger log.Logger, cfg ResultsCacheConfig, limits limits: limits, merger: merger, extractor: extractor, + splitter: splitter, } }), c, nil } @@ -111,7 +138,7 @@ func (s resultsCache) Do(ctx context.Context, r Request) (Response, error) { } var ( - key = generateKey(userID, r, s.cfg.SplitInterval) + key = s.splitter.GenerateCacheKey(userID, r) extents []Extent response Response ) diff --git a/pkg/querier/queryrange/results_cache_test.go b/pkg/querier/queryrange/results_cache_test.go index 6eb0c3266d..96296f6c73 100644 --- a/pkg/querier/queryrange/results_cache_test.go +++ b/pkg/querier/queryrange/results_cache_test.go @@ -266,14 +266,16 @@ func (fakeLimits) MaxQueryParallelism(string) int { func TestResultsCache(t *testing.T) { calls := 0 + cfg := ResultsCacheConfig{ + CacheConfig: cache.Config{ + Cache: cache.NewMockCache(), + }, + SplitInterval: 24 * time.Hour, + } rcm, _, err := NewResultsCacheMiddleware( log.NewNopLogger(), - ResultsCacheConfig{ - CacheConfig: cache.Config{ - Cache: cache.NewMockCache(), - }, - SplitInterval: 24 * time.Hour, - }, + cfg, + cfg, fakeLimits{}, PrometheusCodec, PrometheusResponseExtractor, @@ -306,8 +308,9 @@ func TestResultsCache(t *testing.T) { func TestResultsCacheRecent(t *testing.T) { var cfg ResultsCacheConfig flagext.DefaultValues(&cfg) + cfg.SplitInterval = 24 * time.Hour cfg.CacheConfig.Cache = cache.NewMockCache() - rcm, _, err := NewResultsCacheMiddleware(log.NewNopLogger(), cfg, fakeLimits{}, PrometheusCodec, PrometheusResponseExtractor) + rcm, _, err := NewResultsCacheMiddleware(log.NewNopLogger(), cfg, cfg, fakeLimits{}, PrometheusCodec, PrometheusResponseExtractor) require.NoError(t, err) req := parsedRequest.WithStartEnd(int64(model.Now())-(60*1e3), int64(model.Now())) @@ -334,14 +337,16 @@ func TestResultsCacheRecent(t *testing.T) { } func Test_resultsCache_MissingData(t *testing.T) { + cfg := ResultsCacheConfig{ + CacheConfig: cache.Config{ + Cache: cache.NewMockCache(), + }, + SplitInterval: 24 * time.Hour, + } rm, _, err := NewResultsCacheMiddleware( log.NewNopLogger(), - ResultsCacheConfig{ - CacheConfig: cache.Config{ - Cache: cache.NewMockCache(), - }, - SplitInterval: 24 * time.Hour, - }, + cfg, + cfg, fakeLimits{}, PrometheusCodec, PrometheusResponseExtractor, diff --git a/pkg/querier/queryrange/roundtrip.go b/pkg/querier/queryrange/roundtrip.go index e9f88537da..84c2fa64fe 100644 --- a/pkg/querier/queryrange/roundtrip.go +++ b/pkg/querier/queryrange/roundtrip.go @@ -46,12 +46,17 @@ type Config struct { func (cfg *Config) RegisterFlags(f *flag.FlagSet) { f.IntVar(&cfg.MaxRetries, "querier.max-retries-per-request", 5, "Maximum number of retries for a single request; beyond this, the downstream error is returned.") f.BoolVar(&cfg.SplitQueriesByDay, "querier.split-queries-by-day", false, "Deprecated: Split queries by day and execute in parallel.") - f.DurationVar(&cfg.SplitQueriesByInterval, "querier.split-queries-by-interval", 0, "Split queries by an interval and execute in parallel, 0 disables it. You should use an a multiple of 24 hours (same as the storage bucketing scheme), to avoid queriers downloading and processing the same chunks.") + f.DurationVar(&cfg.SplitQueriesByInterval, "querier.split-queries-by-interval", 0, "Split queries by an interval and execute in parallel, 0 disables it. You should use an a multiple of 24 hours (same as the storage bucketing scheme), to avoid queriers downloading and processing the same chunks. This also determines how cache keys are chosen when result caching is enabled") f.BoolVar(&cfg.AlignQueriesWithStep, "querier.align-querier-with-step", false, "Mutate incoming queries to align their start and end with their step.") f.BoolVar(&cfg.CacheResults, "querier.cache-results", false, "Cache query results.") cfg.ResultsCacheConfig.RegisterFlags(f) } +// GenerateCacheKey impls CacheSplitter +func (cfg Config) GenerateCacheKey(userID string, r Request) string { + return generateKey(userID, r, cfg.SplitQueriesByInterval) +} + // HandlerFunc is like http.HandlerFunc, but for Handler. type HandlerFunc func(context.Context, Request) (Response, error) @@ -89,6 +94,19 @@ func MergeMiddlewares(middleware ...Middleware) Middleware { }) } +func determineCacheSplitter(cfg Config, log log.Logger) CacheSplitter { + if cfg.ResultsCacheConfig.SplitInterval > 0 { + level.Warn(log).Log("msg", "flag frontend.cache-split-interval (or config cache_split_interval) is deprecated, use querier.split-queries-by-interval instead.") + return cfg.ResultsCacheConfig + } + if cfg.SplitQueriesByInterval > 0 { + return cfg + } + + // by default use 24 hour segments + return constSplitter(24 * time.Hour) +} + // NewTripperware returns a Tripperware configured with middlewares to limit, align, split, retry and cache requests. func NewTripperware(cfg Config, log log.Logger, limits Limits, codec Codec, cacheExtractor Extractor) (frontend.Tripperware, cache.Cache, error) { queryRangeMiddleware := []Middleware{LimitsMiddleware(limits)} @@ -105,7 +123,7 @@ func NewTripperware(cfg Config, log log.Logger, limits Limits, codec Codec, cach } var c cache.Cache if cfg.CacheResults { - queryCacheMiddleware, cache, err := NewResultsCacheMiddleware(log, cfg.ResultsCacheConfig, limits, codec, cacheExtractor) + queryCacheMiddleware, cache, err := NewResultsCacheMiddleware(log, cfg.ResultsCacheConfig, determineCacheSplitter(cfg, log), limits, codec, cacheExtractor) if err != nil { return nil, nil, err } From ac8634f60511cb47324d9b8e75a32bbf52bb5ad4 Mon Sep 17 00:00:00 2001 From: Owen Diehl Date: Mon, 27 Jan 2020 15:58:11 -0500 Subject: [PATCH 2/9] regens docs, updates changelog Signed-off-by: Owen Diehl --- CHANGELOG.md | 1 + docs/configuration/config-file-reference.md | 10 ++++++---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 09af247e7b..5b97385a1e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## master / unreleased +* [CHANGE] Deprecated unnecessary `frontend.cache-split-interval` in favor of `querier.split-queries-by-interval` both to reduce configuration complexity and guarantee alignment of these two configs. * [CHANGE] Removed remaining support for using denormalised tokens in the ring. If you're still running ingesters with denormalised tokens (Cortex 0.4 or earlier, with `-ingester.normalise-tokens=false`), such ingesters will now be completely invisible to distributors and need to be either switched to Cortex 0.6.0 or later, or be configured to use normalised tokens. #2034 * [CHANGE] Moved `--store.min-chunk-age` to the Querier config as `--querier.query-store-after`, allowing the store to be skipped during query time if the metrics wouldn't be found. The YAML config option `ingestermaxquerylookback` has been renamed to `query_ingesters_within` to match its CLI flag. #1893 * `--store.min-chunk-age` has been removed diff --git a/docs/configuration/config-file-reference.md b/docs/configuration/config-file-reference.md index cb6351032c..8baf847cf1 100644 --- a/docs/configuration/config-file-reference.md +++ b/docs/configuration/config-file-reference.md @@ -550,7 +550,8 @@ The `queryrange_config` configures the query splitting and caching in the Cortex ```yaml # Split queries by an interval and execute in parallel, 0 disables it. You # should use an a multiple of 24 hours (same as the storage bucketing scheme), -# to avoid queriers downloading and processing the same chunks. +# to avoid queriers downloading and processing the same chunks. This also +# determines how cache keys are chosen when result caching is enabled # CLI flag: -querier.split-queries-by-interval [split_queries_by_interval: | default = 0s] @@ -604,10 +605,11 @@ results_cache: # CLI flag: -frontend.max-cache-freshness [max_freshness: | default = 1m0s] - # The maximum interval expected for each request, results will be cached per - # single interval. + # Deprecated: The maximum interval expected for each request, results will be + # cached per single interval. This flag has been deprecated and this behavior + # is now determined by querier.split-queries-by-interval # CLI flag: -frontend.cache-split-interval - [cache_split_interval: | default = 24h0m0s] + [cache_split_interval: | default = 0s] # Cache query results. # CLI flag: -querier.cache-results From 64932d24f96281351a74089183f38eeb86165e56 Mon Sep 17 00:00:00 2001 From: Owen Diehl Date: Mon, 27 Jan 2020 16:06:48 -0500 Subject: [PATCH 3/9] adds PR # Signed-off-by: Owen Diehl --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b97385a1e..afea4b95e5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## master / unreleased -* [CHANGE] Deprecated unnecessary `frontend.cache-split-interval` in favor of `querier.split-queries-by-interval` both to reduce configuration complexity and guarantee alignment of these two configs. +* [CHANGE] Deprecated unnecessary `frontend.cache-split-interval` in favor of `querier.split-queries-by-interval` both to reduce configuration complexity and guarantee alignment of these two configs. #2040 * [CHANGE] Removed remaining support for using denormalised tokens in the ring. If you're still running ingesters with denormalised tokens (Cortex 0.4 or earlier, with `-ingester.normalise-tokens=false`), such ingesters will now be completely invisible to distributors and need to be either switched to Cortex 0.6.0 or later, or be configured to use normalised tokens. #2034 * [CHANGE] Moved `--store.min-chunk-age` to the Querier config as `--querier.query-store-after`, allowing the store to be skipped during query time if the metrics wouldn't be found. The YAML config option `ingestermaxquerylookback` has been renamed to `query_ingesters_within` to match its CLI flag. #1893 * `--store.min-chunk-age` has been removed @@ -20,6 +20,7 @@ Cortex 0.6.0 is the last version that can *read* denormalised tokens. Starting w Note that the ruler flags need to be changed in this upgrade. You're moving from a single node ruler to something that might need to be sharded. Further, if you're using the configs service, we've upgraded the migration library and this requires some manual intervention. See full instructions below to upgrade your PostgreSQL. + * [CHANGE] The frontend component now does not cache results if it finds a `Cache-Control` header and if one of its values is `no-store`. #1974 * [CHANGE] Flags changed with transition to upstream Prometheus rules manager: * `-ruler.client-timeout` is now `ruler.configs.client-timeout` in order to match `ruler.configs.url`. From 4e8794d6e72501432a132af212a1423399ca8458 Mon Sep 17 00:00:00 2001 From: Owen Diehl Date: Tue, 28 Jan 2020 14:31:43 -0500 Subject: [PATCH 4/9] removes frontend.cache-split-interval Signed-off-by: Owen Diehl --- CHANGELOG.md | 3 +-- pkg/cortex/cortex.go | 3 +++ pkg/querier/queryrange/results_cache.go | 20 ++++---------- pkg/querier/queryrange/results_cache_test.go | 9 +++---- pkg/querier/queryrange/roundtrip.go | 28 +++++++++----------- 5 files changed, 25 insertions(+), 38 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index afea4b95e5..c10ee15325 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## master / unreleased -* [CHANGE] Deprecated unnecessary `frontend.cache-split-interval` in favor of `querier.split-queries-by-interval` both to reduce configuration complexity and guarantee alignment of these two configs. #2040 +* [CHANGE] Removed unnecessary `frontend.cache-split-interval` in favor of `querier.split-queries-by-interval` both to reduce configuration complexity and guarantee alignment of these two configs. #2040 * [CHANGE] Removed remaining support for using denormalised tokens in the ring. If you're still running ingesters with denormalised tokens (Cortex 0.4 or earlier, with `-ingester.normalise-tokens=false`), such ingesters will now be completely invisible to distributors and need to be either switched to Cortex 0.6.0 or later, or be configured to use normalised tokens. #2034 * [CHANGE] Moved `--store.min-chunk-age` to the Querier config as `--querier.query-store-after`, allowing the store to be skipped during query time if the metrics wouldn't be found. The YAML config option `ingestermaxquerylookback` has been renamed to `query_ingesters_within` to match its CLI flag. #1893 * `--store.min-chunk-age` has been removed @@ -20,7 +20,6 @@ Cortex 0.6.0 is the last version that can *read* denormalised tokens. Starting w Note that the ruler flags need to be changed in this upgrade. You're moving from a single node ruler to something that might need to be sharded. Further, if you're using the configs service, we've upgraded the migration library and this requires some manual intervention. See full instructions below to upgrade your PostgreSQL. - * [CHANGE] The frontend component now does not cache results if it finds a `Cache-Control` header and if one of its values is `no-store`. #1974 * [CHANGE] Flags changed with transition to upstream Prometheus rules manager: * `-ruler.client-timeout` is now `ruler.configs.client-timeout` in order to match `ruler.configs.url`. diff --git a/pkg/cortex/cortex.go b/pkg/cortex/cortex.go index 48576fd84f..bcc19bc488 100644 --- a/pkg/cortex/cortex.go +++ b/pkg/cortex/cortex.go @@ -147,6 +147,9 @@ func (c *Config) Validate() error { if err := c.Querier.Validate(); err != nil { return errors.Wrap(err, "invalid querier config") } + if err := c.QueryRange.Validate(); err != nil { + return errors.Wrap(err, "invalid queryrange config") + } return nil } diff --git a/pkg/querier/queryrange/results_cache.go b/pkg/querier/queryrange/results_cache.go index f3d327fcee..9c3a8fae0a 100644 --- a/pkg/querier/queryrange/results_cache.go +++ b/pkg/querier/queryrange/results_cache.go @@ -19,6 +19,7 @@ import ( "github.com/cortexproject/cortex/pkg/chunk/cache" "github.com/cortexproject/cortex/pkg/ingester/client" + "github.com/cortexproject/cortex/pkg/util/flagext" "github.com/cortexproject/cortex/pkg/util/spanlogger" ) @@ -31,20 +32,15 @@ var ( type ResultsCacheConfig struct { CacheConfig cache.Config `yaml:"cache"` MaxCacheFreshness time.Duration `yaml:"max_freshness"` - SplitInterval time.Duration `yaml:"cache_split_interval"` } // RegisterFlags registers flags. func (cfg *ResultsCacheConfig) RegisterFlags(f *flag.FlagSet) { cfg.CacheConfig.RegisterFlagsWithPrefix("frontend.", "", f) - f.DurationVar(&cfg.MaxCacheFreshness, "frontend.max-cache-freshness", 1*time.Minute, "Most recent allowed cacheable result, to prevent caching very recent results that might still be in flux.") - f.DurationVar(&cfg.SplitInterval, "frontend.cache-split-interval", 0, "Deprecated: The maximum interval expected for each request, results will be cached per single interval. This flag has been deprecated and this behavior is now determined by querier.split-queries-by-interval") -} + flagext.DeprecatedFlag(f, "frontend.cache-split-interval", "Deprecated: The maximum interval expected for each request, results will be cached per single interval. This behavior is now determined by querier.split-queries-by-interval.") -// GenerateCacheKey impls CacheSplitter -func (cfg ResultsCacheConfig) GenerateCacheKey(userID string, r Request) string { - return generateKey(userID, r, cfg.SplitInterval) + f.DurationVar(&cfg.MaxCacheFreshness, "frontend.max-cache-freshness", 1*time.Minute, "Most recent allowed cacheable result, to prevent caching very recent results that might still be in flux.") } // Extractor is used by the cache to extract a subset of a response from a cache entry. @@ -61,18 +57,12 @@ func (e ExtractorFunc) Extract(start, end int64, from Response) Response { return e(start, end, from) } -// CacheSplitter generates cache keys +// CacheSplitter generates cache keys. This is a useful interface for downstream +// consumers who wish to impl their own strategies. type CacheSplitter interface { GenerateCacheKey(userID string, r Request) string } -// constSplitter is a utility for using a constant split interval when determining cache keys -type constSplitter time.Duration - -func (t constSplitter) GenerateCacheKey(userID string, r Request) string { - return generateKey(userID, r, time.Duration(t)) -} - // PrometheusResponseExtractor is an `Extractor` for a Prometheus query range response. var PrometheusResponseExtractor = ExtractorFunc(func(start, end int64, from Response) Response { promRes := from.(*PrometheusResponse) diff --git a/pkg/querier/queryrange/results_cache_test.go b/pkg/querier/queryrange/results_cache_test.go index 96296f6c73..e13142ae46 100644 --- a/pkg/querier/queryrange/results_cache_test.go +++ b/pkg/querier/queryrange/results_cache_test.go @@ -270,12 +270,11 @@ func TestResultsCache(t *testing.T) { CacheConfig: cache.Config{ Cache: cache.NewMockCache(), }, - SplitInterval: 24 * time.Hour, } rcm, _, err := NewResultsCacheMiddleware( log.NewNopLogger(), cfg, - cfg, + Config{SplitQueriesByInterval: 24 * time.Hour}, fakeLimits{}, PrometheusCodec, PrometheusResponseExtractor, @@ -308,9 +307,8 @@ func TestResultsCache(t *testing.T) { func TestResultsCacheRecent(t *testing.T) { var cfg ResultsCacheConfig flagext.DefaultValues(&cfg) - cfg.SplitInterval = 24 * time.Hour cfg.CacheConfig.Cache = cache.NewMockCache() - rcm, _, err := NewResultsCacheMiddleware(log.NewNopLogger(), cfg, cfg, fakeLimits{}, PrometheusCodec, PrometheusResponseExtractor) + rcm, _, err := NewResultsCacheMiddleware(log.NewNopLogger(), cfg, Config{SplitQueriesByInterval: 24 * time.Hour}, fakeLimits{}, PrometheusCodec, PrometheusResponseExtractor) require.NoError(t, err) req := parsedRequest.WithStartEnd(int64(model.Now())-(60*1e3), int64(model.Now())) @@ -341,12 +339,11 @@ func Test_resultsCache_MissingData(t *testing.T) { CacheConfig: cache.Config{ Cache: cache.NewMockCache(), }, - SplitInterval: 24 * time.Hour, } rm, _, err := NewResultsCacheMiddleware( log.NewNopLogger(), cfg, - cfg, + Config{SplitQueriesByInterval: 24 * time.Hour}, fakeLimits{}, PrometheusCodec, PrometheusResponseExtractor, diff --git a/pkg/querier/queryrange/roundtrip.go b/pkg/querier/queryrange/roundtrip.go index 84c2fa64fe..6ad2839938 100644 --- a/pkg/querier/queryrange/roundtrip.go +++ b/pkg/querier/queryrange/roundtrip.go @@ -17,6 +17,7 @@ package queryrange import ( "context" + "errors" "flag" "net/http" "strings" @@ -52,6 +53,17 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) { cfg.ResultsCacheConfig.RegisterFlags(f) } +func (cfg *Config) Validate() error { + if cfg.SplitQueriesByDay { + cfg.SplitQueriesByInterval = day + } + + if cfg.CacheResults && cfg.SplitQueriesByInterval <= 0 { + return errors.New("querier.cache-results may only be enabled in conjunction with querier.split-queries-by-interval. Please set the latter.") + } + return nil +} + // GenerateCacheKey impls CacheSplitter func (cfg Config) GenerateCacheKey(userID string, r Request) string { return generateKey(userID, r, cfg.SplitQueriesByInterval) @@ -94,19 +106,6 @@ func MergeMiddlewares(middleware ...Middleware) Middleware { }) } -func determineCacheSplitter(cfg Config, log log.Logger) CacheSplitter { - if cfg.ResultsCacheConfig.SplitInterval > 0 { - level.Warn(log).Log("msg", "flag frontend.cache-split-interval (or config cache_split_interval) is deprecated, use querier.split-queries-by-interval instead.") - return cfg.ResultsCacheConfig - } - if cfg.SplitQueriesByInterval > 0 { - return cfg - } - - // by default use 24 hour segments - return constSplitter(24 * time.Hour) -} - // NewTripperware returns a Tripperware configured with middlewares to limit, align, split, retry and cache requests. func NewTripperware(cfg Config, log log.Logger, limits Limits, codec Codec, cacheExtractor Extractor) (frontend.Tripperware, cache.Cache, error) { queryRangeMiddleware := []Middleware{LimitsMiddleware(limits)} @@ -116,14 +115,13 @@ func NewTripperware(cfg Config, log log.Logger, limits Limits, codec Codec, cach // SplitQueriesByDay is deprecated use SplitQueriesByInterval. if cfg.SplitQueriesByDay { level.Warn(log).Log("msg", "flag querier.split-queries-by-day (or config split_queries_by_day) is deprecated, use querier.split-queries-by-interval instead.") - cfg.SplitQueriesByInterval = day } if cfg.SplitQueriesByInterval != 0 { queryRangeMiddleware = append(queryRangeMiddleware, InstrumentMiddleware("split_by_interval"), SplitByIntervalMiddleware(cfg.SplitQueriesByInterval, limits, codec)) } var c cache.Cache if cfg.CacheResults { - queryCacheMiddleware, cache, err := NewResultsCacheMiddleware(log, cfg.ResultsCacheConfig, determineCacheSplitter(cfg, log), limits, codec, cacheExtractor) + queryCacheMiddleware, cache, err := NewResultsCacheMiddleware(log, cfg.ResultsCacheConfig, cfg, limits, codec, cacheExtractor) if err != nil { return nil, nil, err } From bba96abad4d0de4c09f454ba4ef467ee1b95be85 Mon Sep 17 00:00:00 2001 From: Owen Diehl Date: Tue, 28 Jan 2020 14:41:26 -0500 Subject: [PATCH 5/9] regens docs Signed-off-by: Owen Diehl --- docs/configuration/config-file-reference.md | 6 ------ pkg/querier/queryrange/roundtrip.go | 2 +- 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/docs/configuration/config-file-reference.md b/docs/configuration/config-file-reference.md index 8baf847cf1..4201e85c97 100644 --- a/docs/configuration/config-file-reference.md +++ b/docs/configuration/config-file-reference.md @@ -605,12 +605,6 @@ results_cache: # CLI flag: -frontend.max-cache-freshness [max_freshness: | default = 1m0s] - # Deprecated: The maximum interval expected for each request, results will be - # cached per single interval. This flag has been deprecated and this behavior - # is now determined by querier.split-queries-by-interval - # CLI flag: -frontend.cache-split-interval - [cache_split_interval: | default = 0s] - # Cache query results. # CLI flag: -querier.cache-results [cache_results: | default = false] diff --git a/pkg/querier/queryrange/roundtrip.go b/pkg/querier/queryrange/roundtrip.go index 6ad2839938..620dddde96 100644 --- a/pkg/querier/queryrange/roundtrip.go +++ b/pkg/querier/queryrange/roundtrip.go @@ -59,7 +59,7 @@ func (cfg *Config) Validate() error { } if cfg.CacheResults && cfg.SplitQueriesByInterval <= 0 { - return errors.New("querier.cache-results may only be enabled in conjunction with querier.split-queries-by-interval. Please set the latter.") + return errors.New("querier.cache-results may only be enabled in conjunction with querier.split-queries-by-interval. Please set the latter") } return nil } From 6d3e98bb6beac9feb71ea6f64b6cd2b4e00fe2c3 Mon Sep 17 00:00:00 2001 From: Owen Diehl Date: Wed, 29 Jan 2020 13:49:56 -0500 Subject: [PATCH 6/9] addresses pr comments, config no longer satisfies CacheSplitter ifc Signed-off-by: Owen Diehl --- CHANGELOG.md | 2 +- pkg/querier/queryrange/results_cache.go | 17 ++++++++++------- pkg/querier/queryrange/results_cache_test.go | 8 ++++---- pkg/querier/queryrange/roundtrip.go | 7 +------ 4 files changed, 16 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c10ee15325..38d1fbb469 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## master / unreleased -* [CHANGE] Removed unnecessary `frontend.cache-split-interval` in favor of `querier.split-queries-by-interval` both to reduce configuration complexity and guarantee alignment of these two configs. #2040 +* [CHANGE] Removed unnecessary `frontend.cache-split-interval` in favor of `querier.split-queries-by-interval` both to reduce configuration complexity and guarantee alignment of these two configs. Starting from now, `-querier.cache-results` may only be enabled in conjunction with `-querier.split-queries-by-interval` (previously the cache interval default was `24h` so if you want to preserve the same behaviour you should set `-querier.split-queries-by-interval=24h`). #2040 * [CHANGE] Removed remaining support for using denormalised tokens in the ring. If you're still running ingesters with denormalised tokens (Cortex 0.4 or earlier, with `-ingester.normalise-tokens=false`), such ingesters will now be completely invisible to distributors and need to be either switched to Cortex 0.6.0 or later, or be configured to use normalised tokens. #2034 * [CHANGE] Moved `--store.min-chunk-age` to the Querier config as `--querier.query-store-after`, allowing the store to be skipped during query time if the metrics wouldn't be found. The YAML config option `ingestermaxquerylookback` has been renamed to `query_ingesters_within` to match its CLI flag. #1893 * `--store.min-chunk-age` has been removed diff --git a/pkg/querier/queryrange/results_cache.go b/pkg/querier/queryrange/results_cache.go index 9c3a8fae0a..7bcbedebe4 100644 --- a/pkg/querier/queryrange/results_cache.go +++ b/pkg/querier/queryrange/results_cache.go @@ -58,11 +58,20 @@ func (e ExtractorFunc) Extract(start, end int64, from Response) Response { } // CacheSplitter generates cache keys. This is a useful interface for downstream -// consumers who wish to impl their own strategies. +// consumers who wish to implement their own strategies. type CacheSplitter interface { GenerateCacheKey(userID string, r Request) string } +// constSplitter is a utility for using a constant split interval when determining cache keys +type constSplitter time.Duration + +// GenerateCacheKey generates a cache key based on the userID, Request and interval. +func (t constSplitter) GenerateCacheKey(userID string, r Request) string { + currentInterval := r.GetStart() / int64(time.Duration(t)/time.Millisecond) + return fmt.Sprintf("%s:%s:%d:%d", userID, r.GetQuery(), r.GetStep(), currentInterval) +} + // PrometheusResponseExtractor is an `Extractor` for a Prometheus query range response. var PrometheusResponseExtractor = ExtractorFunc(func(start, end int64, from Response) Response { promRes := from.(*PrometheusResponse) @@ -376,12 +385,6 @@ func (s resultsCache) filterRecentExtents(req Request, extents []Extent) ([]Exte return extents, nil } -// generateKey generates a cache key based on the userID, Request and interval. -func generateKey(userID string, r Request, interval time.Duration) string { - currentInterval := r.GetStart() / int64(interval/time.Millisecond) - return fmt.Sprintf("%s:%s:%d:%d", userID, r.GetQuery(), r.GetStep(), currentInterval) -} - func (s resultsCache) get(ctx context.Context, key string) ([]Extent, bool) { found, bufs, _ := s.cache.Fetch(ctx, []string{cache.HashKey(key)}) if len(found) != 1 { diff --git a/pkg/querier/queryrange/results_cache_test.go b/pkg/querier/queryrange/results_cache_test.go index e13142ae46..b3e574261b 100644 --- a/pkg/querier/queryrange/results_cache_test.go +++ b/pkg/querier/queryrange/results_cache_test.go @@ -274,7 +274,7 @@ func TestResultsCache(t *testing.T) { rcm, _, err := NewResultsCacheMiddleware( log.NewNopLogger(), cfg, - Config{SplitQueriesByInterval: 24 * time.Hour}, + constSplitter(24*time.Hour), fakeLimits{}, PrometheusCodec, PrometheusResponseExtractor, @@ -308,7 +308,7 @@ func TestResultsCacheRecent(t *testing.T) { var cfg ResultsCacheConfig flagext.DefaultValues(&cfg) cfg.CacheConfig.Cache = cache.NewMockCache() - rcm, _, err := NewResultsCacheMiddleware(log.NewNopLogger(), cfg, Config{SplitQueriesByInterval: 24 * time.Hour}, fakeLimits{}, PrometheusCodec, PrometheusResponseExtractor) + rcm, _, err := NewResultsCacheMiddleware(log.NewNopLogger(), cfg, constSplitter(24*time.Hour), fakeLimits{}, PrometheusCodec, PrometheusResponseExtractor) require.NoError(t, err) req := parsedRequest.WithStartEnd(int64(model.Now())-(60*1e3), int64(model.Now())) @@ -343,7 +343,7 @@ func Test_resultsCache_MissingData(t *testing.T) { rm, _, err := NewResultsCacheMiddleware( log.NewNopLogger(), cfg, - Config{SplitQueriesByInterval: 24 * time.Hour}, + constSplitter(24*time.Hour), fakeLimits{}, PrometheusCodec, PrometheusResponseExtractor, @@ -398,7 +398,7 @@ func Test_generateKey(t *testing.T) { } for _, tt := range tests { t.Run(fmt.Sprintf("%s - %s", tt.name, tt.interval), func(t *testing.T) { - if got := generateKey("fake", tt.r, tt.interval); got != tt.want { + if got := constSplitter(tt.interval).GenerateCacheKey("fake", tt.r); got != tt.want { t.Errorf("generateKey() = %v, want %v", got, tt.want) } }) diff --git a/pkg/querier/queryrange/roundtrip.go b/pkg/querier/queryrange/roundtrip.go index 620dddde96..a76881e5e5 100644 --- a/pkg/querier/queryrange/roundtrip.go +++ b/pkg/querier/queryrange/roundtrip.go @@ -64,11 +64,6 @@ func (cfg *Config) Validate() error { return nil } -// GenerateCacheKey impls CacheSplitter -func (cfg Config) GenerateCacheKey(userID string, r Request) string { - return generateKey(userID, r, cfg.SplitQueriesByInterval) -} - // HandlerFunc is like http.HandlerFunc, but for Handler. type HandlerFunc func(context.Context, Request) (Response, error) @@ -121,7 +116,7 @@ func NewTripperware(cfg Config, log log.Logger, limits Limits, codec Codec, cach } var c cache.Cache if cfg.CacheResults { - queryCacheMiddleware, cache, err := NewResultsCacheMiddleware(log, cfg.ResultsCacheConfig, cfg, limits, codec, cacheExtractor) + queryCacheMiddleware, cache, err := NewResultsCacheMiddleware(log, cfg.ResultsCacheConfig, constSplitter(cfg.SplitQueriesByInterval), limits, codec, cacheExtractor) if err != nil { return nil, nil, err } From 33894b2a3944d45ce5c662a320427cabf27d6b07 Mon Sep 17 00:00:00 2001 From: Owen Diehl Date: Thu, 30 Jan 2020 08:33:51 -0500 Subject: [PATCH 7/9] passes logger to config validation to log warning/mutation Signed-off-by: Owen Diehl --- cmd/cortex/main.go | 4 ++-- pkg/cortex/cortex.go | 5 +++-- pkg/querier/queryrange/roundtrip.go | 8 +++----- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/cmd/cortex/main.go b/cmd/cortex/main.go index 7540d0fbb9..bd45e3a053 100644 --- a/cmd/cortex/main.go +++ b/cmd/cortex/main.go @@ -73,9 +73,10 @@ func main() { runtime.SetMutexProfileFraction(mutexProfileFraction) } + util.InitLogger(&cfg.Server) // Validate the config once both the config file has been loaded // and CLI flags parsed. - err := cfg.Validate() + err := cfg.Validate(util.Logger) if err != nil { fmt.Printf("error validating config: %v\n", err) os.Exit(1) @@ -84,7 +85,6 @@ func main() { // Allocate a block of memory to alter GC behaviour. See https://github.com/golang/go/issues/23044 ballast := make([]byte, ballastBytes) - util.InitLogger(&cfg.Server) util.InitEvents(eventSampleRate) // Setting the environment variable JAEGER_AGENT_HOST enables tracing diff --git a/pkg/cortex/cortex.go b/pkg/cortex/cortex.go index bcc19bc488..f8a37431f6 100644 --- a/pkg/cortex/cortex.go +++ b/pkg/cortex/cortex.go @@ -5,6 +5,7 @@ import ( "fmt" "os" + "github.com/go-kit/kit/log" "github.com/go-kit/kit/log/level" "github.com/pkg/errors" "github.com/weaveworks/common/middleware" @@ -125,7 +126,7 @@ func (c *Config) RegisterFlags(f *flag.FlagSet) { // Validate the cortex config and returns an error if the validation // doesn't pass -func (c *Config) Validate() error { +func (c *Config) Validate(log log.Logger) error { if err := c.Schema.Validate(); err != nil { return errors.Wrap(err, "invalid schema config") } @@ -147,7 +148,7 @@ func (c *Config) Validate() error { if err := c.Querier.Validate(); err != nil { return errors.Wrap(err, "invalid querier config") } - if err := c.QueryRange.Validate(); err != nil { + if err := c.QueryRange.Validate(log); err != nil { return errors.Wrap(err, "invalid queryrange config") } return nil diff --git a/pkg/querier/queryrange/roundtrip.go b/pkg/querier/queryrange/roundtrip.go index a76881e5e5..6e87d78c6e 100644 --- a/pkg/querier/queryrange/roundtrip.go +++ b/pkg/querier/queryrange/roundtrip.go @@ -53,9 +53,11 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) { cfg.ResultsCacheConfig.RegisterFlags(f) } -func (cfg *Config) Validate() error { +func (cfg *Config) Validate(log log.Logger) error { + // SplitQueriesByDay is deprecated use SplitQueriesByInterval. if cfg.SplitQueriesByDay { cfg.SplitQueriesByInterval = day + level.Warn(log).Log("msg", "flag querier.split-queries-by-day (or config split_queries_by_day) is deprecated, use querier.split-queries-by-interval instead.") } if cfg.CacheResults && cfg.SplitQueriesByInterval <= 0 { @@ -107,10 +109,6 @@ func NewTripperware(cfg Config, log log.Logger, limits Limits, codec Codec, cach if cfg.AlignQueriesWithStep { queryRangeMiddleware = append(queryRangeMiddleware, InstrumentMiddleware("step_align"), StepAlignMiddleware) } - // SplitQueriesByDay is deprecated use SplitQueriesByInterval. - if cfg.SplitQueriesByDay { - level.Warn(log).Log("msg", "flag querier.split-queries-by-day (or config split_queries_by_day) is deprecated, use querier.split-queries-by-interval instead.") - } if cfg.SplitQueriesByInterval != 0 { queryRangeMiddleware = append(queryRangeMiddleware, InstrumentMiddleware("split_by_interval"), SplitByIntervalMiddleware(cfg.SplitQueriesByInterval, limits, codec)) } From cd85becd2a428cbe5fc453f7e8e7c82da0d5724d Mon Sep 17 00:00:00 2001 From: Owen Diehl Date: Thu, 30 Jan 2020 09:01:26 -0500 Subject: [PATCH 8/9] uses day const in tests Signed-off-by: Owen Diehl --- pkg/querier/queryrange/results_cache_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/querier/queryrange/results_cache_test.go b/pkg/querier/queryrange/results_cache_test.go index b3e574261b..3df0c428a1 100644 --- a/pkg/querier/queryrange/results_cache_test.go +++ b/pkg/querier/queryrange/results_cache_test.go @@ -274,7 +274,7 @@ func TestResultsCache(t *testing.T) { rcm, _, err := NewResultsCacheMiddleware( log.NewNopLogger(), cfg, - constSplitter(24*time.Hour), + constSplitter(day), fakeLimits{}, PrometheusCodec, PrometheusResponseExtractor, @@ -308,7 +308,7 @@ func TestResultsCacheRecent(t *testing.T) { var cfg ResultsCacheConfig flagext.DefaultValues(&cfg) cfg.CacheConfig.Cache = cache.NewMockCache() - rcm, _, err := NewResultsCacheMiddleware(log.NewNopLogger(), cfg, constSplitter(24*time.Hour), fakeLimits{}, PrometheusCodec, PrometheusResponseExtractor) + rcm, _, err := NewResultsCacheMiddleware(log.NewNopLogger(), cfg, constSplitter(day), fakeLimits{}, PrometheusCodec, PrometheusResponseExtractor) require.NoError(t, err) req := parsedRequest.WithStartEnd(int64(model.Now())-(60*1e3), int64(model.Now())) @@ -343,7 +343,7 @@ func Test_resultsCache_MissingData(t *testing.T) { rm, _, err := NewResultsCacheMiddleware( log.NewNopLogger(), cfg, - constSplitter(24*time.Hour), + constSplitter(day), fakeLimits{}, PrometheusCodec, PrometheusResponseExtractor, From 9c69da2a987a291b04db0875ea01a6fd5c17701d Mon Sep 17 00:00:00 2001 From: Owen Diehl Date: Thu, 30 Jan 2020 09:04:13 -0500 Subject: [PATCH 9/9] consistent renaming for constSplitter tests Signed-off-by: Owen Diehl --- pkg/querier/queryrange/results_cache_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/querier/queryrange/results_cache_test.go b/pkg/querier/queryrange/results_cache_test.go index 3df0c428a1..5a2d3c89e8 100644 --- a/pkg/querier/queryrange/results_cache_test.go +++ b/pkg/querier/queryrange/results_cache_test.go @@ -378,7 +378,7 @@ func Test_resultsCache_MissingData(t *testing.T) { require.False(t, hit) } -func Test_generateKey(t *testing.T) { +func TestConstSplitter_generateCacheKey(t *testing.T) { t.Parallel() tests := []struct {