Added matcher syntax support for Routing tree editor. #2065

Evreke · 2021-11-20T09:26:30Z

I added matchers syntax support to alertmanager routing tree editor
PR closes #1638 closes #1965

* feat: Added matcher syntax support for Alertmanager visual editor. First four examples from here https://prometheus.io/docs/alerting/latest/configuration/#matcher should be fine. The last one has an issue, escaped quotes make regexp test fail. Signed-off-by: Alex Antipin <[email protected]>

* fix: parseSearch method behave incorrect for matchers syntax: It's intended to split string to label\value pairs, but it would split values also if comma is present there. It is legitimate to have comma in values according to matcher value rules listed here https://prometheus.io/docs/alerting/latest/configuration/#matcher Also it couldn't handle strings properly by ignoring escaped quotes that is produced after new syntax parsing. Signed-off-by: Alex Antipin <[email protected]>

juliusv · 2021-11-23T17:45:21Z

Hi, thanks!

I played around with it a bit at https://deploy-preview-2065--prometheus-docs.netlify.app/webtools/alerting/routing-tree-editor/ and it seems to only work when not putting any spaces around the match op, e.g. foo=~bar works, but foo =~ bar does not. But the docs at https://prometheus.io/docs/alerting/latest/configuration/#matcher mainly show the style including the spaces, which confused me initially... could we support whitespace here as well?

Also, should a regex matcher like foo=~bar be shown as foo: /^(?:bar)$/ in the tree view? While technically not incorrect, should we maybe show the original string value instead? Also, I see != being listed as isRegex = true; currently, but that shouldn't be a regex match, right?

Generally: For now going about it in this direction seems fine, but long term I think this piece of JS is not easy to maintain or get fully correct. At some point it would be great for this to have a Go backend that could do proper validation of everything and reuse the Go datatypes + code for all of this and just return a tree to the UI... kind of like PromLens (https://promlens.com/) is doing for PromQL.

Evreke · 2021-11-24T08:19:38Z

I understood the comments. I'll see what can be done

You are right about maintainability of that solution and using existing code base would be preferable over reinventing the wheel in JS.
I'd like to work on it in next iteration.
Would you like to consider webassembly solution or traditional way with some external backend would be preferable?

juliusv · 2021-11-25T14:08:17Z

@Evreke Maybe webasm could be a solution, but I know very little about it so far... the other option would be just having a Go backend. Of course that would need to be hosted somewhere, which is annoying.

But yeah, the current piece of JS:

has no tests
is not typed (plain old single JS file)
has to (imperfectly) re-create a lot of the logic from the Go code

...which is not great.

* fix: there could be zero or one heading\trailing spaces around selector Signed-off-by: Alex Antipin <[email protected]>

Evreke · 2021-11-26T06:20:12Z

@juliusv
Totally agree. Code should be refactored.
This is a big work and it should be done carefully. I would prefer to create one or more issues to discuss and plan it there.

About comments.

I add regexp fix to capture single or zero spaces around selector.
About =~ matcher and non regexp value. This could be done but things will complicated even more as we need to check value is it regexp or not. Yeah it could be interpreted as regular equals matcher but it's just not worth it.
About using non regexp value for negate matcher !=. If we would like to avoid regexp for != we will lose lucidity and make things more complicated. The most obvious solution from my perspective is to introduce new field and check it in matchLabel but it wouldn't be used in other cases.

I think 2,3 is just a waste of time in case if we want to rewrite this part of code to be evaluated by WASM\external API or something else. Correct me if i'm wrong.

In summary:
In my opinion, now it is better to refactor, so as not to get bogged down in fixing notoriously difficult decisions that will entail more complication.

Evreke · 2021-12-01T06:34:36Z

@juliusv
I've done small research and found that using WASM could be difficult from debugging perspective. We don't have mature debugging instruments yet. Also it could be difficult or impossible just to reuse existing codebase due to lots of package dependencies.

So we have to look at hosted backend for reusing existing code base.

juliusv · 2021-12-05T09:34:40Z

@Evreke Thanks, yeah, makes sense! I'll be honest, I won't have too much time to put into something completely new with a hosted backend (especially since that needs to be run somewhere separately and maintained by the team). I think we have two options:

For now, try to make as much as possible work with the current script, although it's not going to be perfect.
If someone wants to really own and be responsible for a new and better version of this with a hosted backend, that would also be great. It could first be built standalone, and then we could eventually figure out where to run it (e.g. on Google Cloud Run).

Evreke · 2021-12-07T07:04:32Z

@juliusv

Sounds reasonable.

I'll watch what could be done on your second and third comment if you insist but from my point of view it is not necessary.
Will try to finish it this week.

juliusv · 2021-12-10T14:44:57Z

I'll watch what could be done on your second and third comment if you insist but from my point of view it is not necessary.

With the third one, do you mean the one about != not being a regex? As far as I can see that part actually needs to be fixed, because right now the non-regex value is just put into a regex, so if it contains any special regex characters (like .), they will be interpreted as such, although they're not supposed to be.

Evreke · 2021-12-15T08:31:20Z

@juliusv
You are right! Appreciate, my bad.
I add immutable object Matcher to hold operators and I rewrote my part using it.
Now evaluating deprecated matchers tends to separate from my solution.

Evreke · 2022-01-20T08:53:18Z

@juliusv
Hi Julius! I congratulate you on the past holidays! Are we okay with the current solution?

juliusv

Thanks and sorry for the extended pause on my side! Looks pretty good to me, one small question.

static/routing-tree.js

* add: Matcher enum-like object holding relevant PromQL matcher operators * add: matchNewSyntax method to check equality for values using Matcher operator * fix: negative matchers should not be considered as Regexp Signed-off-by: Alex Antipin <[email protected]>

juliusv · 2022-02-09T19:55:26Z

Great, thank you again for the work and the patience :) 👍

