Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added Ruler support for RulesAPI; Refactored Manager. #2562

Merged
merged 1 commit into from
May 21, 2020
Merged

Added Ruler support for RulesAPI; Refactored Manager. #2562

merged 1 commit into from
May 21, 2020

Conversation

bwplotka
Copy link
Member

@bwplotka bwplotka commented May 5, 2020

s-urbaniak
s-urbaniak reviewed May 5, 2020
View reviewed changes
cmd/thanos/rule.go Outdated Show resolved Hide resolved
@s-urbaniak
Copy link
Contributor

looking great so far!

@bwplotka bwplotka force-pushed the ruler-rulesAPI branch 2 times, most recently from 6c1ba3e to f891f6a Compare May 6, 2020 17:30
povilasv
povilasv reviewed May 7, 2020
View reviewed changes
pkg/rules/api/v1_test.go Outdated Show resolved Hide resolved
pkg/rules/manager.go Show resolved Hide resolved
pkg/rules/manager.go Outdated Show resolved Hide resolved
pkg/rules/manager.go Show resolved Hide resolved
@bwplotka bwplotka force-pushed the ruler-rulesAPI branch 6 times, most recently from c72d4f7 to 8fa2a4b Compare May 7, 2020 18:30
povilasv
povilasv approved these changes May 8, 2020
View reviewed changes
Copy link
Member

@povilasv povilasv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work 🥇 love the tests

@s-urbaniak
Copy link
Contributor

@bwplotka ping :-) can we merge once tests are fixed, i'd love to rebase on these changes 💚

@bwplotka
Copy link
Member Author

@s-urbaniak It's not as easy as it looks. I changed Prometheus version, so all deps will be reinstalled on CI causing major slowdown. We need to update CI but in mean time Go version changed on master.. 🤦

Plus I am fixing other things, Now. I would propose following:

Since we have approval on design, let's merge CURRENT feature branch to master (or rather. propose a PR first). Let's rebase / pull latest master as well. Can we do it? 🤗 With this it would be easier for me to rebase my PRs on this (since this is actually chained with #2558 (!), then fix tests, once we have Go version. Ideally we could merge feature branch now and iterate on master from now! (:

@s-urbaniak
Copy link
Contributor

That would be a question for the remaining maintainers I guess, fine from my side, but I don't know what the policy is regarding master feature consistency.

@bwplotka
Copy link
Member Author

So far things can be in progress. E.g see the receiver work. It was done on master.

@bwplotka
Copy link
Member Author

As long as our new flags are hidden

@bwplotka
Copy link
Member Author

But yea, let's just rebase/ pull master to feature branch anyway =D

@brancz
Copy link
Member

brancz commented May 14, 2020

I'm fine with merging things in steps as long as the flags are hidden until the feature is done and out of experimental.

@bwplotka bwplotka force-pushed the ruler branch 4 times, most recently from 3318116 to 7cdd6c3 Compare May 20, 2020 09:29
Base automatically changed from ruler to features/rules-proxy May 20, 2020 10:30
@bwplotka bwplotka force-pushed the ruler-rulesAPI branch 2 times, most recently from 77269f6 to ca75efb Compare May 20, 2020 10:32
@bwplotka
Copy link
Member Author

Should be good to go @s-urbaniak

@s-urbaniak
Copy link
Contributor

lgtm modulo the tests! 💚

@bwplotka bwplotka force-pushed the ruler-rulesAPI branch 7 times, most recently from 8eba0fd to 8d4e9dd Compare May 21, 2020 09:49
@bwplotka
Copy link
Member Author

bwplotka commented May 21, 2020 •
edited
Loading

Looks like there is some deduplication bug, but you totally rewrote this in next PR: #2555

So I am skipping TestRulesAPI_Fanout test here. Let's enable on feature branch.

@bwplotka bwplotka merged commit 3fe1758 into features/rules-proxy May 21, 2020
@bwplotka bwplotka deleted the ruler-rulesAPI branch May 21, 2020 13:23
bwplotka added a commit that referenced this pull request May 22, 2020
@bwplotka
Added Ruler support for RulesAPI; Refactored Manager. (#2562)
87bcc73 
As per: https://thanos.io/proposals/202003_thanos_rules_federation.md/

Signed-off-by: Bartlomiej Plotka <[email protected]>
brancz pushed a commit that referenced this pull request May 25, 2020
@bwplotka @s-urbaniak
Merged Experimental Federated Rules API feature branch to master. (#2200
c733564 
)

* Added RulesAPI.

Signed-off-by: Bartlomiej Plotka <[email protected]>

* Added warnings.

Signed-off-by: Bartlomiej Plotka <[email protected]>

* Added Type to rules requests as it is on HTTP API. (#2201)

Signed-off-by: Bartlomiej Plotka <[email protected]>

* 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 <[email protected]>

* proto: regenerate

Signed-off-by: Sergiusz Urbaniak <[email protected]>

* Made storepb.RuleGroups a source of truth for rules API (Go, JSON, proto). Added tests. (#2242)

Signed-off-by: Bartlomiej Plotka <[email protected]>

* 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 <[email protected]>

* Fixed broken test.

Signed-off-by: Bartlomiej Plotka <[email protected]>

* 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 <[email protected]>

* TestPrometheusStore_Rules_e2e: fix test fixture

Signed-off-by: Sergiusz Urbaniak <[email protected]>

Co-authored-by: Sergiusz Urbaniak <[email protected]>

* cmd/thanos/query: add initial rules support (#2240)

* cmd/thanos/query: add initial rules support

Signed-off-by: Sergiusz Urbaniak <[email protected]>

* pkg/query/api/v1: initial implementation

Signed-off-by: Sergiusz Urbaniak <[email protected]>

* e2e: initial implementation and fixes

Signed-off-by: Sergiusz Urbaniak <[email protected]>

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

Signed-off-by: Sergiusz Urbaniak <[email protected]>

* Refactored proto generation and separated store from rules APIs. (#2558)

* Refactored proto generation and separate store from rules APIs.

Signed-off-by: Bartlomiej Plotka <[email protected]>

* Addressed comments.

Signed-off-by: Bartlomiej Plotka <[email protected]>

* Fixed proto gen.

Signed-off-by: Bartlomiej Plotka <[email protected]>

* Addressed Serg comments.

Signed-off-by: Bartlomiej Plotka <[email protected]>

* Added Ruler support for RulesAPI; Refactored Manager. (#2562)

As per: https://thanos.io/proposals/202003_thanos_rules_federation.md/

Signed-off-by: Bartlomiej Plotka <[email protected]>

* Small fixes to changelog and flags. Do not add any.

Signed-off-by: Bartlomiej Plotka <[email protected]>

* Fixed after rebase.

Signed-off-by: Bartlomiej Plotka <[email protected]>

Co-authored-by: Sergiusz Urbaniak <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@s-urbaniak s-urbaniak s-urbaniak left review comments

@povilasv povilasv povilasv approved these changes

@kakkoyun kakkoyun Awaiting requested review from kakkoyun

@GiedriusS GiedriusS Awaiting requested review from GiedriusS

@brancz brancz Awaiting requested review from brancz

@squat squat Awaiting requested review from squat

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bwplotka @s-urbaniak @brancz @povilasv