Add label-matcher support to Rules API by saswatamcode · Pull Request #10194 · prometheus/prometheus

saswatamcode · 2022-01-22T15:40:00Z

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

It filters both recording and alerting rules via their labels (both templated and non-templated).

Open question on whether active alerts should be handled differently?

A major use case for this is multi-tenant applications built on Prometheus which require a way to isolate tenant rules. Currently, one possible way of doing this is by filtering the API response. But, for endpoints like api/v1/labels, only a matcher needs to be added to URL params. So this PR aims to bring the same convenience to rules.

Fixes #9627.

roidelapluie · 2022-01-24T23:10:07Z

While it makes sense to filter active alerts, it does not make sense to filter alert and rules definition as the label value is a templated string. I am against adding this feature.

roidelapluie · 2022-01-24T23:11:40Z

I have also commented on the issue.

bwplotka

Thanks @saswatamcode for this PR and @roidelapluie for review.

Let's start with Rules API first as this is the API that matters the most.

And as per your point @roidelapluie I see this being weird from the first glance, but what if we provide the most straightforward mechanism that will be expected by everyone. Rules API is all about loaded configured, so to me we should filter by non-rendered, configured labels. This is also how it is implemented right now in this PR. I proposed a doc entry. What's wrong with that @roidelapluie 🤔

docs/querying/api.md

rules/manager.go

web/api/v1/api.go

docs/querying/api.md

web/api/v1/api_test.go

roidelapluie · 2022-01-26T14:18:47Z

What is wrong is the semantic. I'd rather have match[] only match labels, and not templates in one API and label values in another API. I'd be okay if we would make this available in a different way, but not via a "promql-like" selector, because it does not make sense to apply regex on golang templates like if they were metrics, it's mixing apples and oranges.

saswatamcode · 2022-01-27T05:37:57Z

As @bwplotka suggested then, maybe we can keep it in the simplest form and match only non-templated labels in rules (with a change in docs to highlight this)? That way match[] doesn't behave differently from other APIs and we don't apply regex on templates. So match[] in api/v1/rules will only match non-templated labels in rules (currently not adding it in /api/v1/alerts). WDYT @roidelapluie? 🙂

roidelapluie · 2022-01-27T19:06:50Z

I trust Bartek's judgment, let's document this properly.

saswatamcode · 2022-01-28T10:32:07Z

I have updated this PR with a doc change. Now only api/v1/rules support matchers and that too for non-templated label values only (approach for detecting templating is using parser node type, but can be changed to a regex-based one)! 🙂

Also, test failure seems unrelated, TestQueryConcurrency in promql/engine_test.go fails in test_windows but haven't been able to reproduce it locally. Relevant log lines here. Maybe running into #6672?

saswatamcode · 2022-02-07T04:30:26Z

Is there anything else that seems to be missing or is blocking this PR, @roidelapluie? Would love to progress on this! 🙂

qinxx108 · 2023-10-03T22:35:50Z

Regarding to only match non-templated labels in rules. From user usage perspective, i should be able to filter based on what i configured? For example as in the test cases, if my rule have a label
Name: "templatedlabel", Value: "{{ $externalURL }}"
If i search for templatedlabel= {{ $externalURL }}, it should return to me the rules that has templatedlabel= {{ $externalURL }}

rules/manager.go

saswatamcode · 2023-10-04T06:24:57Z

Hmm, not sure why the test/build fails here...maybe due to stringlabels?

alvinlin123 · 2023-10-07T01:16:10Z

Regarding to only match non-templated labels in rules. From user usage perspective, i should be able to filter based on what i configured? For example as in the test cases, if my rule have a label Name: "templatedlabel", Value: "{{ $externalURL }}" If i search for templatedlabel= {{ $externalURL }}, it should return to me the rules that has templatedlabel= {{ $externalURL }}

I agree with this. While I understand @roidelapluie view about the semantic of match for templated string, as @bwplotka said "...Rules API is all about loaded configured" so it makes perfect sense the filter parameter works against whatever is configured, which includes the templated string. As a system admin, there are legit use cases where I want to do something like "gimme all the rule definition that has {{ $someTypo }}" because I made a boo boo and I want to production impact analysis and fix the issue.

For

maybe we can keep it in the simplest form and match only non-templated labels in rules

I am against this because it's not intuitive. Sure we can document it, but I think it's simpler for the the suer to understand "the filter works against template string" than "if the value is a template string then ... otherwise ...". Math only non-templated labels seems like an inconsistent behaviour to me.

