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

Support UTF-8 label matchers: Add test for PromQL braces when parsing lists of matchers #3507

Conversation

grobinson-grafana
Copy link
Contributor

@grobinson-grafana grobinson-grafana commented Sep 5, 2023

This pull request has been added to address the discussion #3453 (comment). Should be merged before #3483.

@grobinson-grafana grobinson-grafana force-pushed the grobinson/add-test-for-promql-matchers branch 2 times, most recently from e6827e7 to 7785118 Compare September 5, 2023 09:00
Signed-off-by: George Robinson <george.robinson@grafana.com>
@grobinson-grafana
Copy link
Contributor Author

@gotjosh could you restart test_frontend?

@gotjosh
Copy link
Member

gotjosh commented Sep 8, 2023

I don't think this is what I meant with #3453 (comment)

My understanding is that the support of {} is basically a breaking change. The idea behind the test is to make sure that when (or if) we're ready to make the breaking changes non-optional this test fails so that we can ensure we're correct.

This is assuming that if we're all done with the parser today we'd have no actual coverage for this scenario - if you have any other ideas or I'm just completely off please feel free to tell me so.

Signed-off-by: George Robinson <george.robinson@grafana.com>
@grobinson-grafana
Copy link
Contributor Author

As discussed offline, we are going to replace the unit test with acceptance tests. I've just pushed that commit this morning, and so a second review would be great whenever convenient! 🙂

Signed-off-by: George Robinson <george.robinson@grafana.com>
silence2 := Silence(0, 4).Match("foo").Comment("test foo")
silence1 := Silence(0, 4).Match("test1", "severity=warn").Comment("test1")
silence2 := Silence(0, 4).Match("alertname=test2", "severity=warn").Comment("test2")
silence3 := Silence(0, 4).Match("{alertname=test3}", "severity=warn").Comment("test3")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gotjosh here I create a silence with the following labels:

alertname="{alertname=test3}"
severity=warn

We will be able to then see this behaviour break when we use the new fallback parser as without the additional check we talked about in #3453 (comment), it will create a silence with the following labels instead:

alertname=test3
severity=warn

Copy link
Member

@gotjosh gotjosh left a comment

Choose a reason for hiding this comment

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

LGTM

@gotjosh gotjosh merged commit 5ba9b9c into prometheus:main Sep 12, 2023
5 checks passed
qinxx108 pushed a commit to qinxx108/alertmanager that referenced this pull request Sep 13, 2023
…#3507)

* Add test for PromQL braces when parsing lists of matchers

Signed-off-by: George Robinson <george.robinson@grafana.com>

* Use acceptance tests

Signed-off-by: George Robinson <george.robinson@grafana.com>

* Add test creating silence with braces

Signed-off-by: George Robinson <george.robinson@grafana.com>

---------

Signed-off-by: George Robinson <george.robinson@grafana.com>
Signed-off-by: Yijie Qin <qinyijie@amazon.com>
@grobinson-grafana grobinson-grafana changed the title Add test for PromQL braces when parsing lists of matchers Support UTF-8 label matchers: Add test for PromQL braces when parsing lists of matchers Sep 13, 2023
qinxx108 pushed a commit to qinxx108/alertmanager that referenced this pull request Sep 14, 2023
…#3507)

* Add test for PromQL braces when parsing lists of matchers

Signed-off-by: George Robinson <george.robinson@grafana.com>

* Use acceptance tests

Signed-off-by: George Robinson <george.robinson@grafana.com>

* Add test creating silence with braces

Signed-off-by: George Robinson <george.robinson@grafana.com>

---------

Signed-off-by: George Robinson <george.robinson@grafana.com>
Signed-off-by: Yijie Qin <qinyijie@amazon.com>
alexweav pushed a commit to grafana/prometheus-alertmanager that referenced this pull request Oct 27, 2023
…#3507)

* Add test for PromQL braces when parsing lists of matchers

Signed-off-by: George Robinson <george.robinson@grafana.com>

* Use acceptance tests

Signed-off-by: George Robinson <george.robinson@grafana.com>

* Add test creating silence with braces

Signed-off-by: George Robinson <george.robinson@grafana.com>

---------

Signed-off-by: George Robinson <george.robinson@grafana.com>
alexweav pushed a commit to grafana/prometheus-alertmanager that referenced this pull request Oct 27, 2023
…#3507)

* Add test for PromQL braces when parsing lists of matchers

Signed-off-by: George Robinson <george.robinson@grafana.com>

* Use acceptance tests

Signed-off-by: George Robinson <george.robinson@grafana.com>

* Add test creating silence with braces

Signed-off-by: George Robinson <george.robinson@grafana.com>

---------

Signed-off-by: George Robinson <george.robinson@grafana.com>
@grobinson-grafana grobinson-grafana deleted the grobinson/add-test-for-promql-matchers branch April 16, 2024 14:44
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.

None yet

2 participants