Update prometheus to v2.16.0 #2088

pstibrany · 2020-02-06T15:02:23Z

What this PR does: This PR updates Prometheus dependency to v2.16.0 (b90be6f32a) (was -rc.0 and -rc.1 before). This is required to finish work on issue #2003.

Recent Prometheus changes introduced some incompatibilities:

promql.EngineOpts struct had its MaxConcurrent field removed, and functionality replaced with active query tracker. Cortex' querier therefore enables the tracker to keep -querier.max-concurrent flag still working. Since tracker needs a directory to write files to, Cortex uses ./active-query-tracker by default. Update: If directory is explicitly set to empty string by using -querier.active-query-tracker-dir="", Active Query Tracker is disabled. (Previously, random temp directory was created for empty dir)
Changes in rulefmt package modified RuleNode type, which replaced previously used Rule. Unfortunately, while this provides better error reporting when doing deserialization, it makes it impossible to serialize this structure back to YAML because of problem in yaml.v3 package. Until we find a better solution, I've made a copy of rulefmt package before the change into Cortex (as "legacy_rulefmt"). It's a very tiny package, but admittedly, this is not a good solution.
Prometheus also added SelectSorted method to storage.Querier interface, but fortunately all our queriers already sorted their output in Select method anyway, so all implementations just call that.
promql.ParseErr has lost its Line and Pos fields and now only has PositionRange with start and end location. We use this in /api/prom/validate_expr API endpoint to report query syntax problems. This endpoint doesn't seem to be documented anywhere and was added by request from Grafana Observability team. It doesn't seem to be used as of now. If it's not used anywhere else, it might be possible to remove it completely (it's not provided by Prometheus). For now I've changed endpoint to return start and end position instead of line and col fields. /cc @davkal. Update: this functionality has been completely removed by Remove unused /validate_expr endpoint. #2152.

Checklist

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

jtlisi

LGTM

Just update the logic for the temp directory in the ruler_test so it's called after the test is completed not the newTestRuler function.

pkg/distributor/http_server.go

pkg/ruler/ruler_test.go

pracucci

Good job @pstibrany ! Changes look good to me. I left few comments I would be glad if you could take a look.

pkg/querier/querier.go

CHANGELOG.md

pkg/querier/chunk_store_queryable.go

pkg/querier/ingester_streaming_queryable.go

pkg/querier/lazy_querier.go

pkg/querier/querier.go

pkg/querier/remote_read_test.go

pkg/querier/unified_querier.go

pstibrany · 2020-02-17T16:42:37Z

I've rebased and addressed your feedback. Now instead of creating temp dir for Active Query Tracker, we disable it, if no dir is configured. Dir defaults to ./active-query-tracker.

pracucci

Thanks @pstibrany for addressing my comments. I left a last comment in the code changes to fix the YAML field name for the query tracker.

I went through the vendor/github.com/prometheus changes and a think catched my attention. The diff in vendor/github.com/prometheus/prometheus/web/api/v1/api.go shows that two new fields have been added to the exported rules:

EvaluationTime float64   `json:"evaluationTime"`
LastEvaluation time.Time `json:"lastEvaluation"`

To keep Prometheus compatibility, I think we should add the support for these two new fields to the ruler API endpoints. @jtlisi ?

pkg/querier/querier.go

pracucci

Thanks Peter! Changes LGTM.

I went through the vendor/github.com/prometheus changes and a think catched my attention. The diff in vendor/github.com/prometheus/prometheus/web/api/v1/api.go shows that two new fields have been added to the exported rules.

About this comment, to me we can address it in a separate PR. It doesn't break the current logic, it's just a new "feature".

pracucci

Withdrawing my approval because we found out that the read path is failing with the error missing port in address.

pstibrany · 2020-02-18T15:02:59Z

Withdrawing my approval because we found out that the read path is failing with the error missing port in address.

There is now workaround for this, basically we fake the address in RemoteAddr field. Since we don't set any QueryLogger, this value is unused. (update: I mean parsed value from RemoteAddr)

I've also sent PR to Prometheus to be less strict about missing or invalid RemoteAddr value.