roidelapluie · 2023-10-07T05:02:01Z

Okay.

alvinlin123 · 2023-10-07T05:21:28Z

Okay

Love the conciseness :D

bboreham · 2023-10-09T15:16:12Z

Hmm, not sure why the test/build fails here...maybe due to stringlabels?

The error log says:

rules/manager.go:467:23: cannot range over l (variable of type labels.Labels)
rules/manager.go:485:24: cannot range over l (variable of type labels.Labels)

It was never a great idea to assume labels.Labels was a slice, and #10991 made this concrete.

You could use code like this:

prometheus/model/labels/test_utils.go

Lines 34 to 41 in 617bee6

    
           func (s Selector) Matches(labels Labels) bool { 
        
           	for _, m := range s { 
        
           		if v := labels.Get(m.Name); !m.Matches(v) { 
        
           			return false 
        
           		} 
        
           	} 
        
           	return true 
        
           }

saswatamcode · 2023-10-10T06:23:18Z

Thanks @bboreham!

@alvinlin123 / @roidelapluie let me know, if we should remove the templated labels check here then.

roidelapluie · 2023-10-10T06:30:13Z

Yes please

saswatamcode · 2023-10-10T07:12:57Z

Done!

docs/querying/api.md

rules/manager.go

roidelapluie · 2023-11-03T19:58:13Z

rules/manager.go

