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

Use proto rules API instead of struct; Moved as much as possible to promclient; Added rulesAPI RPC to sidecar. #2243

Merged
merged 3 commits into from
Mar 19, 2020
Merged

Use proto rules API instead of struct; Moved as much as possible to promclient; Added rulesAPI RPC to sidecar. #2243

merged 3 commits into from
Mar 19, 2020

Conversation

bwplotka
Copy link
Member

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

cc @s-urbaniak

@bwplotka bwplotka changed the title Use proto rules API instead of struct; Added rulesAPI RPC to sidecar. Use proto rules API instead of struct; Moved as much as possible to promclient; Added rulesAPI RPC to sidecar. Mar 10, 2020
@bwplotka bwplotka mentioned this pull request Mar 10, 2020
Closed
@bwplotka bwplotka force-pushed the api-on-proto branch 2 times, most recently from 299c29d to bac1909 Compare March 10, 2020 19:04
@bwplotka
Copy link
Member Author

This should be ready to go @s-urbaniak!

@bwplotka
Copy link
Member Author

PTAL

@bwplotka bwplotka requested review from yeya24, GiedriusS and squat March 10, 2020 19:05
yeya24
yeya24 reviewed Mar 11, 2020
View reviewed changes
pkg/promclient/promclient.go Outdated
}
defer runutil.ExhaustCloseWithLogOnErr(logger, resp.Body, "metrics body")
fmt.Println(matcher)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@bwplotka bwplotka force-pushed the api-on-proto branch 2 times, most recently from ff9d8a7 to ac89485 Compare March 11, 2020 11:14
s-urbaniak
s-urbaniak reviewed Mar 11, 2020
View reviewed changes
pkg/promclient/promclient.go
if err != nil {
return nil, nil, errors.Wrapf(err, "perform GET request against %s", u.String())
return nil, nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

no wrapping is necessary any more?

Copy link
Member Author

Choose a reason for hiding this comment

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

correct

Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: Use with Stack on command error.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fatalf("%+v", err)

pkg/errors

Copy link
Member Author

Choose a reason for hiding this comment

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

#2277

@s-urbaniak
Copy link
Contributor

wow, this is huge 😲
i am a bit overwhelmed, but i see RulesServer now being registered, so i am super happy 🎉 😊

@bwplotka
Copy link
Member Author

Happy to have a quick pair-review @s-urbaniak if needed

@bwplotka bwplotka force-pushed the api-on-proto branch 2 times, most recently from 7616456 to 5d0c93f Compare March 11, 2020 12:44
bwplotka
bwplotka commented Mar 11, 2020
View reviewed changes
pkg/promclient/promclient.go
if err != nil {
return nil, nil, errors.Wrapf(err, "perform GET request against %s", u.String())
return nil, nil, err
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: Use with Stack on command error.

pkg/promclient/promclient.go
if err != nil {
return nil, nil, errors.Wrapf(err, "perform GET request against %s", u.String())
return nil, nil, err
Copy link
Member Author

Choose a reason for hiding this comment

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

Fatalf("%+v", err)

pkg/errors

pkg/testutil/testpromcompatibility/api_compatibility.go
PartialResponseStrategy string `json:"partialResponseStrategy"`
}

type AlertingRule struct {
Copy link
Member Author

Choose a reason for hiding this comment

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

Upstream make it public?

@s-urbaniak
Copy link
Contributor

lgtm 👍

@s-urbaniak
Copy link
Contributor

looking at the CI failure 👀

@s-urbaniak
Copy link
Contributor

yay, I can repro locally, let me try to fix this so I'll learn 👷‍♂️

@bwplotka
Copy link
Member Author

Are you on it @s-urbaniak ? If not, I can fix

@bwplotka
Copy link
Member Author

Fixed hopefully.

@s-urbaniak
Copy link
Contributor

@bwplotka i was on it, but my fix was related to the test itself, i will continue from here for the remaining failures.

@s-urbaniak s-urbaniak mentioned this pull request Mar 19, 2020
Merged
@s-urbaniak
Use proto rules API instead of struct; Moved as much as possible to p…
b5f814d 
…romclient; 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]>
@bwplotka bwplotka merged commit 932f2ee into features/rules-proxy Mar 19, 2020
@bwplotka
Copy link
Member Author

Thanks @s-urbaniak for help!

Let's move it forward.

We still need:

  • e2e tests
  • proxy implementation
  • ruler implementation

@bwplotka bwplotka deleted the api-on-proto branch March 19, 2020 11:57
@s-urbaniak
Copy link
Contributor

@bwplotka on it 👍

bwplotka added a commit that referenced this pull request May 22, 2020
@bwplotka @s-urbaniak
Use proto rules API instead of struct; Moved as much as possible to p…
a9c660d 
…romclient; 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]>
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

@yeya24 yeya24 yeya24 left review comments

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

@GiedriusS GiedriusS Awaiting requested review from GiedriusS

@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.
3 participants
@bwplotka @s-urbaniak @yeya24