pracucci

Thanks @pstibrany! I'm OK keeping the temporarily fakeRemoteAddr() until the fix won't be upstream.

….0-rc.0) Signed-off-by: Peter Štibraný <[email protected]>

Signed-off-by: Peter Štibraný <[email protected]>

That change is currently incompatible with our serialization code, so until that is fixed, I've copied previous state of rulefmt from Prometheus (github.com/prometheus/prometheus/pkg/rulefmt). It's a small package, and I've included tests as well. Signed-off-by: Peter Štibraný <[email protected]>

Signed-off-by: Peter Štibraný <[email protected]>

Also explains why we use temp dir, if it's not configured. Signed-off-by: Peter Štibraný <[email protected]>

Signed-off-by: Peter Štibraný <[email protected]>

…fe654cc) Signed-off-by: Peter Štibraný <[email protected]>

Signed-off-by: Peter Štibraný <[email protected]>

Disable AQT if no directory is set. Signed-off-by: Peter Štibraný <[email protected]>

Signed-off-by: Peter Štibraný <[email protected]>

Since our Select methods already return sorted results, it makes sense to call them SelectSorted, and let Select simply call SelectSorted, rather then the other way around. Signed-off-by: Peter Štibraný <[email protected]>

Signed-off-by: Peter Štibraný <[email protected]>

…Tracker Signed-off-by: Peter Štibraný <[email protected]>

Signed-off-by: Peter Štibraný <[email protected]>

Prometheus requires non-empty RemoteAddr, but only uses it when Engine actually has QueryLogger set. Which it doesn't, in Cortex. Signed-off-by: Peter Štibraný <[email protected]>

Signed-off-by: Peter Štibraný <[email protected]>

pracucci

Thanks @pstibrany for addressing the rebase! The changes after the rebase LGTM.

CHANGELOG.md

pkg/querier/lazyquery/lazyquery.go

Signed-off-by: Peter Štibraný <[email protected]>

jtlisi mentioned this pull request Feb 6, 2020

ensure RuleGroups can be marshaled into YAML prometheus/prometheus#6778

Closed

jtlisi approved these changes Feb 6, 2020

View reviewed changes

pkg/distributor/http_server.go Outdated Show resolved Hide resolved

pkg/ruler/ruler_test.go Outdated Show resolved Hide resolved

pstibrany changed the title ~~Update prometheus to v2.16.0-rc.0~~ Update prometheus to v2.16.0-rc.1 Feb 12, 2020

pstibrany changed the title ~~Update prometheus to v2.16.0-rc.1~~ Update prometheus to v2.16.0 Feb 17, 2020

pracucci reviewed Feb 17, 2020

View reviewed changes

This was referenced Feb 18, 2020

Don't forget about lack of MaxConcurreny in PromQL the next Prometheus dep update #2045

Closed

Set a max concurrency TSDB blocks compaction #2003

Closed

Remove unused /validate_expr endpoint. #2152

Merged

pracucci reviewed Feb 18, 2020

View reviewed changes

pkg/querier/querier.go Outdated Show resolved Hide resolved

pracucci approved these changes Feb 18, 2020

View reviewed changes

pracucci reviewed Feb 18, 2020

View reviewed changes

pracucci self-requested a review February 18, 2020 11:54

pstibrany mentioned this pull request Feb 18, 2020

Don't return error in ContextFromRequest function. prometheus/prometheus#6840

Merged

pracucci approved these changes Feb 18, 2020

View reviewed changes

pracucci mentioned this pull request Feb 18, 2020

Added query-frontend integration test #2154

Merged

3 tasks

pstibrany added 13 commits February 21, 2020 07:49

Updated Prometheus to 22a04239c937be61df95fdb60f0661684693cf3b (v2.16…

0d10dce

….0-rc.0) Signed-off-by: Peter Štibraný <[email protected]>

Added SelectSorted method.

46cc862

Signed-off-by: Peter Štibraný <[email protected]>

Use activity tracker to limit concurrency.

e86da92

Signed-off-by: Peter Štibraný <[email protected]>

