-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
Improve TestQueryStatistics and fix bugs exposed by it #13667
Conversation
72016c4
to
14b42e8
Compare
Signed-off-by: beorn7 <beorn@grafana.com>
Signed-off-by: beorn7 <beorn@grafana.com>
Also, fix the bugs exposed by the tests. Signed-off-by: beorn7 <beorn@grafana.com>
14b42e8
to
f48c7a5
Compare
@@ -1221,7 +1224,6 @@ func (ev *evaluator) rangeEval(prepSeries func(labels.Labels, *EvalSeriesHelper) | |||
if ev.currentSamples > ev.maxSamples { | |||
ev.error(ErrTooManySamples(env)) | |||
} | |||
ev.samplesStats.UpdatePeak(ev.currentSamples) |
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.
This has already been done in line 1219.
The third commit makes major changes to PromQL engine, which is not described by the PR title or description. |
I mean, "major changes" == fixing how we determine if the max-sample-threshold was crossed. But your point is correct, the PR should mention that this fixes bugs. I have updated the title. (The commit description already mentioned that it fixes the bugs exposed by the improved tests.) |
@marctc this fixes your code, so you would be a suitable reviewer for this. Thank you very much. |
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.
LGTM! Love those new tests, great job!
These are follow-ups that arose from the review of #9626.
The improvements to TestQueryStatistics are only possible now that native histograms have been integrated into the PromQL testing framework. The improved test exposed a number of smaller bugs when counting histogram samples, mostly where the code originally assumed that the sample count would only get incremented by one and wrote conditions in a more concise style based on that. However, even a single histogram sample can increment the count by more than one, so the
if
statements had to be rearranged.