feat: enable multiple label values #115

Kirchen99 · 2022-06-17T10:21:06Z

Signed-off-by: Kirchen99 latias_latios@126.com

Solves the issue: #27

Example:

Run prom-label-proxy with following args:

                "--insecure-listen-address=127.0.0.1:8080",
                "--upstream=http://demo.do.prometheus.io:9090",
                "--label=job"

Try to get something with curl:

curl http://127.0.0.1:8080/api/v1/query\?query\="up"\&job\="grafana"\&job\="random"\&job\="caddy"

See the result:

{
   "status":"success",
   "data":{
      "resultType":"vector",
      "result":[
         {
            "metric":{
               "__name__":"up",
               "instance":"demo.do.prometheus.io:3000",
               "job":"grafana"
            },
            "value":[
               1655460956.224,
               "1"
            ]
         },
         {
            "metric":{
               "__name__":"up",
               "instance":"demo.do.prometheus.io:8996",
               "job":"random"
            },
            "value":[
               1655460956.224,
               "1"
            ]
         },
         {
            "metric":{
               "__name__":"up",
               "instance":"demo.do.prometheus.io:8997",
               "job":"random"
            },
            "value":[
               1655460956.224,
               "1"
            ]
         },
         {
            "metric":{
               "__name__":"up",
               "instance":"demo.do.prometheus.io:8998",
               "job":"random"
            },
            "value":[
               1655460956.224,
               "1"
            ]
         },
         {
            "metric":{
               "__name__":"up",
               "instance":"demo.do.prometheus.io:8999",
               "job":"random"
            },
            "value":[
               1655460956.224,
               "1"
            ]
         },
         {
            "metric":{
               "__name__":"up",
               "instance":"localhost:2019",
               "job":"caddy"
            },
            "value":[
               1655460956.224,
               "1"
            ]
         }
      ]
   }
}

injectproxy/routes.go

simonpasquier

you'd need a rebase :)

.gitignore

injectproxy/routes.go

simonpasquier · 2022-07-08T15:08:12Z

injectproxy/routes.go

