-
Notifications
You must be signed in to change notification settings - Fork 543
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
MQE: track number of processed samples in each query #10232
base: main
Are you sure you want to change the base?
Conversation
24164eb
to
344bfc7
Compare
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.
Thank you for being so quick on this. Looks good to me.
@@ -119,6 +120,8 @@ func (v *InstantVectorSelector) NextSeries(ctx context.Context) (types.InstantVe | |||
continue | |||
} | |||
|
|||
v.Stats.TotalSamples++ |
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.
Have we benchmarked this change?
One alternate would be to do v.Stats.TotalSamples += v.Selector.TimeRange.EndT / v.Selector.TimeRange.IntervalMilliseconds
(probably with floor
and an offset for the start or similar)
Or is this done here to catch stats for errored queries? I'm not sure if there would be value in that.
If not, alternatively we could look at the len
of the returned data.
// QueryStats tracks statistics about the execution of a single query. | ||
// | ||
// It is not safe to use this type from multiple goroutines simultaneously. | ||
type QueryStats struct { |
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.
Why create our own struct instead of using stats.QuerySamples and passing that to the appropriate operators?
It would also make it easier to implement TotalSamplesPerStep
if we want to support that too (which I think we do)
dense_series 0 1 2 3 4 5 6 7 8 9 10 | ||
start_series 0 1 _ _ _ _ _ _ _ _ _ | ||
end_series _ _ _ _ _ 5 6 7 8 9 10 | ||
sparse_series 0 _ _ _ _ _ _ 7 _ _ _ |
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.
should have some NH's to test too
Also some stale and NaN's etc.
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.
While addressing this I found out NH's were not being counted to samples as PromQL does. I have adjusted the calculation.
require.Equal(t, testCase.expectedTotalSamples, prometheusCount, "invalid test case: expected samples does not match value from Prometheus' engine") | ||
|
||
mimirCount := runQueryAndGetTotalSamples(t, mimirEngine, testCase.expr, testCase.isInstantQuery) | ||
require.Equal(t, testCase.expectedTotalSamples, mimirCount) |
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.
We can also compare the samples loaded as part of our test gauntlet if we expect it to be the same in all cases
56ffb11
to
5dea7f9
Compare
What this PR does
This PR adds support for tracking the number of samples processed in a query evaluated by MQE.
Which issue(s) this PR fixes or relates to
Resolves #10138
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]
.about-versioning.md
updated with experimental features.