* Added feature for matcher syntax to Visual Editor. (prometheus#1638) * feat: Added matcher syntax support for Alertmanager visual editor. First four examples from here https://prometheus.io/docs/alerting/latest/configuration/#matcher should be fine. The last one has an issue, escaped quotes make regexp test fail. Signed-off-by: Alex Antipin <[email protected]> * Added feature for matcher syntax to Visual Editor. (prometheus#1638) * fix: parseSearch method behave incorrect for matchers syntax: It's intended to split string to label\value pairs, but it would split values also if comma is present there. It is legitimate to have comma in values according to matcher value rules listed here https://prometheus.io/docs/alerting/latest/configuration/#matcher Also it couldn't handle strings properly by ignoring escaped quotes that is produced after new syntax parsing. Signed-off-by: Alex Antipin <[email protected]> * Added feature for matcher syntax to Visual Editor. (prometheus#1638) * fix: there could be zero or one heading\trailing spaces around selector Signed-off-by: Alex Antipin <[email protected]> * Added feature for matcher syntax to Visual Editor. (prometheus#1638) * add: Matcher enum-like object holding relevant PromQL matcher operators * add: matchNewSyntax method to check equality for values using Matcher operator * fix: negative matchers should not be considered as Regexp Signed-off-by: Alex Antipin <[email protected]> Signed-off-by: cstef <[email protected]>

Evreke added 2 commits November 20, 2021 11:49

Evreke mentioned this pull request Nov 20, 2021

Routing Tree Editor website not accurately simulating routes #1638

Closed

Added feature for matcher syntax to Visual Editor. (prometheus#1638)

fae9e2f

* fix: there could be zero or one heading\trailing spaces around selector Signed-off-by: Alex Antipin <[email protected]>

Evreke force-pushed the evreke/matchers-syntax branch from 7851681 to 56c6541 Compare December 15, 2021 08:17

juliusv reviewed Feb 1, 2022

View reviewed changes

static/routing-tree.js Outdated Show resolved Hide resolved

Evreke force-pushed the evreke/matchers-syntax branch 3 times, most recently from 6c475d8 to 83fba0c Compare February 6, 2022 05:46

Evreke force-pushed the evreke/matchers-syntax branch from 83fba0c to 3500e07 Compare February 6, 2022 05:50

Evreke requested a review from juliusv February 8, 2022 06:12

juliusv merged commit 6c1f1a3 into prometheus:main Feb 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added matcher syntax support for Routing tree editor. #2065

Added matcher syntax support for Routing tree editor. #2065

Evreke commented Nov 20, 2021 •

edited

Loading

juliusv commented Nov 23, 2021

Evreke commented Nov 24, 2021 •

edited

Loading

juliusv commented Nov 25, 2021

Evreke commented Nov 26, 2021 •

edited

Loading

Evreke commented Dec 1, 2021

juliusv commented Dec 5, 2021

Evreke commented Dec 7, 2021

juliusv commented Dec 10, 2021

Evreke commented Dec 15, 2021 •

edited

Loading

Evreke commented Jan 20, 2022 •

edited

Loading

juliusv left a comment

juliusv commented Feb 9, 2022

Added matcher syntax support for Routing tree editor. #2065

Added matcher syntax support for Routing tree editor. #2065

Conversation

Evreke commented Nov 20, 2021 • edited Loading

juliusv commented Nov 23, 2021

Evreke commented Nov 24, 2021 • edited Loading

juliusv commented Nov 25, 2021

Evreke commented Nov 26, 2021 • edited Loading

Evreke commented Dec 1, 2021

juliusv commented Dec 5, 2021

Evreke commented Dec 7, 2021

juliusv commented Dec 10, 2021

Evreke commented Dec 15, 2021 • edited Loading

Evreke commented Jan 20, 2022 • edited Loading

juliusv left a comment

Choose a reason for hiding this comment

juliusv commented Feb 9, 2022

Evreke commented Nov 20, 2021 •

edited

Loading

Evreke commented Nov 24, 2021 •

edited

Loading

Evreke commented Nov 26, 2021 •

edited

Loading

Evreke commented Dec 15, 2021 •

edited

Loading

Evreke commented Jan 20, 2022 •

edited

Loading