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

Add matcher support to Query Rules endpoint #5111

Merged
merged 5 commits into from
Feb 3, 2022

Conversation

saswatamcode
Copy link
Member

@saswatamcode saswatamcode commented Jan 29, 2022

This PR adds support for label matchers as URL query params to the api/v1/rules endpoint of Thanos Query API. This will not break existing functionality.

It filters both recording and alerting rules via their non-templated labels and doesn't take the rendered labels of active alerts into account.

Currently, this PR only implements this functionality in Thanos, but this can be changed to a Prometheus-based implementation in the future (relevant PR: prometheus#10194)!

Addresses #4812.

Open question: Templated labels are identified by parsing the label values as templates and checking the number and type of node. So if it is parsed as a single node of type parser.NodeText it is treated as a non-templated label. Would a regex-based approach be better suited here?

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

Changes

  • Adds support for match[] URL param in api/v1/rules of Query API.
  • Adds MatcherString to RulesRequest in rules proto

Verification

Tested locally.

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
}

for _, g := range ruleGroups {
filteredRules := []*rulespb.Rule{}
Copy link
Member

Choose a reason for hiding this comment

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

A small advice: use filteredRules := g.Rules[:0] avoid allocate.
https://github.com/golang/go/wiki/SliceTricks#filtering-without-allocating

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!

CHANGELOG.md Outdated
@@ -24,6 +24,7 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re
- [#4974](https://github.com/thanos-io/thanos/pull/4974) Store: Support tls_config configuration for connecting with Azure storage.
- [#4999](https://github.com/thanos-io/thanos/pull/4999) COS: Support `endpoint` configuration for vpc internal endpoint.
- [#5059](https://github.com/thanos-io/thanos/pull/5059) Compactor: Adding minimum retention flag validation for downsampling retention.
- [#5111](https://github.com/thanos-io/thanos/pull/5111) Add matcher support to Query 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.

A small nit: add dot at the line end.

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! Also, have added tests! 🙂

@hanjm
Copy link
Member

hanjm commented Jan 30, 2022

It will be better to add some unit tests (:

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
@saswatamcode
Copy link
Member Author

Added tests pass here!

@@ -40,6 +40,7 @@ message RulesRequest {
}
Type type = 1;
PartialResponseStrategy partial_response_strategy = 2;
repeated string MatcherString = 3;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we keep this field name snake_case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, let me change!

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!

for _, s := range req.MatcherString {
matchers, err := parser.ParseMetricSelector(s)
if err != nil {
return nil, nil, errors.Wrap(err, "proxy Rules")
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Should make the error description just a tiny bit clearer?

Suggested change
return nil, nil, errors.Wrap(err, "proxy Rules")
return nil, nil, errors.Wrap(err, "parser ParseMetricSelector")

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah! Nice catch, will change 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!

@@ -58,6 +61,16 @@ func (rr *GRPCClient) Rules(ctx context.Context, req *rulespb.RulesRequest) (*ru
return nil, nil, errors.Wrap(err, "proxy Rules")
}

var matcherSets [][]*labels.Matcher
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we pre-allocate here? We know the final slice's length, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, will pre-allocate!

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!

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
matej-g
matej-g previously approved these changes Jan 31, 2022
Copy link
Collaborator

@matej-g matej-g left a comment

Choose a reason for hiding this comment

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

Nice job @saswatamcode!

Re regex approach, to me doing this node check seems more straightforward (and possibly also better performance-wise).

I'm wondering how long before this is supported in the downstream in Prometheus, since the linked PR seem to be well in progress, but I guess this is a good solution for the meantime 👍.

Copy link
Contributor

@jessicalins jessicalins left a comment

Choose a reason for hiding this comment

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

cool! @saswatamcode 🎉

@@ -58,6 +61,16 @@ func (rr *GRPCClient) Rules(ctx context.Context, req *rulespb.RulesRequest) (*ru
return nil, nil, errors.Wrap(err, "proxy Rules")
}

var err error
matcherSets := make([][]*labels.Matcher, len(req.MatcherString))
Copy link
Contributor

Choose a reason for hiding this comment

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

should we check if len(req.MatcherString) > 0 or this is already covered from here?

Copy link
Member Author

Choose a reason for hiding this comment

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

If the length is zero, matcherSets would be an empty array of type [][]*labels.Matcher and the loop below this won't run. filterRules gets called right after and it checks the length of matcherSets here and if it is 0, returns all the rules. So we kind of do check for the length of MatcherString, just not directly. 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good!

onprem
onprem previously approved these changes Feb 2, 2022
Copy link
Member

@onprem onprem left a comment

Choose a reason for hiding this comment

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

Neat work! I have a couple of non-blocking comments, but this looks ready to merge. 🚀

pkg/api/query/v1.go Outdated Show resolved Hide resolved
pkg/rules/rules.go Outdated Show resolved Hide resolved
pkg/rules/rules_test.go Show resolved Hide resolved
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
@saswatamcode saswatamcode dismissed stale reviews from onprem and matej-g via 9e25ab2 February 3, 2022 03:36
Copy link
Member

@onprem onprem left a comment

Choose a reason for hiding this comment

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

LGTM. Great work!

@onprem onprem merged commit 4357002 into thanos-io:main Feb 3, 2022
Nicholaswang pushed a commit to Nicholaswang/thanos that referenced this pull request Mar 6, 2022
* Add matcher support to Query Rules endpoint

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>

* Add CHANGELOG entry

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>

* Add tests and implement suggestions

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>

* Pre-allocate matcherSets

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>

* Add other matchtype testcase

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
Signed-off-by: Nicholaswang <wzhever@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants