Update prometheus to latest by aaron7 · Pull Request #763 · cortexproject/cortex

aaron7 · 2018-03-21T16:00:04Z

Updated github.com/prometheus/prometheus to latest
Updated github.com/prometheus/tsdb to latest
make update-vendor
Fixed API changes
Created querier.Config for MaxConcurrent and Timeout

Fixes #762 via prometheus/prometheus#3995 - fsnotify was moved and broke dep here.

bboreham · 2018-03-22T13:02:20Z

Needs some tweak to compile.

bboreham · 2018-03-23T17:51:43Z

OK, maybe more than tweaks.
Needs Go 1.10
Various changes to APIs.

bboreham · 2018-03-23T18:11:56Z

I made those changes. I did not look more closely whether the underlying Prometheus changes let us remove some shim layers.

aaron7 · 2018-03-26T12:53:37Z

I will take a look at test failures now.

bboreham · 2018-03-26T14:05:37Z

cmd/querier/main.go


-	engine := promql.NewEngine(sampleQueryable, nil)
+	maxConcurrent := 20
+	timeout := 2 * time.Minute


Both of these should be command-line parameters.

let's put these in querier config

bboreham · 2018-03-26T14:05:43Z

cmd/querier/main.go

 		querier.DummyTargetRetriever{},
 		querier.DummyAlertmanagerRetriever{},
 		func() config.Config { return config.Config{} },
+		map[string]string{}, // TODO: include configuration flags


this is a recent addition to allow flags to be shown via api endpoint - prometheus/prometheus#3864

We will need to include querier flags here.

OK; I think we can happily pass blank.

aaron7 · 2018-03-26T14:09:10Z

@bboreham I think the change you made here: https://github.com/weaveworks/cortex/pull/763/files#diff-bf05432384ae7a97e67ae71efaf68ffcR201 requires slightly different args.

cfg.NumWorkers - defaults to 1 - promql default engine has 20
cfg.GroupTimeout - defaults to 10s - promql default engine has 2 minutes

aaron7 · 2018-03-26T14:27:51Z

Created querier.Config for query engine config and used in lite, querier and ruler with default values of 20 max concurrent queries and 2 minute query timeout.

bboreham · 2018-03-26T16:01:30Z

Why would we particularly want to follow the promql defaults?
The ruler already has its own configuration; we should use it.

aaron7 · 2018-03-26T16:19:47Z

Why would we particularly want to follow the promql defaults?
The ruler already has its own configuration; we should use it.

The ruler config has:

ruler.num-workers default: 1 (we override to 10)- Number of rule evaluator worker routines in this process
ruler.group-timeout default: 10s (we override to 30s) - Timeout for rule group evaluation, including sending result to ingester

The new parameters we need to pass:

number of concurrent promql.Engine queries (prom default is 20)
timeout for individual queries (prom defaut is 2 mins)

ruler.num-workers seems different to number of concurrent engine queries. Should we share this?

Is the ruler.group-timeout default 10s suitable for query timeouts? We override to 30s, is this suitable? It seems like the answer is yes (there are some open GH issues about wanting to reduce the query timeout).

If we do share these values, should the config names be more generic and not ruler specific?

bboreham · 2018-03-26T17:30:40Z

The number of workers in the ruler is also the maximum number of queries the ruler will issue at the same time. We don’t need another bound.

The group timeout is an upper bound on the time allowed for a single query; this seems close enough to just use it, in the ruler.

The querier is different. I can’t see an alternative to adding parameters for it. Maybe unify with the ruler, but removing the ruler-specific ones has to be done carefully.

bboreham · 2018-04-05T15:01:09Z

Is it OK if I pick this up and fix it?

aaron7 · 2018-04-06T13:25:50Z

Sorry, other stuff took priority after on-call.

I pushed the change to reuse ruler config - after looking at how the ruler evals groups (sequentially) I agree we should be reusing ruler params.

Is it OK if I pick this up and fix it?

If there are any other changes to make, sure, please feel free to make them and I can then review.

bboreham · 2018-04-06T14:51:12Z

