Port Cortex to use Prometheus 2 packages #583

juliusv · 2017-10-17T05:12:25Z

This is the main part of #555

As promised, this is a crazy mega change. Unfortunately, when swapping out vendored packages and making everything work again, a smaller change wasn't really possible.

Here's a high-level overview:

We change the vendoring of most Prometheus packages (API, rules, promql, storage interfaces, etc.) to Prometheus 2.0, which is very different in many regards. The using packages in Cortex are adjusted to work with the new interfaces of Prom 2.
This change doesn't actually change the internal storage of Cortex, and it continues to use Prom 1's chunk format. Because it is impossible to vendor packages both from Prom 1 and Prom 2, I hard-forked (copied) the necessary storage (chunk+metric) packages from Prometheus into pkg/prom1/... and rewrote all the imports for them.
Alertmanager vendoring is also updated to the latest state, to be able to work properly with Prom 2's common dependencies (it's not possible to vendor multiple versions of the same repo).
The latest prometheus/common/log does not automatically configure log level flags anymore, so there's extra code to do that in Cortex now.
The Go builder image is updated from 1.8.3 to 1.9.1 now because I think I encountered some problems with the old one, but now I'm not sure anymore.
The linter needs new exceptions with Go 1.9.1's vet.
Prom 2's querier Select() method is used for both metadata and sample querying, but a remote implementation would want to know beforehand whether it needs to get full sample data or only metadata. That's why I allow parametrizing the Queryable (which produces a Querier) to tell it whether it should produce full sample or only metadata queriers, and then hand in the right kind of Queryable to the using packages.
The prometheus/common/route package doesn't have a context function argument in its constructor anymore, but we also didn't need that anymore, so that dummy function argument is just removed.
The multi-tentant Alertmanager needed only small adjustments to work with the updated upstream AM packages.
I updated golang.org/x/net/context to native context in many places (was required for some things), but not everywhere yet.
The LazySeriesIterators were never actually used lazily, and would have made Prom 2 usage much harder, so I removed them in favor of eagerly evaluating even queries that fuzzy-match the metric name.
Underlying Get() and Query() functions now return model.Matrix instead of iterators, since the above-mentioned eager eval obviated the need for iterators again and this made things simpler.
I converted many label and metric types to Prom 2 types (mostly, where necessary), but not all of them yet (didn't want to make the change even bigger than necessary). I changed helper functions accordingly.
I copied and adapted the private concreteSeries / concreteSeriesSet / errorSeriesSet implementations from Prometheus's remote storage code to be able to return series from Select() that are backed by already present sample data (or errors).
The adapation of the new appender interface in pkg/ruler/compat.go fixes Don't create one ingester write request per rule-generated sample #569 as a side-effect by batching up all samples from a given rule evaluation and only sending them upon Commit() (called at the end of a rule evaluation).
Extreme dep dependency vendoring wrangling was necessary to make all the different package versions and conflicting dependencies work. Hopefully, everything fits well enough together now (at least it compiles!).

User-visible changes due to this PR:

All PromQL 2.0 functionality (including staleness handling) should be available.
The latest Prom 2.0 /api/v1 functionality should be available (there weren't many (any?)) changes. Note that /api/v2 isn't available yet, but only contains some extra endpoints which we don't want to implement anyway so far.
Any new Alertmanager features should be available now via the AM config (and embedded UI updates).
NOTE: The rule format for alerting / recording rules is still Prometheus 1.0 format! This is because Prometheus 2.0 packages still have functionality for parsing the old format, and we are still using this. (This functionality is still in Prom 2.0 to enable promtool to do conversions). This is great, as it allows us to keep at least one major user-visible change out of this PR, and it is easy to change the code to expect the Prom 2.0 rule format in a subsequent change.

How to review this change? Let's be honest: you probably can't :) Instead, help test it! All the pieces should be roughly there, but I expect bugs. I tested basic ingestion/querying/alerting rules/AM UI, but no edge cases or other very thorough manual testing yet.

Please help test everything you can think of in this current state and let me know what is still broken! It would be good to get this merged after we are reasonably confident that it is working, and do other code cleanups and features in later changes, since probably a lot of other stuff is blocked on this one large code change.

juliusv · 2017-10-17T05:50:48Z

(will take a look at the remaining test failures)

juliusv · 2017-10-17T06:05:29Z

Hmm, tests were "just" flaky, not sure if related to my changes or not.

rade · 2017-10-17T06:25:24Z

Did you forget to dep prune? When I run dep ensure && dep prune about 13k files get removed.

juliusv · 2017-10-17T06:36:05Z

@rade Indeed, thanks. Still new to dep. Fixed. I squashed the pruning into the existing vendoring commit to not pollute the git history with the billions of extra files forever.

rade · 2017-10-17T06:55:37Z

Fixed

Actually, according to the docs one is meant to run make update-vendor. This does the dep ensure && dep prune but also, crucially, retains the BUILD.bazel files.

Apologies for the mis-guidance; I haven't built cortex for a while.

rade · 2017-10-17T07:07:03Z

one is meant to run make update-vendor ... retains the BUILD.bazel files.

...though it looks like that would also retain the BUILD.bazel files of removed dependencies, which doesn't seem right.

jml

Thanks! I'll get to testing this.

Comments are mostly questions to help me understand than suggestions for changing things.

jml · 2017-10-17T10:06:39Z

cmd/querier/main.go

+		engine,
+		metadataQueryable,
+		querier.DummyTargetRetriever{},
+		querier.DummyAlertmanagerRetriever{},


What does it mean to give the API dummy retrievers?

The normal Prometheus API can provide a list of targets (e.g. http://demo.robustperception.io:9090/api/v1/targets) and configured Alertmanagers (http://demo.robustperception.io:9090/api/v1/alertmanagers). We don't need/have this information for Cortex, so we pass in dummy implementations that just give empty lists.

jml · 2017-10-17T10:08:20Z

pkg/chunk/chunk_store.go

-		orgID, err := user.ExtractOrgID(ctx)
-		if err != nil {
-			return nil, err
+		// TODO(prom2): Does this contraption over-match?


How would we test this?

This is something I still want to look at closer before merging. Mainly I extracted this code from the lazy iterators and moved it to here (for eager loading). I just need to reason through it all one more time before giving a better answer :)

So to elaborate:

The question I have around this code is not related to this PR really, as I just moved it here to be eager-loading instead of lazy-loading. The same question applies to the current master.

Somehow it seems to actually select only the correct series, although if I run it in my head there should be a bug: the code gets all series where the metric name matcher applies, then filters them down to only the series where the other matchers also apply. From those matching series, it builds a set of new matchers that are then used to look up the final series chunks. But those matchers should then match too many series in some cases.

So e.g. I thought this would over-match in the following scenario:

TSDB has both foo{a="b"} and foo{a="b", c="d"}

Query is for {__name__=~"foo", c!="d"}

(correctly this should return foo{a="b"}, but not foo{a="b", c="d"})

What I would expect to happen in the code is then:

it finds and matches both foo series in the metric name index

it filters out the foo{a="b", c="d"} series because it doesn't match the c!="d" matcher

it builds new matchers out of the remaining series's labels (foo{a="b"})

it uses those new matchers to look up all series that match them, which should also match foo{a="b", c="d"}, since it is a superset of labels

But somehow, it does not return this wrong series and the query result is correct. But why? Do you see what I'm missing?

So of course I couldn't just sleep with this mystery unsolved. Many debug Printf's later, I found out the following:

The returned series were only correct because they came from the ingesters only, and not from the chunk store.

The chunk store actually didn't select any data for the above sample scenario because of a bug I introduced (fixed in latest push) that passed in __name__ instead of the metric name into getMetricNameMatrix(). Now that that's fixed, it does select too many series, as outlined in my example scenario.

The current master is actually worse: rather than selecting too many series, the lazy iterator just crashes with a nil pointer dereference in the above scenario, as soon as there are relevant chunks in S3. This is currently happening in production as well.

So: master is currently broken in a crashy way (queries fail completely), whereas this PR branch is broken in that it selects too many series in an edge case. That problem would also occur in master, if it didn't crash before that. Thus, I am almost tending to not try and fix this unrelated bug in this PR. @jml What do you think?

jml · 2017-10-17T10:09:27Z

pkg/chunk/chunk_store_test.go

 }

-// TestChunkStore_Get tests iterators are returned correctly depending on the type of query
-func TestChunkStore_Get_concrete(t *testing.T) {
+// TODO(prom2): reintroduce tests that were part of TestChunkStore_Get_lazy


Given that our next step is testing, it would be good to actually reintroduce these.

jml · 2017-10-17T10:11:08Z

pkg/configs/client/configs.go

@@ -8,6 +8,7 @@ import (
 	"net/url"
 	"time"

+	gklog "github.com/go-kit/kit/log"


Why use go-kit's logger?

Prometheus 2.0 packages use go-kit's logger interfaces everywhere now, so we need to interface with that. See this in the same file below:

// TODO(prom2): Wrap our logger into a gokit logger. rule = rules.NewAlertingRule(r.Name, r.Expr, r.Duration, r.Labels, r.Annotations, gklog.NewNopLogger())

jml · 2017-10-17T10:17:50Z

pkg/querier/series_set.go

+	return e.err
+}
+
+type metadataSeriesSet struct {


I don't understand what this is for.

Whoops, I forgot to remove that type. Removed.

juliusv · 2017-10-17T19:15:42Z

@rade Hm yeah,

update-vendor:
        dep ensure && dep prune
        git status | grep BUILD.bazel | cut -d' ' -f 5 | xargs git checkout HEAD

I guess preserving those BUILD.bazel files would only work now if they hadn't already been removed in previous commits. And that also means we have to manually write Bazel files for new dependencies? Or is there an automation for that? /cc @tomwilkie

juliusv · 2017-10-17T22:29:28Z

Ok, so I re-added the Bazel build files by running make gazelle and then copying some of the problematic/missing build files from master (see comment in Makefile about the gazelle target):

make gazelle
# Get 
git checkout master -- vendor/github.com/openzipkin/zipkin-go-opentracing/_thrift/gen-go/zipkincore/BUILD.bazel
git checkout master -- vendor/github.com/openzipkin/zipkin-go-opentracing/_thrift/gen-go/scribe/BUILD.bazel
git checkout master -- vendor/golang.org/x/crypto/curve25519/BUILD.bazel

It builds with Bazel now :)

juliusv · 2017-10-19T01:48:29Z

@jml I finished all remaining TODO(prom2) items now, so I'm removing the [WIP]. Although it would still be good if you could also do some more manual testing of everything.

Most importantly, the over-selection edge case in the chunk store is now fixed, and there are tests to cover that and other cases. The new test cases also uncovered another bug that is fixed now. I also simplified the test scenario code a bit.

I think this is ready for final testing and review.

jml · 2017-10-19T10:27:55Z

@juliusv Thanks! I'm not going to get a chance to do any testing today (and am away tomorrow & all week next week). Happily, @leth has volunteered to run this on dev.

jml · 2017-10-19T10:32:33Z

New commits LGTM. Thanks especially for the tests!

juliusv · 2017-10-29T09:37:20Z

Besides testing the alert notifications end-to-end one more time in dev, does anyone see any other things that we should ensure before merging? I at least have not found more issues so far, and beginning of the week would generally be good timing for deploying to prod and then dealing with any potential fallout.

jml · 2017-10-30T13:45:26Z

Just chatted w/ Marcus about this in person. - end-to-end alert notifications still need to be tested - need to clarify migration plan for rules/alert configs We want to have an alert correctness job, but we shouldn't block the prom2 merge/prod-deploy on that.

…

On Sun, 29 Oct 2017 at 09:37 Julius Volz ***@***.***> wrote: Besides testing the alert notifications end-to-end one more time in dev, does anyone see any other things that we should ensure before merging? I at least have not found more issues so far, and beginning of the week would generally be good timing for deploying to prod and then dealing with any potential fallout. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#583 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHq6uJ2cXGo3J2hIqFYOHLjgy89TIyRks5sxEdQgaJpZM4P7lu6> .

jml · 2017-10-30T13:45:36Z

@leth – tagging re above message.

…

On Mon, 30 Oct 2017 at 13:45 Jonathan Lange ***@***.***> wrote: Just chatted w/ Marcus about this in person. - end-to-end alert notifications still need to be tested - need to clarify migration plan for rules/alert configs We want to have an alert correctness job, but we shouldn't block the prom2 merge/prod-deploy on that. On Sun, 29 Oct 2017 at 09:37 Julius Volz ***@***.***> wrote: > Besides testing the alert notifications end-to-end one more time in dev, > does anyone see any other things that we should ensure before merging? I at > least have not found more issues so far, and beginning of the week would > generally be good timing for deploying to prod and then dealing with any > potential fallout. > > — > You are receiving this because you were mentioned. > > > Reply to this email directly, view it on GitHub > <#583 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAHq6uJ2cXGo3J2hIqFYOHLjgy89TIyRks5sxEdQgaJpZM4P7lu6> > . >

juliusv · 2017-10-30T13:49:52Z

Note that this PR doesn't change the rule format yet, so there's no migration needed for this yet. I'll do the (still manual) alert testing tonight.

…

On Oct 30, 2017 2:45 PM, "Jonathan Lange" ***@***.***> wrote: Just chatted w/ Marcus about this in person. - end-to-end alert notifications still need to be tested - need to clarify migration plan for rules/alert configs We want to have an alert correctness job, but we shouldn't block the prom2 merge/prod-deploy on that. On Sun, 29 Oct 2017 at 09:37 Julius Volz ***@***.***> wrote: > Besides testing the alert notifications end-to-end one more time in dev, > does anyone see any other things that we should ensure before merging? I at > least have not found more issues so far, and beginning of the week would > generally be good timing for deploying to prod and then dealing with any > potential fallout. > > — > You are receiving this because you were mentioned. > > > Reply to this email directly, view it on GitHub > <#583 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ AAHq6uJ2cXGo3J2hIqFYOHLjgy89TIyRks5sxEdQgaJpZM4P7lu6> > . > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#583 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAg1mEl-d_MUMDy-y8JaoSqUNUuCNkkpks5sxdL3gaJpZM4P7lu6> .

jml

Our conviction is like an arrow already in flight.

The updated `go vet` complains about more errors than the old one.

The upstream prometheus/common/log changed and doesn't automatically register flags anymore.

The laziness was actually never used.

Only the v8 schema supports omitting the metric name in a query.

- Fix over-selection bug when the metric name was non-equals-matched and one selects a series that has a subset of labels of another. - Regex matches against the metric name were broken because the metric name was equals-compared to the matcher regex instead of applying it as a regex. - Unneeded sorting of label matchers has been removed.

The ingester only needs the MaxChunkAge from the SchemaConfig, and the ingester.Config is actually a better place to store and configure that information authoritatively.

The deep copy that dumped it as JSON and loaded it again stumbled over the fact that the JSON marshaling renders Secret fields as <secret> and thus loses the original secret field contents when reloading it from JSON.

bboreham · 2018-02-08T12:37:13Z

Remove SchemaConfig dependency from ingester
The ingester only needs the MaxChunkAge from the SchemaConfig, and the ingester.Config is actually a better place to store and configure that information authoritatively.

Unfortunately this caused table-manager to see a zero value for MaxChunkAge, which causes #621

juliusv requested a review from jml October 17, 2017 05:12

juliusv force-pushed the prom2-port branch from 6cb9fc9 to 4041df2 Compare October 17, 2017 06:36

jml reviewed Oct 17, 2017

View reviewed changes

juliusv force-pushed the prom2-port branch 4 times, most recently from 473008a to 44a2337 Compare October 19, 2017 01:44

juliusv changed the title ~~[WIP] Port Cortex to use Prometheus 2 packages~~ Port Cortex to use Prometheus 2 packages Oct 19, 2017

leth mentioned this pull request Oct 20, 2017

Querier panic with {job="fluxy/prose",__name__!=""} #582

Closed

juliusv force-pushed the prom2-port branch from cdb24bd to 8775a06 Compare October 27, 2017 10:13

juliusv force-pushed the prom2-port branch 2 times, most recently from 73a82ba to 95f4254 Compare November 6, 2017 15:30

jml approved these changes Nov 13, 2017

View reviewed changes

juliusv added 2 commits November 13, 2017 16:35

Vendor update for Prometheus 2 port

e45e5c0

Hard-fork still-required Prometheus 1 storage packages

930984d

juliusv and others added 24 commits November 13, 2017 16:35

Update build image to latest Go version

3dc7a0e

Add lint/vat exceptions

88e527e

The updated `go vet` complains about more errors than the old one.

Re-add flag for configuring log level ourselves

31d7fd6

The upstream prometheus/common/log changed and doesn't automatically register flags anymore.

Adjust multitenant Alertmanager to new upstream packages

7480e9f

Implement new querier wrappers for Prom 2 packages

9c2ddcf

Other adjustments to new packages and types

d9c3e43

Remove now-obsolete MergeSeriesIterator

7540b39

Remove now-obsolete LazySeriesIterator

9caa627

The laziness was actually never used.

Wire up new API, querier, etc. in main functions

faa3813

Update/fix go test -tags separation syntax

1e3be18

Remove unused metadataSeriesSet type

8a99bda

Re-add Bazel build files

0d5741a

Adjust comment about (now-removed) lazy iterators

1c1245d

Pass correct metric name into getMetricNameMatrix()

419de9c

Pass in go-kit logger to alerting rule creation

f05630e

Fix ErrNoMetricNameNotSupported message

331a690

Only the v8 schema supports omitting the metric name in a query.

Add back -log.level flag to ingester as well

9053e8e

Ensure sorting of series labels during creation

e65e334

Add test for querier label sorting

1b83f3b

Remove SchemaConfig dependency from ingester

d6417ae

The ingester only needs the MaxChunkAge from the SchemaConfig, and the ingester.Config is actually a better place to store and configure that information authoritatively.

Resolve build problems after master rebase

c6b3551

Reparse AM fallback config every time so we can mutate it

1720b6c

The deep copy that dumped it as JSON and loaded it again stumbled over the fact that the JSON marshaling renders Secret fields as <secret> and thus loses the original secret field contents when reloading it from JSON.

Fix flaky chunk test

8f14044

juliusv force-pushed the prom2-port branch from 730db93 to 8f14044 Compare November 13, 2017 15:47

juliusv merged commit d714b62 into master Nov 13, 2017

juliusv deleted the prom2-port branch November 13, 2017 16:10

bboreham mentioned this pull request Feb 8, 2018

Move SchemaConfig.MaxChunkAge to NewTableManager() #693

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port Cortex to use Prometheus 2 packages #583

Port Cortex to use Prometheus 2 packages #583

juliusv commented Oct 17, 2017 •

edited

Loading

juliusv commented Oct 17, 2017

juliusv commented Oct 17, 2017

rade commented Oct 17, 2017 •

edited

Loading

juliusv commented Oct 17, 2017

rade commented Oct 17, 2017

rade commented Oct 17, 2017

jml left a comment

jml Oct 17, 2017

juliusv Oct 17, 2017

jml Oct 17, 2017

juliusv Oct 17, 2017

juliusv Oct 18, 2017 •

edited

Loading

juliusv Oct 18, 2017 •

edited

Loading

jml Oct 17, 2017

juliusv Oct 17, 2017

jml Oct 17, 2017

juliusv Oct 17, 2017

jml Oct 17, 2017

juliusv Oct 17, 2017

juliusv commented Oct 17, 2017

juliusv commented Oct 17, 2017

juliusv commented Oct 19, 2017

jml commented Oct 19, 2017

jml commented Oct 19, 2017

juliusv commented Oct 29, 2017

jml commented Oct 30, 2017 via email

jml commented Oct 30, 2017 via email

juliusv commented Oct 30, 2017 via email

jml left a comment

bboreham commented Feb 8, 2018

Port Cortex to use Prometheus 2 packages #583

Port Cortex to use Prometheus 2 packages #583

Conversation

juliusv commented Oct 17, 2017 • edited Loading

juliusv commented Oct 17, 2017

juliusv commented Oct 17, 2017

rade commented Oct 17, 2017 • edited Loading

juliusv commented Oct 17, 2017

rade commented Oct 17, 2017

rade commented Oct 17, 2017

jml left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliusv Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

juliusv Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliusv commented Oct 17, 2017

juliusv commented Oct 17, 2017

juliusv commented Oct 19, 2017

jml commented Oct 19, 2017

jml commented Oct 19, 2017

juliusv commented Oct 29, 2017

jml commented Oct 30, 2017 via email

jml commented Oct 30, 2017 via email

juliusv commented Oct 30, 2017 via email

jml left a comment

Choose a reason for hiding this comment

bboreham commented Feb 8, 2018

juliusv commented Oct 17, 2017 •

edited

Loading

rade commented Oct 17, 2017 •

edited

Loading

juliusv Oct 18, 2017 •

edited

Loading

juliusv Oct 18, 2017 •

edited

Loading