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.

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

ruler: fix the /api/v1/rules endpoint #1773

Merged
merged 4 commits into from
Nov 22, 2019

Conversation

simonpasquier
Copy link
Contributor

@simonpasquier simonpasquier commented Nov 21, 2019

Closes #1189

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Fixed the switch type in the rules handler because alerting rules are instances of *rules.AlertingRule, not thanosrule.AlertingRule. This is because the rules are returned from the Prometheus rule managers and have no notion of PartialResponseStrategy.

Verification

The /api/v1/rules and /api/v1/alerts endpoints are now covered by the end-to-end tests.

Signed-off-by: Simon Pasquier <spasquie@redhat.com>
@simonpasquier simonpasquier changed the title test/e2e: test alerts and rules API endpoints ruler: fix the /api/v1/rules endpoint Nov 21, 2019
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
@s-urbaniak
Copy link
Contributor

/cc @bwplotka

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Some comments, otherwise LGTM (:

expr1, err := promql.ParseExpr(`absent(test_metric3) != 1`)
if err != nil {
m.testing.Fatalf("unable to parse alert expression: %s", err)
panic(fmt.Sprintf("unable to parse alert expression: %s", err))
Copy link
Member

Choose a reason for hiding this comment

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

why panicking in tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it should never fail but I can revert if you prefer.

CHANGELOG.md Outdated
@@ -27,6 +27,7 @@ We use *breaking* word for marking changes that are not backward compatible (rel
- [#1669](https://github.com/thanos-io/thanos/pull/1669) Fixed store sharding. Now it does not load excluded meta.jsons and load/fetch index-cache.json files.
- [#1670](https://github.com/thanos-io/thanos/pull/1670) Fixed un-ordered blocks upload. Sidecar now uploads the oldest blocks first.
- [#1568](https://github.com/thanos-io/thanos/pull/1709) Thanos Store now retains the first raw value of a chunk during downsampling to avoid losing some counter resets that occur on an aggregation boundary.
- [#1773](https://github.com/thanos-io/thanos/pull/1773) Thanos Ruler: fixed the /api/v1/rules endpoint.
Copy link
Member

Choose a reason for hiding this comment

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

Can we tell users what exactly problem was fixed?

Signed-off-by: Simon Pasquier <spasquie@redhat.com>
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM

@bwplotka bwplotka merged commit 40cb634 into thanos-io:master Nov 22, 2019
@s-urbaniak
Copy link
Contributor

thanks @bwplotka for the review and @simonpasquier for the contribution! 🎉

@simonpasquier simonpasquier deleted the fix-rules-api branch November 25, 2019 08:51
IKSIN pushed a commit to monitoring-tools/thanos that referenced this pull request Nov 26, 2019
* test/e2e: test alerts and rules API endpoints

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

* pkg/rule: fix /api/v1/rules endpoint

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

* Address Bartek's comments

Signed-off-by: Simon Pasquier <spasquie@redhat.com>
Signed-off-by: Aleksey Sin <asin@ozon.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rules api internal failed to assert type of rule
3 participants