Added SelectSorted method.

8c87686

Signed-off-by: Peter Štibraný <[email protected]>

Use ActiveQueryTracker for limiting concurrent queries.

42daf51

Signed-off-by: Peter Štibraný <[email protected]>

Pass logger to activity tracker. Log directory for debugging.

1018d33

Signed-off-by: Peter Štibraný <[email protected]>

Renamed legacy_rulefmt package back to rulefmt.

c3ee264

Signed-off-by: Peter Štibraný <[email protected]>

Use legacy_rulefmt import name.

63c2f46

Signed-off-by: Peter Štibraný <[email protected]>

Clean tempdir after test has completed.

c556caf

Signed-off-by: Peter Štibraný <[email protected]>

Added -querier.active-query-tracker-dir flag

a3339b1

Signed-off-by: Peter Štibraný <[email protected]>

Added CHANGELOG.md about new querier.active-query-tracker-dir flag

ebf6cb2

Also explains why we use temp dir, if it's not configured. Signed-off-by: Peter Štibraný <[email protected]>

make doc

895cfe3

Signed-off-by: Peter Štibraný <[email protected]>

pstibrany added 15 commits February 21, 2020 07:54

Updated Prometheus to v2.16.0-rc.1 (56eaaadb54560d124310dcd779b81a4c5…

37ad209

…fe654cc) Signed-off-by: Peter Štibraný <[email protected]>

Updated Prometheus to 2.16.0 (b90be6f32a33c03163d700e1452b54454ddce0ec)

ebdfbc0

Signed-off-by: Peter Štibraný <[email protected]>

Active query tracker documentation.

e5a9bad

Disable AQT if no directory is set. Signed-off-by: Peter Štibraný <[email protected]>

Updated CHANGELOG.md

fd9a1d8

Signed-off-by: Peter Štibraný <[email protected]>

Swap Select and SelectSorted.

f2e5731

Since our Select methods already return sorted results, it makes sense to call them SelectSorted, and let Select simply call SelectSorted, rather then the other way around. Signed-off-by: Peter Štibraný <[email protected]>

Updated docs

003e50b

Signed-off-by: Peter Štibraný <[email protected]>

No need to log directory used by active query tracker anymore.

aa02273

Signed-off-by: Peter Štibraný <[email protected]>

Ignore active-query-tracker, it is default dir name for Active Query …

7103bbf

…Tracker Signed-off-by: Peter Štibraný <[email protected]>

Fixed active_query_tracker_dir yaml field name to match CLI option.

97c82b9

Signed-off-by: Peter Štibraný <[email protected]>

Fake RemoteAddr if it is empty.

5a002b5

Prometheus requires non-empty RemoteAddr, but only uses it when Engine actually has QueryLogger set. Which it doesn't, in Cortex. Signed-off-by: Peter Štibraný <[email protected]>

Add SelectSorted method to blocksQuerier.

df60102

Signed-off-by: Peter Štibraný <[email protected]>

Added comment about possible removal in the future.

0189028

Signed-off-by: Peter Štibraný <[email protected]>

Fix compilation and test issues after master rebase.

cd95df8

Signed-off-by: Peter Štibraný <[email protected]>

Go mod tidy

0a8d5f5

Signed-off-by: Peter Štibraný <[email protected]>

Lint

d6bc0fd

Signed-off-by: Peter Štibraný <[email protected]>

pracucci approved these changes Feb 21, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

pkg/querier/lazyquery/lazyquery.go Outdated Show resolved Hide resolved

Review feedback. Also fixed impls -> implements.

45ca8fa

Signed-off-by: Peter Štibraný <[email protected]>

pracucci merged commit fbe1c63 into cortexproject:master Feb 21, 2020

Update prometheus to v2.16.0 #2088

Update prometheus to v2.16.0 #2088

Uh oh!

Conversation

pstibrany commented Feb 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtlisi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pstibrany commented Feb 17, 2020

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

pstibrany commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pstibrany commented Feb 6, 2020 •

edited

Loading

pstibrany commented Feb 18, 2020 •

edited

Loading