+	var ok bool
+	for _, matchers := range matcherSets {
+		if matches(lbls, matchers...) {
+			ok = true


we should do the opposite. I expect that ALL matchers should match

the other query parameters for example
rule_name[], rule_group[], and file[] are doing
"If the parameter is repeated, rules with any of the provided rule group names are returned." should we follow this behavior as well for the new match parameter?
Reference:
https://github.com/prometheus/prometheus/pull/12270/files

My expectation is that all the Matchers in each []*labels.Matcher will be ANDed, and then the set of those will be ORed. I think this is what we do elsewhere.
It could be made clearer in comments.

Yup, I think this is what we are doing here. matches ANDs all the matchers in each set i.e return false if any of them don't match, and then matchesMatcherSet takes the set of those results and ORs them i.e returns true if any of them are true. Added a comment for this.

roidelapluie · 2023-11-03T19:58:21Z

rules/manager.go

+		return true
+	}
+
+	var ok bool


should be true by default

qinxx108 · 2023-12-27T21:29:36Z

@roidelapluie could you please take a look at this pr again? Thanks

bboreham

Code looks broadly fine; I made some small notes in places.

A major use case for this is multi-tenant applications built on Prometheus

Having worked on Cortex and Mimir, I don't understand this part. Both of them take a tenant ID in the request, and return rules only for that tenant.

Can you plese update the description to match what you want to get merged?

bboreham · 2024-01-15T17:10:12Z

rules/manager.go

+	var ok bool
+	for _, matchers := range matcherSets {
+		if matches(lbls, matchers...) {
+			ok = true


My expectation is that all the Matchers in each []*labels.Matcher will be ANDed, and then the set of those will be ORed. I think this is what we do elsewhere.
It could be made clearer in comments.

docs/querying/api.md

web/api/v1/api_test.go

web/api/v1/errors_test.go

alvinlin123 · 2024-01-15T19:37:05Z

A major use case for this is multi-tenant applications built on Prometheus
Having worked on Cortex and Mimir, I don't understand this part. Both of them take a tenant ID in the request, and return rules only for that tenant.
Can you plese update the description to match what you want to get merged?

Hi @bboreham! Long time no "see"!

Yea initially I found this use case head scratching too. But I think the original author is implementing some kind of "poor man's multi-tenancy" on top of just Prometheus using just labelling convention for metrics and rules.

saswatamcode · 2024-01-16T03:52:50Z

I think the original author is implementing some kind of "poor man's multi-tenancy" on top of just Prometheus using just labelling convention for metrics and rules

That's correct, this is what the PR aims to do! We already kind of have this in Thanos, and can just use a proxy in front to ensure rules with a particular tenant label get returned, without having to maintain the notion of tenancy internally.

This isn't a new pattern I guess and can extend beyond tenancy. Even projects like prom-label-proxy need to modify rules response and filter currently, when just setting a match[] param would be more convenient.

Can you please update the description to match what you want to get merged?

The original description would still be valid I think, but I'm happy to change it to something more suitable. Open to any suggestions cc: @qinxx108 🙂

bboreham · 2024-01-16T11:53:32Z

The original description would still be valid I think,

I say this statement is not true, given Cortex etc. manage to do it:

multi-tenant applications built on Prometheus [...] Currently, this is only possible by filtering the API response

bwplotka · 2024-01-16T11:57:10Z

(During Prometheus bug scrub meeting)

Just to make sure it's clear: Sounds like maintainers would accept such a change, looks like there are two use cases (workable UI and label-based multi-tenancy).

saswatamcode · 2024-01-16T12:08:14Z

Thanks! Have updated it to:

Currently, one possible way of doing this is by filtering the API response.

qinxx108 · 2024-02-05T17:35:36Z

@saswatamcode @roidelapluie @bboreham I've addressed the comments. Please take another look if you have time!

Ty!

yeya24 · 2024-07-09T17:41:01Z

Hi Prometheus maintainers @bboreham @roidelapluie @bwplotka, it looks like there is a concensus to accept this change. It would be great help if any of the maintainer could take a look. Thanks!

bwplotka · 2024-07-10T06:17:49Z

(https://cloud-native.slack.com/archives/C01AUBA4PFE/p1720546903110739) discussion why it's not merged. TL;DR: Comments not adresseed (plus now needs rebase).

But generally looks good, just let's agree on implementation and defaults.

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com> Signed-off-by: Yijie Qin <qinyijie@amazon.com>

Signed-off-by: Yijie Qin <qinyijie@amazon.com>

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

bwplotka

LGTM, thanks!

* Add label-matcher support to Rules API Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com> Signed-off-by: Yijie Qin <qinyijie@amazon.com> * Implement suggestions Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com> Signed-off-by: Yijie Qin <qinyijie@amazon.com> * Match any matcherSet instead of all Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com> Signed-off-by: Yijie Qin <qinyijie@amazon.com> * Don't treat labels.Labels as slice Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com> Signed-off-by: Yijie Qin <qinyijie@amazon.com> * Remove non-templated check and fix tests Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com> Signed-off-by: Yijie Qin <qinyijie@amazon.com> * Update docs Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com> Signed-off-by: Yijie Qin <qinyijie@amazon.com> * fix comments Signed-off-by: Yijie Qin <qinyijie@amazon.com> * fix comment Signed-off-by: Yijie Qin <qinyijie@amazon.com> * Add comment for matching logic, fix tests after rebase Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com> --------- Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com> Signed-off-by: Yijie Qin <qinyijie@amazon.com> Co-authored-by: Yijie Qin <qinyijie@amazon.com> Signed-off-by: kushagra Shukla <kushalshukla110@gmail.com>

* Add label-matcher support to Rules API Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com> Signed-off-by: Yijie Qin <qinyijie@amazon.com> * Implement suggestions Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com> Signed-off-by: Yijie Qin <qinyijie@amazon.com> * Match any matcherSet instead of all Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com> Signed-off-by: Yijie Qin <qinyijie@amazon.com> * Don't treat labels.Labels as slice Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com> Signed-off-by: Yijie Qin <qinyijie@amazon.com> * Remove non-templated check and fix tests Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com> Signed-off-by: Yijie Qin <qinyijie@amazon.com> * Update docs Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com> Signed-off-by: Yijie Qin <qinyijie@amazon.com> * fix comments Signed-off-by: Yijie Qin <qinyijie@amazon.com> * fix comment Signed-off-by: Yijie Qin <qinyijie@amazon.com> * Add comment for matching logic, fix tests after rebase Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com> --------- Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com> Signed-off-by: Yijie Qin <qinyijie@amazon.com> Co-authored-by: Yijie Qin <qinyijie@amazon.com>

bwplotka reviewed Jan 26, 2022

View reviewed changes

docs/querying/api.md Outdated Show resolved Hide resolved

rules/manager.go Outdated Show resolved Hide resolved

web/api/v1/api.go Outdated Show resolved Hide resolved

docs/querying/api.md Outdated Show resolved Hide resolved

web/api/v1/api_test.go Show resolved Hide resolved

bwplotka mentioned this pull request Jan 26, 2022

Rules API: support label-based matcher. [Feature Request] #9627

Closed

saswatamcode mentioned this pull request Jan 29, 2022

Add matcher support to Query Rules endpoint thanos-io/thanos#5111

Merged

2 tasks

gotjosh self-requested a review May 25, 2023 16:59

roidelapluie self-assigned this Aug 8, 2023

roidelapluie mentioned this pull request Sep 29, 2023

Rules API : add rules label filter #12902

Closed

qinxx108 reviewed Oct 3, 2023

View reviewed changes

rules/manager.go Outdated Show resolved Hide resolved

saswatamcode force-pushed the rules-matcher branch from 168395b to 15f8133 Compare October 4, 2023 05:34

qinxx108 reviewed Oct 10, 2023

View reviewed changes

docs/querying/api.md Outdated Show resolved Hide resolved

roidelapluie reviewed Nov 3, 2023

View reviewed changes

qinxx108 force-pushed the rules-matcher branch from 8de11dd to d31e47a Compare December 27, 2023 00:50

bboreham reviewed Jan 15, 2024

View reviewed changes

qinxx108 force-pushed the rules-matcher branch from cfba634 to 13665b3 Compare January 21, 2024 08:34

saswatamcode and others added 8 commits July 10, 2024 10:47

Add label-matcher support to Rules API

2989ba9

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com> Signed-off-by: Yijie Qin <qinyijie@amazon.com>

Implement suggestions

8748a87

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com> Signed-off-by: Yijie Qin <qinyijie@amazon.com>

Match any matcherSet instead of all

0ab7d45

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com> Signed-off-by: Yijie Qin <qinyijie@amazon.com>

Don't treat labels.Labels as slice

9b9b717

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com> Signed-off-by: Yijie Qin <qinyijie@amazon.com>

Remove non-templated check and fix tests

a580a62

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com> Signed-off-by: Yijie Qin <qinyijie@amazon.com>

Update docs

ee34eaa

Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com> Signed-off-by: Yijie Qin <qinyijie@amazon.com>

fix comments

0c83bf1

Signed-off-by: Yijie Qin <qinyijie@amazon.com>

fix comment

7adbf2d

Signed-off-by: Yijie Qin <qinyijie@amazon.com>

saswatamcode force-pushed the rules-matcher branch from 13665b3 to e313ab1 Compare July 10, 2024 11:09

Add comment for matching logic, fix tests after rebase

398b17f

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

saswatamcode force-pushed the rules-matcher branch from e313ab1 to 398b17f Compare July 10, 2024 11:15

bwplotka approved these changes Jul 10, 2024

View reviewed changes

bwplotka merged commit 398f42d into prometheus:main Jul 10, 2024

euniceek mentioned this pull request Jul 12, 2024

Add matchers filtering field to rules api cortexproject/cortex#6083

Merged

3 tasks

rapphil mentioned this pull request Jul 15, 2024

Add paginated feature to list rules api #14017

Merged

saswatamcode mentioned this pull request Jul 19, 2024

api/rules: Add filtering on rule name/group/file thanos-io/thanos#7560

Merged

2 tasks

simonpasquier mentioned this pull request Jul 24, 2024

Make use of label matcher support in Rules API prometheus-community/prom-label-proxy#236

Closed

Conversation

saswatamcode commented Jan 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roidelapluie commented Jan 24, 2022

Uh oh!

roidelapluie commented Jan 24, 2022

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

roidelapluie commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saswatamcode commented Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roidelapluie commented Jan 27, 2022

Uh oh!

saswatamcode commented Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saswatamcode commented Feb 7, 2022

Uh oh!

qinxx108 commented Oct 3, 2023

Uh oh!

Uh oh!

saswatamcode commented Oct 4, 2023

Uh oh!

alvinlin123 commented Oct 7, 2023

Uh oh!

roidelapluie commented Oct 7, 2023

Uh oh!

alvinlin123 commented Oct 7, 2023

Uh oh!

bboreham commented Oct 9, 2023

Uh oh!

saswatamcode commented Oct 10, 2023

Uh oh!

roidelapluie commented Oct 10, 2023

Uh oh!

saswatamcode commented Oct 10, 2023

Uh oh!

Uh oh!

Uh oh!

roidelapluie Nov 3, 2023

Choose a reason for hiding this comment

Uh oh!

qinxx108 Dec 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bboreham Jan 15, 2024

Choose a reason for hiding this comment

Uh oh!

saswatamcode Jul 10, 2024

Choose a reason for hiding this comment

Uh oh!

roidelapluie Nov 3, 2023

Choose a reason for hiding this comment

Uh oh!

qinxx108 commented Dec 27, 2023

Uh oh!

bboreham left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bboreham Jan 15, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alvinlin123 commented Jan 15, 2024

saswatamcode commented Jan 22, 2022 •

edited

Loading

roidelapluie commented Jan 26, 2022 •

edited

Loading

saswatamcode commented Jan 27, 2022 •

edited

Loading

saswatamcode commented Jan 28, 2022 •

edited

Loading

qinxx108 Dec 27, 2023 •

edited

Loading

bboreham left a comment •

edited

Loading