[4189] Fix colliding filters by updating match method for MultiCharSequenceFilter to iterate through all patterns for the most complete match and not return on the first pattern found #4188

saad-zaman · 2023-02-15T21:39:59Z

What this PR does / why we need it: Currently, the multiCharSequenceFilter matches function returns on the first pattern found here even if there are other patterns available with better matches. This results in a suboptimal byte[] val being returned which causes the public matches function to return a false negative here. The len(val) is not zero because a suboptimal val was returned when a better pattern could have been found if the multiCharSequenceFilter matches function iterated through all patterns available and looked for the best match.

Fixes # #4189

Special notes for your reviewer: Please provide any go style improvements or tips you think are relevant

Does this PR introduce a user-facing and/or backwards incompatible change?:

NONE

Does this PR require updating code package or user-facing documentation?:

NONE

Benchmark Test Results Before and After Changes to matches method of multiCharSequenceFilter:

testname	old-bench-test-results.txt sec/op	new-bench-test-results.txt sec/op	sec/op vs base
MultiRangeFilterPrefixCollision-10	10.61n ± 1%	16.27n ± 4%	+53.47% (p=0.000 n=10)
MultiRangeFilterSuffixCollision-10	11.51n ± 7%	17.00n ± 3%	+47.65% (p=0.000 n=10)
MultiRangeFilterTwo-10	17.48n ± 5%	17.91n ± 2%	+2.43% (p=0.004 n=10)
MultiRangeFilterSelectTwo-10	21.45n ± 3%	24.30n ± 2%	+13.31% (p=0.000 n=10)
MultiRangeFilterTrieTwo-10	16.44n ± 4%	16.07n ± 2%	~ (p=0.342 n=10)
MultiRangeFilterSix-10	38.89n ± 1%	42.51n ± 2%	+9.32% (p=0.000 n=10)
MultiRangeFilterSelectSix-10	56.47n ± 0%	65.47n ± 1%	+15.94% (p=0.000 n=10)
MultiRangeFilterTrieSix-10	33.67n ± 0%	33.97n ± 4%	+0.89% (p=0.005 n=10)
geomean	22.04n	25.57n	+16.04%

…ilter to iterate through all patterns for the most complete match and not return on the first pattern found

CLAassistant · 2023-02-15T21:40:06Z

All committers have signed the CLA.

justinjc · 2023-02-15T23:28:11Z

src/metrics/filters/filter_test.go

@@ -27,6 +27,19 @@ import (
 	"github.com/stretchr/testify/require"
 )