-	if r.labelValue != "" && formValue != "" {
+	err := req.ParseForm()
+	if err != nil {
+		return "", fmt.Errorf("the query parameter can not be parsed. %s", err.Error())


Suggested change

return "", fmt.Errorf("the query parameter can not be parsed. %s", err.Error())

return "", fmt.Errorf("the query parameter can not be parsed: %w", err)

injectproxy/routes.go

main.go

injectproxy/routes_test.go

injectproxy/routes.go

simonpasquier

I realize that there's more work needed: while the PR works the /api/v1/query and /api/v1/query_range endpoints, it would break other endpoints such as /federate, /api/v1/rules, /api/v2/silences, ...
I think that enforceLabel() needs to store the slice of label values in the context. Then each downstream handler does whatever it needs with it.

injectproxy/routes.go

simonpasquier

Thanks a lot for the effort! It's definitely more involved than initially envisioned 😅
I'm still not decided on the best way to deal with the silences endpoint, i'll need to think about it 🤔

injectproxy/silences.go

injectproxy/routes.go

injectproxy/silences.go

simonpasquier · 2022-08-04T09:36:39Z

injectproxy/silences.go

 	modified := models.Matchers{
-		&models.Matcher{Name: &(r.label), Value: &lvalue, IsRegex: &falsy},
+		&models.Matcher{Name: &(r.label), Value: &matcherValue, IsRegex: &truthy},
 	}
 	for _, m := range sil.Matchers {
 		if m.Name != nil && *m.Name == r.label {


if the enforced label is namespace=~"ns1|ns2" and someone creates a silence with namespace="ns1" then namespace="ns1" will be dropped which is not the desired outcome because you may want to narrow down the silence.

I think that we need to keep all input matchers and append the enforced matcher unconditionally? It would also simplify the delete endpoint since the only check would be to verify that the deleted silence contains a namespace=~"ns1|ns2" matcher (given example above).

I have updated the code.

If the enforced label is namespace=~"ns1", and someone creates a silence with namespace="ns1", then both matchers will be added:

[ "namespace=~\"ns1\"", "namespace=\"ns1\"" ]

For List, Update and Delete, only silences exactly contain a namespace=~"ns1" will be processed.

Did I understand you correctly?

injectproxy/routes.go

injectproxy/rules.go

Kirchen99 · 2022-09-09T15:16:07Z

I resolved some conversations to make this thread(pull request) easier to read and follow. Please tell me if I'd better not to resolve them. @simonpasquier

gasmick · 2022-09-29T09:58:49Z

Wow, it would be great to have this merged soon!

bt909 · 2022-10-04T09:57:11Z

Nice useful feature, is there any plan to merge this soon?

kekscode · 2022-10-06T08:09:48Z

@simonpasquier can we expect that this gets merged anytime soon or are there any open issues you see?

lud97x · 2022-10-17T13:18:52Z

+1

dbluxo · 2022-11-09T16:23:10Z

@simonpasquier doesn't seem to have had time to review this PR further. Would maybe someone else from the maintainers have time to take a look here? @squat @brancz

simonpasquier · 2022-12-02T15:48:48Z

hey! Sorry for the lag on this PR. I'm putting it at the top of my review list as I'm aware that there's high demand.

simonpasquier · 2023-01-04T13:31:24Z

@Kirchen99 now that #118 is merged, you'd have to rebase and resolve conflicts. Sorry about that but once it's done, I'll review the PR :)

Signed-off-by: Kirchen99 <latias_latios@126.com>

Signed-off-by: Kirchen99 <9981745+Kirchen99@users.noreply.github.com>

simonpasquier · 2023-06-12T13:27:39Z

👋 I'm taking a look at it right now. Since the silences API is problematic with multiple label values, I'd be in favor of declaring that the proxy doesn't support silences in this mode of operations. The main use case of multi-labels is for the Prometheus API anyway so not supporting the Alertmanager API isn't a blocker IMHO.

I'm going to push some commits on top of your PR to implement this.

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

When the original filter query parameter already includes the enforced label, the proxy should preserve it as it indicates that the user wants to filter down the list of silences/alerts. Signed-off-by: Simon Pasquier <spasquie@redhat.com>

When prom-label-proxy is configured with multiple label values, it should preserve the existing matchers because users actually want to return results for a subset of the metrics. In practice when configured with `namespace=~"bar|foo"`, the query `up{namespace="foo"}` is translated into `up{namespace="foo",namespace=~"bar|foo"}` instead of `{namespace=~"bar|foo"}`. This change is similar to 26b0a61 but for the Prometheus API endpoints. Signed-off-by: Simon Pasquier <spasquie@redhat.com>

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

simonpasquier · 2023-06-13T09:41:23Z

@Kirchen99 I've added a couple of commits on top of your work. The main changes are

Any call to the Alertmanager Silences endpoints with multi label values configured returns 422 Unprocessable Entity (https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/422).
When multi label values are configured, the proxy won't replace the existing matchers. E.g. when you query up{namespace="bar"}, the proxy should translate to up{namespace="bar",namespace=~"bar|foo"} and not up{namespace=~"bar|foo"}.
The same applies to the Alertmanager's Alerts and Alert groups API.

Regarding the last 2 points, I don't remember if we already discussed it in this PR but IMHO this is what makes most sense compared to dropping the existing matcher unilaterally.

My changes are https://github.com/prometheus-community/prom-label-proxy/pull/115/files/8472eba307611d1f2cf9181e70e3319ecbb90976..4882a18523e612324514b1f5ff8c7f8c465e71cc. Happy to hear your feedback.

Kirchen99 · 2023-06-13T10:36:28Z

@simonpasquier Thank you for your commits and the detailed explanation of the changes. I like your Idea on the last 2 points and really appreciate your work and the improvements you've made. The changes look good to me, and I'm excited to see them being merged.

README.md

simonpasquier

I'll give a few days for @squat and @SuperQ to review the change (it's quite large). If no reply, I'll merge it by the end of the week probably and cut a new release.

simonpasquier · 2023-06-15T07:29:24Z

Thanks again @Kirchen99 for your tenacity 🎉 and sorry it took so long to review...
I'll cut a new release either today or tomorrow.

squat reviewed Jun 17, 2022

View reviewed changes

injectproxy/routes.go Outdated Show resolved Hide resolved

Kirchen99 force-pushed the multi-value-or-regex branch from 53e7eeb to 5fbd088 Compare June 17, 2022 14:03

Kirchen99 requested a review from squat June 17, 2022 14:30

squat reviewed Jun 20, 2022

View reviewed changes

injectproxy/routes.go Show resolved Hide resolved

simonpasquier reviewed Jul 1, 2022

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

injectproxy/routes.go Outdated Show resolved Hide resolved

injectproxy/routes.go Show resolved Hide resolved

Kirchen99 force-pushed the multi-value-or-regex branch from 7e7980d to 4e21823 Compare July 4, 2022 11:47

Kirchen99 requested review from simonpasquier and squat July 4, 2022 12:36

simonpasquier reviewed Jul 8, 2022

View reviewed changes

Kirchen99 requested a review from simonpasquier July 11, 2022 13:24

simonpasquier reviewed Jul 18, 2022

View reviewed changes

injectproxy/routes.go Outdated Show resolved Hide resolved

injectproxy/routes.go Outdated Show resolved Hide resolved

main.go Outdated Show resolved Hide resolved

injectproxy/routes_test.go Outdated Show resolved Hide resolved

Kirchen99 force-pushed the multi-value-or-regex branch from 6387f2e to 47020f0 Compare July 18, 2022 15:04

Kirchen99 requested a review from simonpasquier July 18, 2022 15:34

simonpasquier reviewed Jul 19, 2022

View reviewed changes

injectproxy/routes.go Outdated Show resolved Hide resolved

simonpasquier reviewed Jul 19, 2022

View reviewed changes

injectproxy/routes.go Outdated Show resolved Hide resolved

simonpasquier reviewed Jul 19, 2022

View reviewed changes

injectproxy/routes.go Outdated Show resolved Hide resolved

Kirchen99 requested a review from simonpasquier July 20, 2022 10:50

simonpasquier reviewed Jul 20, 2022

View reviewed changes

injectproxy/routes.go Outdated Show resolved Hide resolved

injectproxy/routes.go Outdated Show resolved Hide resolved

injectproxy/routes.go Outdated Show resolved Hide resolved

Kirchen99 requested a review from simonpasquier July 22, 2022 14:10

simonpasquier reviewed Aug 4, 2022

View reviewed changes

Kirchen99 requested a review from simonpasquier August 5, 2022 11:52

dbluxo mentioned this pull request Oct 20, 2022

Option to specify multiple permitted label values #126

Closed

Kirchen99 and others added 13 commits June 12, 2023 15:10

remove unused new line

7aaf28f

Signed-off-by: Kirchen99 <latias_latios@126.com>

change the logic of endpoint 'silence'

768eb5a

Signed-off-by: Kirchen99 <latias_latios@126.com>

let me have a look at the result of rebase

c6aaa13

Signed-off-by: Kirchen99 <latias_latios@126.com>

fix some unexpected changes during the rebase

d9c70b3

Signed-off-by: Kirchen99 <latias_latios@126.com>

pass test cases

974e01a

Signed-off-by: Kirchen99 <latias_latios@126.com>

enable multi values for http header

9db24cf

Signed-off-by: Kirchen99 <latias_latios@126.com>

fix grammar

fd413ac

Signed-off-by: Kirchen99 <latias_latios@126.com>

remove old doc

dde1861

Signed-off-by: Kirchen99 <latias_latios@126.com>

remove unused code

15730b8

Signed-off-by: Kirchen99 <latias_latios@126.com>

apply suggestion, skip empty value and fix lint error

96d4cb1

Signed-off-by: Kirchen99 <latias_latios@126.com>

move sort function to MustLabelValues

c7b54be

Signed-off-by: Kirchen99 <latias_latios@126.com>

should not use sort in test

93cdf02

Signed-off-by: Kirchen99 <latias_latios@126.com>

chore: replace errors.Wrap with fmt.Errorf

8472eba

Signed-off-by: Kirchen99 <9981745+Kirchen99@users.noreply.github.com>

Kirchen99 force-pushed the multi-value-or-regex branch from c2083a3 to 8472eba Compare June 12, 2023 13:23

simonpasquier added 5 commits June 13, 2023 08:34

README.md: minor updates

040d5fd

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

Don't support multiple label values with the Silences API

8530327

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

Replace contains() by slices.Contains()

3847271

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

simonpasquier force-pushed the multi-value-or-regex branch from 661cf19 to 3847271 Compare June 13, 2023 08:56

simonpasquier requested a review from SuperQ June 13, 2023 09:41

simonpasquier reviewed Jun 13, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

simonpasquier approved these changes Jun 13, 2023

View reviewed changes

SuperQ approved these changes Jun 13, 2023

View reviewed changes

simonpasquier merged commit e4829ba into prometheus-community:main Jun 15, 2023

simonpasquier mentioned this pull request Jun 15, 2023

change injected PromQL label from labels.MatchEqual to labels. MatchRegexp to support multiple namespaces #27

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable multiple label values #115

feat: enable multiple label values #115

Kirchen99 commented Jun 17, 2022

simonpasquier left a comment

simonpasquier Jul 8, 2022

simonpasquier left a comment

simonpasquier left a comment

simonpasquier Aug 4, 2022

Kirchen99 Aug 5, 2022

Kirchen99 commented Sep 9, 2022

gasmick commented Sep 29, 2022

bt909 commented Oct 4, 2022

kekscode commented Oct 6, 2022

lud97x commented Oct 17, 2022

dbluxo commented Nov 9, 2022

simonpasquier commented Dec 2, 2022

simonpasquier commented Jan 4, 2023

simonpasquier commented Jun 12, 2023

simonpasquier commented Jun 13, 2023

Kirchen99 commented Jun 13, 2023

simonpasquier left a comment

simonpasquier commented Jun 15, 2023

	return "", fmt.Errorf("the query parameter can not be parsed. %s", err.Error())
	return "", fmt.Errorf("the query parameter can not be parsed: %w", err)

feat: enable multiple label values #115

feat: enable multiple label values #115

Conversation

Kirchen99 commented Jun 17, 2022

simonpasquier left a comment

Choose a reason for hiding this comment

simonpasquier Jul 8, 2022

Choose a reason for hiding this comment

simonpasquier left a comment

Choose a reason for hiding this comment

simonpasquier left a comment

Choose a reason for hiding this comment

simonpasquier Aug 4, 2022

Choose a reason for hiding this comment

Kirchen99 Aug 5, 2022

Choose a reason for hiding this comment

Kirchen99 commented Sep 9, 2022

gasmick commented Sep 29, 2022

bt909 commented Oct 4, 2022

kekscode commented Oct 6, 2022

lud97x commented Oct 17, 2022

dbluxo commented Nov 9, 2022

simonpasquier commented Dec 2, 2022

simonpasquier commented Jan 4, 2023

simonpasquier commented Jun 12, 2023

simonpasquier commented Jun 13, 2023

Kirchen99 commented Jun 13, 2023

simonpasquier left a comment

Choose a reason for hiding this comment

simonpasquier commented Jun 15, 2023