Thanks! I added one commit, taking out a couple of TODOs

bboreham · 2018-04-06T15:02:30Z

Couple of odd-looking files under vendor/github.com/json-iterator/go/ in that last commit.

aaron7 · 2018-04-06T15:10:18Z

Couple of odd-looking files under vendor/github.com/json-iterator/go/ in that last commit.

Yep, these were removed in last commit after dep ensure - they shouldn't have been added during the rebase.

This reverts commit 6252335. The new Prometheus web api is incompatible with the scheme Cortex uses to differentiate sample from metadata queries.

bboreham · 2018-04-06T17:09:32Z

Reverted in f2a3355 - see #787

aaron7 force-pushed the 762-update-prometheus branch from f798758 to 0571f24 Compare March 26, 2018 13:47

aaron7 requested a review from bboreham March 26, 2018 13:55

aaron7 force-pushed the 762-update-prometheus branch from 0571f24 to 871ab87 Compare March 26, 2018 13:59

bboreham reviewed Mar 26, 2018

View reviewed changes

aaron7 requested a review from tomwilkie March 26, 2018 14:29

aaron7 force-pushed the 762-update-prometheus branch 6 times, most recently from aaea966 to 883c457 Compare March 26, 2018 15:57

aaron7 and others added 7 commits April 6, 2018 14:22

Update prometheus to latest

e9760c0

Upgrade to Go 1.10

89124d0

Fix up for API changes

03eb954

Update prometheus tsdb

9ff4c80

Update promql.NewEngine and v1.NewAPI

a1c542a

Create querier.Config

a0bac59

Reuse ruler config for ruler querier engine

1319a27

aaron7 force-pushed the 762-update-prometheus branch from 81d1588 to 1319a27 Compare April 6, 2018 13:24

Clarify passing no flags map to NewEngine()

746f903

bboreham approved these changes Apr 6, 2018

View reviewed changes

Update Gopkg.lock

19d13c7

aaron7 merged commit 6252335 into master Apr 6, 2018

aaron7 deleted the 762-update-prometheus branch April 6, 2018 15:11

bboreham added a commit that referenced this pull request Apr 6, 2018

Revert "Update prometheus to latest (#763)"

f2a3355

This reverts commit 6252335. The new Prometheus web api is incompatible with the scheme Cortex uses to differentiate sample from metadata queries.

aaron7 mentioned this pull request Apr 6, 2018

dep broken because of moved fsnotify package #762

Closed

bboreham mentioned this pull request Apr 6, 2018

Cortex queryable is incompatible with latest Prometheus #787

Closed

bboreham mentioned this pull request Apr 18, 2018

Update prometheus #802

Merged

Conversation

aaron7 commented Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bboreham commented Mar 22, 2018

Uh oh!

bboreham commented Mar 23, 2018

Uh oh!

bboreham commented Mar 23, 2018

Uh oh!

aaron7 commented Mar 26, 2018

Uh oh!

bboreham Mar 26, 2018

Choose a reason for hiding this comment

Uh oh!

aaron7 Mar 26, 2018

Choose a reason for hiding this comment

Uh oh!

bboreham Mar 26, 2018

Choose a reason for hiding this comment

Uh oh!

aaron7 Mar 26, 2018

Choose a reason for hiding this comment

Uh oh!

bboreham Mar 26, 2018

Choose a reason for hiding this comment

Uh oh!

aaron7 commented Mar 26, 2018

Uh oh!

aaron7 commented Mar 26, 2018

Uh oh!

bboreham commented Mar 26, 2018

Uh oh!

aaron7 commented Mar 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bboreham commented Mar 26, 2018

Uh oh!

bboreham commented Apr 5, 2018

Uh oh!

aaron7 commented Apr 6, 2018

Uh oh!

bboreham commented Apr 6, 2018

Uh oh!

bboreham commented Apr 6, 2018

Uh oh!

aaron7 commented Apr 6, 2018

Uh oh!

bboreham commented Apr 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aaron7 commented Mar 21, 2018 •

edited

Loading

aaron7 commented Mar 26, 2018 •

edited

Loading