+func TestPrefixCompositeR2Filter(t *testing.T) {


Seems like a better place to add these test cases is in TestMultiCharSequenceFilter in this file.

Agreed, I also think the naming also isn't great. Is R2 naming only relevant internally to Uber?

Nah, there's R2 references aplenty in OSS world.

justinjc · 2023-02-15T23:47:23Z

It's not immediately clear that this is a bug without mentioning how it's used in the public boolean Matches function here.

Maybe explicitly mention this in the description and how it would cause false negatives.

justinjc · 2023-02-15T23:47:58Z

src/metrics/filters/filter.go

-			return val[len(pattern):], true
+		if f.backwards {
+			if bytes.HasSuffix(val, pattern) {
+				if bestPattern == nil || len(pattern) > len(bestPattern) {


Nit: seems like the bestPattern == nil isn't needed

Good catch, updated.

justinjc · 2023-02-15T23:52:09Z

src/metrics/filters/filter_test.go

@@ -349,6 +349,11 @@ func TestMultiCharSequenceFilter(t *testing.T) {
 	validateLookup(t, f, "12test", true, "12")
 	validateLookup(t, f, "12test2", true, "12")
 	validateLookup(t, f, "123book", true, "123")
+
+	f, err = newMultiCharSequenceFilter([]byte("arachne_failures,arachne_failures_by_rack"), false)


Add a similar test case where backwards == true.

…patterns and backwards enabled

saad-zaman · 2023-02-16T00:24:13Z

It's not immediately clear that this is a bug without mentioning how it's used in the public boolean Matches function here.

Maybe explicitly mention this in the description and how it would cause false negatives.

Updated in the description and the associated issue.

shaan420 · 2023-02-16T07:57:43Z

src/metrics/filters/filter_test.go

+	id1 := "arachne_failures_by_rack"
+	f, err := newMultiCharSequenceFilter([]byte("arachne_failures,arachne_failures_by_rack"), false)
+	require.NoError(t, err)
+	_, matches1 := f.matches([]byte(id0))


Maybe I'm understanding the requirement wrong: Should "arachne_failures" match "arachne_failures" or "arachne_failures_by_rack" ? If your expectation was to match with "arachne_failures" with "arachne_failures" then wouldn't "len(pattern) > len(bestPattern)" make it match "arachne_failures_by_rack" ?

Follow-ups:

Could you also assert the returned matchIndex ?

Maybe add similar bestMatch testcase for backwards (HasSuffix) case as well ?

I think you're looking at an older commit possibly.

"arachne_failures" should match with "arachne-failures" and "arachne_failures_by_rack" should match with "arachne_failures_by_rack" even if it also matches with other patterns in the filter since this pattern is the best match.

In the latest commit I have tests for the length of the value in the returned byte array and with backwards set to true.

Gotcha! Yeah was looking at the older commit. Thanks for the update!

prateek · 2023-02-16T16:36:10Z

@saad-zaman before landing, can you run existing/make new benchmarks for this change too -- https://dave.cheney.net/2013/06/30/how-to-write-benchmarks-in-go is a good guide to get you going.

saad-zaman · 2023-02-16T20:44:34Z

src/metrics/filters/filter_benchmark_test.go

@@ -125,6 +125,14 @@ func BenchmarkRangeFilterRangeMatchNegation(b *testing.B) {
 	benchRangeFilterRange(b, []byte("!a-z"), []byte("p"), false)
 }

+func BenchmarkMultiRangeFilterPrefixCollision(b *testing.B) {


@prateek PTAL

Please put the results of the benchmark before and after the change.

To Justin's point - please add a comment to the code with "After change results"

And in the PR, update description with both before/after results

Updated the PR description to add benchmark results and a table to help with comparison. Also updated the code to add a small optimization to stop iterating through patterns if an optimal match was found and updated tests to ensure it was working

Updated table with a more legible benchmark test comparison

Can you add b.ReportAllocs() to these benchmarks?

Having a >50% impact seems pretty steep for this change, but I'm not seeing any obvious ways to make this function faster. Perhaps this impact is just very high because we're purposely checking the scenario where these "colliding filters" are present. Can we see the before/after numbers for the vanilla case where there aren't any colliding filters?

I ran it with b.ReportAllocs() and there are 0 allocs with the old and new logic.

Updated the tests to show the change for all benchmark tests related to my code. There is a small difference (~0-15%) in test cases where the prefix/suffix collision does not occur likely due to the additional conditionals present after a prefix or suffix detection is found.

…ugh patterns if complete match is found

Fix colliding filters by updating match method for MultiCharSequenceF…

caeb6f6

…ilter to iterate through all patterns for the most complete match and not return on the first pattern found

saad-zaman requested review from justinjc and prateek February 15, 2023 21:40

saad-zaman added 2 commits February 15, 2023 15:01

Remove empty newline

6086f86

Update new unit test to test length of returned byte array

1b9ea72

justinjc reviewed Feb 15, 2023

View reviewed changes

Update location of new test added to fold into an existing test

4b36d5f

justinjc reviewed Feb 15, 2023

View reviewed changes

Simplify code and add test for multiCharSequenceFilter with multiple …

8f0402a

…patterns and backwards enabled

saad-zaman marked this pull request as ready for review February 16, 2023 00:37

shaan420 reviewed Feb 16, 2023

View reviewed changes

shaan420 approved these changes Feb 16, 2023

View reviewed changes

Add benchmarks tests for new logic in multicharsequence matches method

2222592

saad-zaman commented Feb 16, 2023

View reviewed changes

saad-zaman added 2 commits February 16, 2023 14:12

Add benchmark tests and optimizations to short circuit iterating thro…

604df47

…ugh patterns if complete match is found

Fix linting error

dceb8da

justinjc approved these changes Feb 22, 2023

View reviewed changes

saad-zaman enabled auto-merge (squash) February 22, 2023 18:02

Merge branch 'master' into szaman/M3-2186/fix-colliding-filters

64fe147

saad-zaman merged commit 3afc5be into master Feb 22, 2023

saad-zaman deleted the szaman/M3-2186/fix-colliding-filters branch February 22, 2023 18:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[4189] Fix colliding filters by updating match method for MultiCharSequenceFilter to iterate through all patterns for the most complete match and not return on the first pattern found #4188

[4189] Fix colliding filters by updating match method for MultiCharSequenceFilter to iterate through all patterns for the most complete match and not return on the first pattern found #4188

saad-zaman commented Feb 15, 2023 •

edited

Loading

CLAassistant commented Feb 15, 2023 •

edited

Loading

justinjc Feb 15, 2023

saad-zaman Feb 15, 2023

justinjc Feb 15, 2023

justinjc commented Feb 15, 2023

justinjc Feb 15, 2023

saad-zaman Feb 16, 2023

justinjc Feb 15, 2023

saad-zaman Feb 16, 2023

saad-zaman commented Feb 16, 2023

shaan420 Feb 16, 2023

saad-zaman Feb 16, 2023

shaan420 Feb 16, 2023

prateek commented Feb 16, 2023

saad-zaman Feb 16, 2023

justinjc Feb 16, 2023

prateek Feb 16, 2023

saad-zaman Feb 16, 2023

saad-zaman Feb 18, 2023

justinjc Feb 21, 2023

saad-zaman Feb 22, 2023

[4189] Fix colliding filters by updating match method for MultiCharSequenceFilter to iterate through all patterns for the most complete match and not return on the first pattern found #4188

[4189] Fix colliding filters by updating match method for MultiCharSequenceFilter to iterate through all patterns for the most complete match and not return on the first pattern found #4188

Conversation

saad-zaman commented Feb 15, 2023 • edited Loading

CLAassistant commented Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinjc commented Feb 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saad-zaman commented Feb 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateek commented Feb 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saad-zaman commented Feb 15, 2023 •

edited

Loading

CLAassistant commented Feb 15, 2023 •

edited

Loading