cmd/thanos/query: add initial rules support #2240

s-urbaniak · 2020-03-09T18:22:08Z

This adds basic support for the rule parameter into the Thanos Query component. Marking as WIP as I'd like to have feedback about whether the trajectory actually is fit.

Bear with me, I am new here :-)

cc @bwplotka

cmd/thanos/query.go

bwplotka

Nice!

Some thoughts, but generally looks amazing. What about changing Rules API and add some Info endpoint?

cmd/thanos/query.go

pkg/query/storeset.go

pkg/query/storeset_test.go

pkg/store/proxy_test.go

pkg/store/prometheus.go

cmd/thanos/query.go

bwplotka · 2020-04-22T14:11:28Z

cmd/thanos/query.go

@@ -76,6 +76,9 @@ func registerQuery(m map[string]setupFunc, app *kingpin.Application) {
 	stores := cmd.Flag("store", "Addresses of statically configured store API servers (repeatable). The scheme may be prefixed with 'dns+' or 'dnssrv+' to detect store API servers through respective DNS lookups.").
 		PlaceHolder("<store>").Strings()

+	rules := cmd.Flag("rule", "Addresses of statically configured rules API servers (repeatable). The scheme may be prefixed with 'dns+' or 'dnssrv+' to detect store API servers through respective DNS lookups.").


We might want to be clear that Rules API has to implement Info as well to work

cmd/thanos/query.go

pkg/query/api/v1.go

pkg/query/storeset.go

cmd/thanos/query.go

docs/components/query.md

pkg/query/storeset.go

pkg/store/proxy.go

Signed-off-by: Sergiusz Urbaniak <sergiusz.urbaniak@gmail.com>

bwplotka

Looks good for now!

Let's merge. 🎉

Just note to @povilasv and @GiedriusS this merge to release branch for now (: Just to not get confused.

GiedriusS

I found some problems with the proxying stuff :P I understand that you are only pushing into a feature branch but I think it's worth fixing these problems early. Awesome work! Can't wait to use this when it will be fully finished.

pkg/store/proxy.go

pkg/query/api/v1.go

povilasv · 2020-04-29T06:24:05Z

pkg/query/rules.go

+	if err := rr.proxy.Rules(&storepb.RulesRequest{
+		PartialResponseStrategy: storepb.PartialResponseStrategy_ABORT,
+	}, resp); err != nil {
+		return nil, nil, errors.Wrap(err, "proxy RuleGroups()")


consider not captilazing error strings https://github.com/golang/go/wiki/CodeReviewComments#error-strings

hmm ... I wanted to stay consistent with

thanos/pkg/query/querier.go

Line 208 in 4240e3c

return nil, nil, errors.Wrap(err, "proxy Series()")

wdyt?

povilasv · 2020-04-29T07:14:31Z

pkg/query/rules.go

+	resp := &rulesServer{ctx: ctx}
+
+	if err := rr.proxy.Rules(&storepb.RulesRequest{
+		PartialResponseStrategy: storepb.PartialResponseStrategy_ABORT,


This should be configureable, no?

yes 👍 I was just impatient and wanted to have something working :)

plus (note to myself): once I have also configurability injected here, this is where I want to implement the deduplication, and of course it needs unit tests here too.

s-urbaniak · 2020-04-29T11:26:19Z

@GiedriusS @povilasv please bear with me, it is still WIP :-) I'll ping once I remove the tag, thanks for the early feedback!!! ❤️

Signed-off-by: Sergiusz Urbaniak <sergiusz.urbaniak@gmail.com>

s-urbaniak · 2020-04-29T12:04:33Z

@GiedriusS @povilasv @bwplotka I added an initial (very simple) e2e test, spinning up a Thanos Querier + Sidecar (so far the two components implementing the API). So far no deduplication logic yet, bear with me :-)

Also ... a general neutral statement from someone contributing the first time: I really really enjoy the code base and the great e2e testing framework capabilities here, so much goodness here and easy to start locally ❤️

Signed-off-by: Sergiusz Urbaniak <sergiusz.urbaniak@gmail.com>

s-urbaniak · 2020-05-04T08:07:22Z

@GiedriusS @povilasv @bwplotka Do you believe we could merge this into the feature branch? It would make logistics easier for me to submit additional PRs for the actual deduplication which I am working on right now. Thank you! :-)

bwplotka · 2020-05-04T09:13:13Z

Let's go! 💪

We can address potential comments in next PRs!

bwplotka · 2020-05-04T09:14:11Z

AFAIK all comments by @povilasv and @GiedriusS were fixed.

* cmd/thanos/query: add initial rules support Signed-off-by: Sergiusz Urbaniak <sergiusz.urbaniak@gmail.com> * pkg/query/api/v1: initial implementation Signed-off-by: Sergiusz Urbaniak <sergiusz.urbaniak@gmail.com> * e2e: initial implementation and fixes Signed-off-by: Sergiusz Urbaniak <sergiusz.urbaniak@gmail.com> * pkg/query: fix racy access to assert rules API store Signed-off-by: Sergiusz Urbaniak <sergiusz.urbaniak@gmail.com>

) * Added RulesAPI. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> * Added warnings. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> * Added Type to rules requests as it is on HTTP API. (#2201) Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> * pkg/store/storepb: fix wrong rule reference (#2237) * pkg/store/storepb: fix wrong rule reference Currently we recursively reference rules instead of recording rules. Signed-off-by: Sergiusz Urbaniak <sergiusz.urbaniak@gmail.com> * proto: regenerate Signed-off-by: Sergiusz Urbaniak <sergiusz.urbaniak@gmail.com> * Made storepb.RuleGroups a source of truth for rules API (Go, JSON, proto). Added tests. (#2242) Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> * Use proto rules API instead of struct; Moved as much as possible to promclient; Added rulesAPI RPC to sidecar. (#2243) * Use proto rules API instead of struct; Added rulesAPI RPC to sidecar. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> * Fixed broken test. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> * Use proto rules API instead of struct; Moved as much as possible to promclient; Added rulesAPI RPC to sidecar. (#2291) * rules_custom_test: fix asserting labels Signed-off-by: Sergiusz Urbaniak <sergiusz.urbaniak@gmail.com> * TestPrometheusStore_Rules_e2e: fix test fixture Signed-off-by: Sergiusz Urbaniak <sergiusz.urbaniak@gmail.com> Co-authored-by: Sergiusz Urbaniak <sergiusz.urbaniak@gmail.com> * cmd/thanos/query: add initial rules support (#2240) * cmd/thanos/query: add initial rules support Signed-off-by: Sergiusz Urbaniak <sergiusz.urbaniak@gmail.com> * pkg/query/api/v1: initial implementation Signed-off-by: Sergiusz Urbaniak <sergiusz.urbaniak@gmail.com> * e2e: initial implementation and fixes Signed-off-by: Sergiusz Urbaniak <sergiusz.urbaniak@gmail.com> * pkg/query: fix racy access to assert rules API store Signed-off-by: Sergiusz Urbaniak <sergiusz.urbaniak@gmail.com> * Refactored proto generation and separated store from rules APIs. (#2558) * Refactored proto generation and separate store from rules APIs. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> * Addressed comments. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> * Fixed proto gen. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> * Addressed Serg comments. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> * Added Ruler support for RulesAPI; Refactored Manager. (#2562) As per: https://thanos.io/proposals/202003_thanos_rules_federation.md/ Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> * Small fixes to changelog and flags. Do not add any. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> * Fixed after rebase. Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com> Co-authored-by: Sergiusz Urbaniak <sergiusz.urbaniak@gmail.com>

s-urbaniak changed the title ~~cmd/thanos/query: add initial rules support~~ [WIP] cmd/thanos/query: add initial rules support Mar 9, 2020

s-urbaniak force-pushed the rule branch 5 times, most recently from 0fe7863 to 2a7ef1b Compare March 9, 2020 18:49

bwplotka reviewed Mar 10, 2020

View reviewed changes

cmd/thanos/query.go Outdated Show resolved Hide resolved

bwplotka reviewed Mar 10, 2020

View reviewed changes

s-urbaniak force-pushed the rule branch 2 times, most recently from e701735 to ba2911a Compare March 19, 2020 12:38

s-urbaniak force-pushed the rule branch 3 times, most recently from d936dcc to 07a96e2 Compare March 30, 2020 13:22

bwplotka reviewed Apr 2, 2020

View reviewed changes

pkg/store/prometheus.go Outdated Show resolved Hide resolved

s-urbaniak force-pushed the rule branch 7 times, most recently from 6bb7d5d to 57ea1bf Compare April 7, 2020 15:06

s-urbaniak force-pushed the rule branch 5 times, most recently from 63f2a14 to 2925c8c Compare April 21, 2020 12:57

bwplotka changed the title ~~[WIP] cmd/thanos/query: add initial rules support~~ cmd/thanos/query: add initial rules support Apr 22, 2020

bwplotka reviewed Apr 22, 2020

View reviewed changes

simonpasquier reviewed Apr 22, 2020

View reviewed changes

cmd/thanos/query.go Outdated Show resolved Hide resolved

pkg/query/api/v1.go Outdated Show resolved Hide resolved

pkg/query/storeset.go Show resolved Hide resolved

povilasv reviewed Apr 23, 2020

View reviewed changes

s-urbaniak force-pushed the rule branch 4 times, most recently from 6de6e54 to e8bc4de Compare April 27, 2020 14:07

s-urbaniak added 2 commits April 28, 2020 08:34

cmd/thanos/query: add initial rules support

c8b5502

Signed-off-by: Sergiusz Urbaniak <sergiusz.urbaniak@gmail.com>

pkg/query/api/v1: initial implementation

94ce497

Signed-off-by: Sergiusz Urbaniak <sergiusz.urbaniak@gmail.com>

s-urbaniak force-pushed the rule branch from e8bc4de to 94ce497 Compare April 28, 2020 06:34

bwplotka approved these changes Apr 28, 2020

View reviewed changes

GiedriusS requested changes Apr 28, 2020

View reviewed changes

pkg/store/proxy.go Show resolved Hide resolved

pkg/store/proxy.go Outdated Show resolved Hide resolved

povilasv reviewed Apr 29, 2020

View reviewed changes

pkg/query/api/v1.go Show resolved Hide resolved

povilasv reviewed Apr 29, 2020

View reviewed changes

s-urbaniak force-pushed the rule branch from 48d8996 to ca4da5d Compare April 29, 2020 11:54

e2e: initial implementation and fixes

c92ed81

Signed-off-by: Sergiusz Urbaniak <sergiusz.urbaniak@gmail.com>

s-urbaniak force-pushed the rule branch from ca4da5d to c92ed81 Compare April 29, 2020 12:01

pkg/query: fix racy access to assert rules API store

1d5663e

Signed-off-by: Sergiusz Urbaniak <sergiusz.urbaniak@gmail.com>

bwplotka merged commit 705d7cf into thanos-io:features/rules-proxy May 4, 2020

s-urbaniak deleted the rule branch May 4, 2020 09:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/thanos/query: add initial rules support #2240

cmd/thanos/query: add initial rules support #2240

s-urbaniak commented Mar 9, 2020 •

edited

Loading

bwplotka left a comment

bwplotka Apr 22, 2020

bwplotka left a comment

GiedriusS left a comment

povilasv Apr 29, 2020

s-urbaniak Apr 29, 2020

povilasv Apr 29, 2020

s-urbaniak Apr 29, 2020

s-urbaniak Apr 29, 2020

s-urbaniak commented Apr 29, 2020

s-urbaniak commented Apr 29, 2020

s-urbaniak commented May 4, 2020

bwplotka commented May 4, 2020

bwplotka commented May 4, 2020

cmd/thanos/query: add initial rules support #2240

cmd/thanos/query: add initial rules support #2240

Conversation

s-urbaniak commented Mar 9, 2020 • edited Loading

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka Apr 22, 2020

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

GiedriusS left a comment

Choose a reason for hiding this comment

povilasv Apr 29, 2020

Choose a reason for hiding this comment

s-urbaniak Apr 29, 2020

Choose a reason for hiding this comment

povilasv Apr 29, 2020

Choose a reason for hiding this comment

s-urbaniak Apr 29, 2020

Choose a reason for hiding this comment

s-urbaniak Apr 29, 2020

Choose a reason for hiding this comment

s-urbaniak commented Apr 29, 2020

s-urbaniak commented Apr 29, 2020

s-urbaniak commented May 4, 2020

bwplotka commented May 4, 2020

bwplotka commented May 4, 2020

s-urbaniak commented Mar 9, 2020 •

edited

Loading