-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Redesigining randomized tests for TS functions #129971
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
Conversation
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
|
@pabloem The approach looks good to me. I ran a hundred iterations and got some failures. Could you take a look? Thank you! |
.../esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/TimeSeriesRateIT.java
Outdated
Show resolved
Hide resolved
.../esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/TimeSeriesRateIT.java
Outdated
Show resolved
Hide resolved
.../esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/TimeSeriesRateIT.java
Outdated
Show resolved
Hide resolved
| var requestCount = requestCounts.compute(host, (k, curr) -> { | ||
| // 15% chance of reset | ||
| if (randomInt(100) <= 15) { | ||
| return Math.toIntExact(Math.round(hostToRate.get(k) * tsChange)); |
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.
Don't we want to add some randomization here too? Otherwise, it's just linear?
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.
the test keeps a randomly-generated, constant linear-rate per host - with random resets. this is how we avoid re-calculating rates
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.
Sounds good, let's document this. I think we can revisit this separately, e.g. store samples in an array per time-series and get the expected rate per interval.
| return Math.toIntExact(Math.round((curr == null ? 0 : curr) + hostToRate.get(k) * tsChange)); | ||
| } | ||
| }); | ||
| if (hosts.contains(host)) { |
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.
Nit: should we skip first, before initializing requestCount?
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.
no, because requestcount follows a linear rate - we always need to calculate for every point in time to be able to keep the same rate. lmk if that makes sense. I can try to rephrase.
.../esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/TimeSeriesRateIT.java
Show resolved
Hide resolved
.../esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/TimeSeriesRateIT.java
Outdated
Show resolved
Hide resolved
.../esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/TimeSeriesRateIT.java
Outdated
Show resolved
Hide resolved
dnhatn
left a comment
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 like the idea. LGTM, thanks @pabloem
|
|
@dnhatn the failure rate is now around 5% - still pretty high. It comes from outliers in the sampling rate. WDYT? a couple ideas ...
|
|
Either way works for me. |
Fixes #129567
This is a redesign of a couple test cases in TimeSeriesIT. The change does the following:
Replicating the same algo in test and db seemed off, so I re-designed the test to do the following:
Some assumptions from the test:
Happy to discuss if the margin of error is just too wide.
PTAL @dnhatn