-
Notifications
You must be signed in to change notification settings - Fork 802
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
Change query fuzz test for bottomk, topk #6350
base: master
Are you sure you want to change the base?
Change query fuzz test for bottomk, topk #6350
Conversation
27fd5ea
to
84c1ffe
Compare
integration/query_fuzz_test.go
Outdated
nonDeterministicQuery := false | ||
for i := 0; i < runForNonDeterCheck; i++ { | ||
r1, err1 := c2.Query(query, queryTime) | ||
r2, err2 := c2.Query(query, queryTime) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you run the same bottomk or topk query against Prometheus or Cortex within a short period time it is highly likely to have the same result I think. I have tested this against Prometheus.
What we can do instead is to use a different result comparer to only check result series length rather than content if we see bottomk
or topk
in the query. Or we have dedicated test cases to test topk
, bottomk
and ignore them in regular test cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it sufficient to compare the length of the series?
Why do bottom
or topk
query results differ? I was thinking the reason is non-determinism. But, I feel like my thinking might be wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you said, it is non deterministic but the non deterministic is not that high I think. If you run 100 queries probably the issue occurs once or twice. I think running the same query with c2 is also highly likely to produce the same result so this can be still flaky.
Is it sufficient to compare the length of the series?
We can add other verification if find doable but also reduce flakiness. At least comparing series length is better than not testing those functions at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, yes.. the same query with c2 is also flaky.
What about comparing # of samples? The series length is also flaky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think number of samples is guaranteed the same. We randomized the timestamp of the first sample from each series so depending on your query you might be able to query some series but miss other series. When the result is non-deterministic depending on the returned series you will have different number of samples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change promsmith to not use bottomk/topk with some "time based" functions (day_of_week
, time_of_day
, etc) ? i think this is the bulk of the errors?
Maybe tbh we can even remove those functions from the fuzzy test, and and create a new fuzzy test just to test those functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alanprot
I haven't found the root cause yet.
I'm suspicious of all the same values. I tested the query bottomk by (job) (pi(), day_of_year(-{__name__="test_series_a"} offset 1m17s))
where I changed the values to return a random value (1~366) of day_of_year
.
Then, there is a high chance of success.
Btw, it sounds good to create the fuzz test testing those functions.
FYI. I changed https://github.com/cortexproject/cortex/blob/master/integration/e2e/util.go#L290 to ref, err = app.Append(ref, series[i], start, float64(time.Hour*4114*time.Duration(i+j+1)))
84c1ffe
to
246d811
Compare
246d811
to
87ab76d
Compare
Signed-off-by: SungJin1212 <[email protected]>
87ab76d
to
9b9e267
Compare
This PR improves the query fuzz test to skip non-deterministic queries.
It can prevent test fail due to the non-deterministic queries like
bottomk by (job) (pi(), days_in_month(-{__name__="test_series_a"} offset 1m17s))
.Change log
bottomk
andtopk
.Which issue(s) this PR fixes:
Fixes #6323
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]