Add matcher support to Query Rules endpoint #5111

saswatamcode · 2022-01-29T15:05:53Z

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

hanjm · 2022-01-30T09:52:33Z

pkg/rules/rules.go

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


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

hanjm · 2022-01-30T09:55:12Z

CHANGELOG.md

@@ -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


A small nit: add dot at the line end.

Done! Also, have added tests! 🙂

hanjm · 2022-01-30T09:58:02Z

It will be better to add some unit tests (:

Signed-off-by: Saswata Mukherjee <[email protected]>

saswatamcode · 2022-01-31T04:24:14Z

Added tests pass here!

matej-g · 2022-01-31T09:38:28Z

pkg/rules/rulespb/rpc.proto

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


Should we keep this field name snake_case?

Sure, let me change!

matej-g · 2022-01-31T09:41:04Z

pkg/rules/rules.go

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


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")

Ah! Nice catch, will change this!

matej-g · 2022-01-31T09:42:47Z

pkg/rules/rules.go

@@ -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


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

Yes, will pre-allocate!

Signed-off-by: Saswata Mukherjee <[email protected]>

matej-g

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 👍.

jessicalins

cool! @saswatamcode 🎉

jessicalins · 2022-02-01T19:33:41Z

pkg/rules/rules.go

@@ -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))


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

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. 🙂

sounds good!

onprem

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

pkg/api/query/v1.go

pkg/rules/rules.go

pkg/rules/rules_test.go

Signed-off-by: Saswata Mukherjee <[email protected]>

onprem

LGTM. Great work!

* Add matcher support to Query Rules endpoint Signed-off-by: Saswata Mukherjee <[email protected]> * Add CHANGELOG entry Signed-off-by: Saswata Mukherjee <[email protected]> * Add tests and implement suggestions Signed-off-by: Saswata Mukherjee <[email protected]> * Pre-allocate matcherSets Signed-off-by: Saswata Mukherjee <[email protected]> * Add other matchtype testcase Signed-off-by: Saswata Mukherjee <[email protected]> Signed-off-by: Nicholaswang <[email protected]>

Add matcher support to Query Rules endpoint

cdc3c2a

Signed-off-by: Saswata Mukherjee <[email protected]>

pull-request-size bot added the size/L label Jan 29, 2022

Add CHANGELOG entry

91002dc

Signed-off-by: Saswata Mukherjee <[email protected]>

hanjm reviewed Jan 30, 2022

View reviewed changes

Add tests and implement suggestions

4a1dc37

Signed-off-by: Saswata Mukherjee <[email protected]>

pull-request-size bot added size/XL and removed size/L labels Jan 31, 2022

matej-g reviewed Jan 31, 2022

View reviewed changes

Pre-allocate matcherSets

2120a1c

Signed-off-by: Saswata Mukherjee <[email protected]>

matej-g previously approved these changes Jan 31, 2022

View reviewed changes

saswatamcode mentioned this pull request Feb 1, 2022

Add matcher based tenancy to api/v1/rules observatorium/api#204

Merged

jessicalins reviewed Feb 1, 2022

View reviewed changes

onprem previously approved these changes Feb 2, 2022

View reviewed changes

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

Add other matchtype testcase

9e25ab2

Signed-off-by: Saswata Mukherjee <[email protected]>

saswatamcode dismissed stale reviews from onprem and matej-g via 9e25ab2 February 3, 2022 03:36

saswatamcode requested a review from onprem February 3, 2022 10:29

onprem approved these changes Feb 3, 2022

View reviewed changes

onprem merged commit 4357002 into thanos-io:main Feb 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add matcher support to Query Rules endpoint #5111

Add matcher support to Query Rules endpoint #5111

saswatamcode commented Jan 29, 2022 •

edited

Loading

hanjm Jan 30, 2022

saswatamcode Jan 31, 2022

hanjm Jan 30, 2022

saswatamcode Jan 31, 2022

hanjm commented Jan 30, 2022

saswatamcode commented Jan 31, 2022

matej-g Jan 31, 2022

saswatamcode Jan 31, 2022

saswatamcode Jan 31, 2022

matej-g Jan 31, 2022

saswatamcode Jan 31, 2022

saswatamcode Jan 31, 2022

matej-g Jan 31, 2022

saswatamcode Jan 31, 2022

saswatamcode Jan 31, 2022

matej-g left a comment

jessicalins left a comment

jessicalins Feb 1, 2022

saswatamcode Feb 2, 2022

jessicalins Feb 2, 2022

onprem left a comment

onprem left a comment

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

Add matcher support to Query Rules endpoint #5111

Add matcher support to Query Rules endpoint #5111

Conversation

saswatamcode commented Jan 29, 2022 • edited Loading

Changes

Verification

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanjm commented Jan 30, 2022

saswatamcode commented Jan 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matej-g left a comment

Choose a reason for hiding this comment

jessicalins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onprem left a comment

Choose a reason for hiding this comment

onprem left a comment

Choose a reason for hiding this comment

saswatamcode commented Jan 29, 2022 •

edited

Loading