feat: improve performance of first_over_time and last_over_time queries by sharding them
#11605
Conversation
|
Trivy scan found the following vulnerabilities:
|
jeschkies
force-pushed
the
karsten/first-over-time
branch
from
e70ccb5 to
8f589fe
Compare
Signed-off-by: Callum Styan <[email protected]>
Signed-off-by: Callum Styan <[email protected]>
Signed-off-by: Callum Styan <[email protected]>
Signed-off-by: Callum Styan <[email protected]>
Signed-off-by: Callum Styan <[email protected]>
Signed-off-by: Callum Styan <[email protected]>
Signed-off-by: Callum Styan <[email protected]>
Signed-off-by: Callum Styan <[email protected]>
Signed-off-by: Callum Styan <[email protected]>
in each vector, not the first Signed-off-by: Callum Styan <[email protected]>
There was a problem hiding this comment.
I'm extended this to include implementing last_over_time here.
My only comments would be:
- that renaming
first_over_time.goto something more generic, orfirst_last_over_time.golike I did in my branch, probably makes sense - some comments would be useful (such as where I commented on the PR) for first time readers of the code
pkg/logql/engine.go
Outdated
| T: ts, | ||
| //T: ts, |
There was a problem hiding this comment.
I'm not sure we can do it like this. What do you think, @cstyan?
There was a problem hiding this comment.
I'm not sure what you mean?
There was a problem hiding this comment.
The current code on main overrides he timestamps in each vector.
There was a problem hiding this comment.
isn't the timestamp from the actual vector samples more accurate than the one got from stepEvaluator.Next()?
Can you help me understand why it won't make sense?
There was a problem hiding this comment.
It does make sense for us. I'm just not sure some other component is relying on this overridden timestamp.
There was a problem hiding this comment.
LGTM. thanks for addressing the feedbacks.
Still this one applies I think.
I see you added tests in TestMappingEquivalence. But do you think should be also added on other tests like TestRangeMappingEquivalence on downstream engine tests? Rationale being the later tests for more cases of different range aggregation.
approving to unblock.
|
hey @kavirajk, actually something fun I learned this past week when finally finding the cause of the issues with our |
cstyan
changed the title
first_over_time and last_over_time queries.first_over_time and last_over_time queries by sharding them
|
@jeschkies any opinions on merging this as is vs as behind a feature flag? given that it's not being executed via a probabilistic data structure I don't think we need a feature flag |
Signed-off-by: Callum Styan <[email protected]>
Signed-off-by: Callum Styan <[email protected]>
Signed-off-by: Callum Styan <[email protected]>
…ueries by sharding them (#11605) Signed-off-by: Callum Styan <[email protected]> Co-authored-by: Callum Styan <[email protected]>
What this PR does / why we need it:
With the introduction of sending query plans with custom nodes to the querier we can now shard
first_over_timequeries.Checklist
CONTRIBUTING.mdguide (required)CHANGELOG.mdupdatedadd-to-release-noteslabeldocs/sources/setup/upgrade/_index.mdproduction/helm/loki/Chart.yamland updateproduction/helm/loki/CHANGELOG.mdandproduction/helm/loki/README.md. Example PRdeprecated-config.yamlanddeleted-config.yamlfiles respectively in thetools/deprecated-config-checkerdirectory. Example PR