fix(filter): Wildcard trigger compatibility with graphite-clickhouse #896
Conversation
- there is a need to discuss the importance of the variable `allowMatchEmpty` because it makes the tests pass but breaks compatibility with graphite-clickhouse
lordvidex
changed the title
- when operators `=~` and `!=~` are used, both sides of the expression are matched LIKE in graphite.
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## master #896 +/- ##
==========================================
- Coverage 69.65% 68.93% -0.72%
==========================================
Files 191 195 +4
Lines 10561 10884 +323
==========================================
+ Hits 7356 7503 +147
- Misses 2779 2937 +158
- Partials 426 444 +18
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
filter/series_by_tag.go
Outdated
| matchingHandlerCondition = func(value string) bool { | ||
| return matchRegex.MatchString(value) | ||
| } | ||
| case NotMatchOperator: | ||
| matchRegex := regexp.MustCompile("^" + spec.Value) | ||
| value := cleanAsterisks(spec.Value) |
There was a problem hiding this comment.
Why is needed ? Replace * to .* needed only if full string is *. Also may be add more tests for *
There was a problem hiding this comment.
I replaced * with .* because in graphite-clickhouse, there is a greedy match. The SQL equivalent was something like %<string>% which basically means that both sides of the expressions are greedily matched.
Also, what we want when we query for cpu.machine*.loaded is not "zero or more characters of e" but any amount of characters after e. That is why it is needed for * to be changed to .* for this to work in regexp.
In the graphite-clickhouse source code, there is something like this too. Check https://github.com/go-graphite/graphite-clickhouse/blob/1db4b3d740c0f3e72ed8a3449603f4a016b5dda8/pkg/where/where.go#L64
There was a problem hiding this comment.
@msaf1980 I don't know if i missed any tests, I have tried the following special cases here: https://github.com/moira-alert/moira/pull/896/files#diff-4e74ec0972e1312934727b9a6b03635e81344c8d854205ea594290ad604bedc7
- empty
- single asterisks
- asterisk after an asterisk
- properly written asterisk should remain i.e.
.*=>..*❌.*=>.*✅
- when regexp has been anchored with
^and$ - prefix and suffix asterisks
- mixed (correct and incorrectly written) asterisks
There was a problem hiding this comment.
In graphite-clickhouse seriesByTags * replaced to .* only in wildcards (it's not regular expression, but translated to regexp for check). Only one case whan * translated to .* in regexp, is a bad regexp *.
There was a problem hiding this comment.
Also, add tests for regexp like a*b and a.*b for check why replacement * to .* is a bad thing.
- changing `*` to `.*` will result in unexpected behaviour when used with matchOperator. - matchOperator should contain valid regexp by default. - matchOperator when invalid should not panic but always return a non-matching callback
lordvidex
force-pushed
the
fix/wildcard-in-tagged-patterns
branch
from
df0c8be
to
ee3b5f1
Compare
- if tag name is not found, there should not be match - changed test to not match tags that cannot be found
|
/build |
| @@ -179,14 +214,14 @@ func createMatchingHandlerForOneTag(spec TagSpec) MatchingHandler { | |||
| } | |||
| } | |||
|
|
|||
| matchEmpty := matchingHandlerCondition("") | |||
| // matchEmpty := matchingHandlerCondition("") | |||
There was a problem hiding this comment.
I want to remove it, but I need @msaf1980 to check graphite-clickhouse and give a review about some errors that might depend on this.
|
Build and push Docker images with tag: fix-wildcard-in-tagged-patterns.2023-08-30.a45bc02 |
|
We had to split this PR into two parts, to deploy them seprately: #924 and #923, so I am going to close this PR @lordvidex, I mentioned you in these PR descriptions, thank you very much for your contribution! |
|
You're welcome! |
allowMatchEmptybecause it breaks compatibility withgraphite-clickhouseAdditional information
BEFORE: Triggers that contained tags like
tag1=~*with wildcard matching was matching for triggers that didn't have tag1 at allNOW: When wildcard is used, at least the
tag1key must be present in the trigger before it can be a valid match.